From e77037e20dfb385d67ff8e8612e001147253c20c Mon Sep 17 00:00:00 2001 From: Lars Reimann Date: Tue, 10 Oct 2023 15:12:44 +0200 Subject: [PATCH] feat: various checks for annotations on parameters and results (#625) Closes partially #543 ### Summary of Changes * Annotations must not be used on parameters of callable types * Annotations must not be used on results of callable types * Annotations must not be used on parameters of lambdas * `@Deprecated` must not be used on required parameters * `@Expert` must not be used on required parameters --- src/language/builtins/safe-ds-annotations.ts | 40 +++++++++------ .../validation/builtins/deprecated.ts | 17 +++++++ src/language/validation/builtins/expert.ts | 20 ++++++++ .../other/declarations/annotationCalls.ts | 51 +++++++++++++++++++ src/language/validation/safe-ds-validator.ts | 17 ++++++- .../safeds/lang/coreAnnotations.sdsstub | 5 ++ .../safeds/lang/documentation.sdsstub | 5 -- .../main.sdstest | 48 +++++++++++++++++ .../main.sdstest | 48 +++++++++++++++++ .../main.sdstest | 16 ++++++ .../main.sdstest | 10 ++++ .../main.sdstest | 10 ++++ 12 files changed, 265 insertions(+), 22 deletions(-) create mode 100644 src/language/validation/builtins/expert.ts create mode 100644 src/language/validation/other/declarations/annotationCalls.ts create mode 100644 tests/resources/validation/builtins/annotations/deprecated/must not be used on required parameters/main.sdstest create mode 100644 tests/resources/validation/builtins/annotations/expert/must not be used on required parameters/main.sdstest create mode 100644 tests/resources/validation/other/declarations/annotation calls/must not be used on lambda parameters/main.sdstest create mode 100644 tests/resources/validation/other/declarations/annotation calls/must not be used on parameters of callable types/main.sdstest create mode 100644 tests/resources/validation/other/declarations/annotation calls/must not be used on results of callable types/main.sdstest diff --git a/src/language/builtins/safe-ds-annotations.ts b/src/language/builtins/safe-ds-annotations.ts index c92461046..e7e6d1a24 100644 --- a/src/language/builtins/safe-ds-annotations.ts +++ b/src/language/builtins/safe-ds-annotations.ts @@ -1,34 +1,44 @@ -import { isSdsAnnotation, SdsAnnotatedObject, SdsAnnotation } from '../generated/ast.js'; +import { isSdsAnnotation, SdsAnnotatedObject, SdsAnnotation, SdsParameter } from '../generated/ast.js'; import { annotationCallsOrEmpty } from '../helpers/nodeProperties.js'; import { SafeDsModuleMembers } from './safe-ds-module-members.js'; import { resourceNameToUri } from '../../helpers/resources.js'; +import { URI } from 'langium'; const CORE_ANNOTATIONS_URI = resourceNameToUri('builtins/safeds/lang/coreAnnotations.sdsstub'); export class SafeDsAnnotations extends SafeDsModuleMembers { isDeprecated(node: SdsAnnotatedObject | undefined): boolean { - return annotationCallsOrEmpty(node).some((it) => { - const annotation = it.annotation?.ref; - return annotation === this.Deprecated; - }); + return this.hasAnnotationCallOf(node, this.Deprecated); } - isExperimental(node: SdsAnnotatedObject | undefined): boolean { - return annotationCallsOrEmpty(node).some((it) => { - const annotation = it.annotation?.ref; - return annotation === this.Experimental; - }); + private get Deprecated(): SdsAnnotation | undefined { + return this.getAnnotation(CORE_ANNOTATIONS_URI, 'Deprecated'); } - private get Deprecated(): SdsAnnotation | undefined { - return this.getAnnotation('Deprecated'); + isExperimental(node: SdsAnnotatedObject | undefined): boolean { + return this.hasAnnotationCallOf(node, this.Experimental); } private get Experimental(): SdsAnnotation | undefined { - return this.getAnnotation('Experimental'); + return this.getAnnotation(CORE_ANNOTATIONS_URI, 'Experimental'); + } + + isExpert(node: SdsParameter | undefined): boolean { + return this.hasAnnotationCallOf(node, this.Expert); + } + + private get Expert(): SdsAnnotation | undefined { + return this.getAnnotation(CORE_ANNOTATIONS_URI, 'Expert'); + } + + private hasAnnotationCallOf(node: SdsAnnotatedObject | undefined, expected: SdsAnnotation | undefined): boolean { + return annotationCallsOrEmpty(node).some((it) => { + const actual = it.annotation?.ref; + return actual === expected; + }); } - private getAnnotation(name: string): SdsAnnotation | undefined { - return this.getModuleMember(CORE_ANNOTATIONS_URI, name, isSdsAnnotation); + private getAnnotation(uri: URI, name: string): SdsAnnotation | undefined { + return this.getModuleMember(uri, name, isSdsAnnotation); } } diff --git a/src/language/validation/builtins/deprecated.ts b/src/language/validation/builtins/deprecated.ts index 9b2335602..208a0c468 100644 --- a/src/language/validation/builtins/deprecated.ts +++ b/src/language/validation/builtins/deprecated.ts @@ -7,14 +7,18 @@ import { SdsArgument, SdsAssignee, SdsNamedType, + SdsParameter, SdsReference, } from '../../generated/ast.js'; import { SafeDsServices } from '../../safe-ds-module.js'; +import { isRequiredParameter } from '../../helpers/nodeProperties.js'; +import { parameterCanBeAnnotated } from '../other/declarations/annotationCalls.js'; export const CODE_DEPRECATED_ASSIGNED_RESULT = 'deprecated/assigned-result'; export const CODE_DEPRECATED_CALLED_ANNOTATION = 'deprecated/called-annotation'; export const CODE_DEPRECATED_CORRESPONDING_PARAMETER = 'deprecated/corresponding-parameter'; export const CODE_DEPRECATED_REFERENCED_DECLARATION = 'deprecated/referenced-declaration'; +export const CODE_DEPRECATED_REQUIRED_PARAMETER = 'deprecated/required-parameter'; export const assigneeAssignedResultShouldNotBeDeprecated = (services: SafeDsServices) => (node: SdsAssignee, accept: ValidationAcceptor) => { @@ -95,3 +99,16 @@ export const referenceTargetShouldNotBeDeprecated = }); } }; + +export const requiredParameterMustNotBeDeprecated = + (services: SafeDsServices) => (node: SdsParameter, accept: ValidationAcceptor) => { + if (isRequiredParameter(node) && parameterCanBeAnnotated(node)) { + if (services.builtins.Annotations.isDeprecated(node)) { + accept('error', 'A deprecated parameter must be optional.', { + node, + property: 'name', + code: CODE_DEPRECATED_REQUIRED_PARAMETER, + }); + } + } + }; diff --git a/src/language/validation/builtins/expert.ts b/src/language/validation/builtins/expert.ts new file mode 100644 index 000000000..df8995508 --- /dev/null +++ b/src/language/validation/builtins/expert.ts @@ -0,0 +1,20 @@ +import { ValidationAcceptor } from 'langium'; +import { SdsParameter } from '../../generated/ast.js'; +import { SafeDsServices } from '../../safe-ds-module.js'; +import { isRequiredParameter } from '../../helpers/nodeProperties.js'; +import { parameterCanBeAnnotated } from '../other/declarations/annotationCalls.js'; + +export const CODE_EXPERT_TARGET_PARAMETER = 'expert/target-parameter'; + +export const requiredParameterMustNotBeExpert = + (services: SafeDsServices) => (node: SdsParameter, accept: ValidationAcceptor) => { + if (isRequiredParameter(node) && parameterCanBeAnnotated(node)) { + if (services.builtins.Annotations.isExpert(node)) { + accept('error', 'An expert parameter must be optional.', { + node, + property: 'name', + code: CODE_EXPERT_TARGET_PARAMETER, + }); + } + } + }; diff --git a/src/language/validation/other/declarations/annotationCalls.ts b/src/language/validation/other/declarations/annotationCalls.ts new file mode 100644 index 000000000..3951ca7d8 --- /dev/null +++ b/src/language/validation/other/declarations/annotationCalls.ts @@ -0,0 +1,51 @@ +import { + isSdsCallable, + isSdsCallableType, + isSdsLambda, + SdsCallableType, + SdsLambda, + SdsParameter, +} from '../../../generated/ast.js'; +import { getContainerOfType, ValidationAcceptor } from 'langium'; +import { annotationCallsOrEmpty, parametersOrEmpty, resultsOrEmpty } from '../../../helpers/nodeProperties.js'; + +export const CODE_ANNOTATION_CALL_TARGET_PARAMETER = 'annotation-call/target-parameter'; +export const CODE_ANNOTATION_CALL_TARGET_RESULT = 'annotation-call/target-result'; + +export const callableTypeParametersMustNotBeAnnotated = (node: SdsCallableType, accept: ValidationAcceptor) => { + for (const parameter of parametersOrEmpty(node)) { + for (const annotationCall of annotationCallsOrEmpty(parameter)) { + accept('error', 'Parameters of callable types must not be annotated.', { + node: annotationCall, + code: CODE_ANNOTATION_CALL_TARGET_PARAMETER, + }); + } + } +}; + +export const callableTypeResultsMustNotBeAnnotated = (node: SdsCallableType, accept: ValidationAcceptor) => { + for (const result of resultsOrEmpty(node.resultList)) { + for (const annotationCall of annotationCallsOrEmpty(result)) { + accept('error', 'Results of callable types must not be annotated.', { + node: annotationCall, + code: CODE_ANNOTATION_CALL_TARGET_RESULT, + }); + } + } +}; + +export const lambdaParametersMustNotBeAnnotated = (node: SdsLambda, accept: ValidationAcceptor) => { + for (const parameter of parametersOrEmpty(node)) { + for (const annotationCall of annotationCallsOrEmpty(parameter)) { + accept('error', 'Lambda parameters must not be annotated.', { + node: annotationCall, + code: CODE_ANNOTATION_CALL_TARGET_PARAMETER, + }); + } + } +}; + +export const parameterCanBeAnnotated = (node: SdsParameter) => { + const containingCallable = getContainerOfType(node, isSdsCallable); + return !isSdsCallableType(containingCallable) && !isSdsLambda(containingCallable); +}; diff --git a/src/language/validation/safe-ds-validator.ts b/src/language/validation/safe-ds-validator.ts index 61d5429d7..444240c2b 100644 --- a/src/language/validation/safe-ds-validator.ts +++ b/src/language/validation/safe-ds-validator.ts @@ -52,6 +52,7 @@ import { assigneeAssignedResultShouldNotBeDeprecated, namedTypeDeclarationShouldNotBeDeprecated, referenceTargetShouldNotBeDeprecated, + requiredParameterMustNotBeDeprecated, } from './builtins/deprecated.js'; import { annotationCallAnnotationShouldNotBeExperimental, @@ -64,6 +65,12 @@ import { placeholderShouldBeUsed } from './other/declarations/placeholders.js'; import { segmentParameterShouldBeUsed, segmentResultMustBeAssignedExactlyOnce } from './other/declarations/segments.js'; import { lambdaParameterMustNotHaveConstModifier } from './other/expressions/lambdas.js'; import { indexedAccessesShouldBeUsedWithCaution } from './experimentalLanguageFeature.js'; +import { requiredParameterMustNotBeExpert } from './builtins/expert.js'; +import { + callableTypeParametersMustNotBeAnnotated, + callableTypeResultsMustNotBeAnnotated, + lambdaParametersMustNotBeAnnotated, +} from './other/declarations/annotationCalls.js'; /** * Register custom validation checks. @@ -97,7 +104,9 @@ export const registerValidationChecks = function (services: SafeDsServices) { SdsCallableType: [ callableTypeMustContainUniqueNames, callableTypeMustNotHaveOptionalParameters, + callableTypeParametersMustNotBeAnnotated, callableTypeParameterMustNotHaveConstModifier, + callableTypeResultsMustNotBeAnnotated, ], SdsClass: [classMustContainUniqueNames], SdsClassBody: [classBodyShouldNotBeEmpty], @@ -109,7 +118,7 @@ export const registerValidationChecks = function (services: SafeDsServices) { SdsExpressionLambda: [expressionLambdaMustContainUniqueNames], SdsFunction: [functionMustContainUniqueNames, functionResultListShouldNotBeEmpty], SdsIndexedAccess: [indexedAccessesShouldBeUsedWithCaution], - SdsLambda: [lambdaParameterMustNotHaveConstModifier], + SdsLambda: [lambdaParametersMustNotBeAnnotated, lambdaParameterMustNotHaveConstModifier], SdsMemberAccess: [memberAccessNullSafetyShouldBeNeeded(services)], SdsModule: [moduleDeclarationsMustMatchFileKind, moduleWithDeclarationsMustStatePackage], SdsNamedType: [ @@ -117,7 +126,11 @@ export const registerValidationChecks = function (services: SafeDsServices) { namedTypeDeclarationShouldNotBeExperimental(services), namedTypeTypeArgumentListShouldBeNeeded, ], - SdsParameter: [parameterMustHaveTypeHint], + SdsParameter: [ + parameterMustHaveTypeHint, + requiredParameterMustNotBeDeprecated(services), + requiredParameterMustNotBeExpert(services), + ], SdsParameterList: [parameterListMustNotHaveRequiredParametersAfterOptionalParameters], SdsPipeline: [pipelineMustContainUniqueNames], SdsPlaceholder: [placeholderShouldBeUsed(services)], diff --git a/src/resources/builtins/safeds/lang/coreAnnotations.sdsstub b/src/resources/builtins/safeds/lang/coreAnnotations.sdsstub index 330c7cc53..2935009a4 100644 --- a/src/resources/builtins/safeds/lang/coreAnnotations.sdsstub +++ b/src/resources/builtins/safeds/lang/coreAnnotations.sdsstub @@ -90,6 +90,11 @@ annotation Deprecated( ]) annotation Experimental +@Experimental +@Description("This parameter should only be used by expert users.") +@Target([AnnotationTarget.Parameter]) +annotation Expert + @Experimental @Description("The function has no side effects and returns the same results for the same arguments.") @Target([AnnotationTarget.Function]) diff --git a/src/resources/builtins/safeds/lang/documentation.sdsstub b/src/resources/builtins/safeds/lang/documentation.sdsstub index 0d714f493..c94d885cc 100644 --- a/src/resources/builtins/safeds/lang/documentation.sdsstub +++ b/src/resources/builtins/safeds/lang/documentation.sdsstub @@ -11,8 +11,3 @@ annotation Since( @Description("The version in which a declaration was added.") version: String ) - -@Experimental -@Description("This parameter should only be used by expert users.") -@Target([AnnotationTarget.Parameter]) -annotation Expert diff --git a/tests/resources/validation/builtins/annotations/deprecated/must not be used on required parameters/main.sdstest b/tests/resources/validation/builtins/annotations/deprecated/must not be used on required parameters/main.sdstest new file mode 100644 index 000000000..201f9c0c6 --- /dev/null +++ b/tests/resources/validation/builtins/annotations/deprecated/must not be used on required parameters/main.sdstest @@ -0,0 +1,48 @@ +package tests.validation.builtins.deprecated.mustNotBeUsedOnRequiredParameters + +// $TEST$ error "A deprecated parameter must be optional." +// $TEST$ no error "A deprecated parameter must be optional." +annotation MyAnnotation(@Deprecated »a«: Int, @Deprecated »b«: Int = 3) + +// $TEST$ error "A deprecated parameter must be optional." +// $TEST$ no error "A deprecated parameter must be optional." +class MyClass(@Deprecated »a«: Int, @Deprecated »b«: Int = 3) { + + // $TEST$ error "A deprecated parameter must be optional." + // $TEST$ no error "A deprecated parameter must be optional." + class MyClass(@Deprecated »a«: Int, @Deprecated »b«: Int = 3) + + // $TEST$ error "A deprecated parameter must be optional." + // $TEST$ no error "A deprecated parameter must be optional." + fun myFunction(@Deprecated »a«: Int, @Deprecated »b«: Int = 3) +} + +enum MyEnum { + + // $TEST$ error "A deprecated parameter must be optional." + // $TEST$ no error "A deprecated parameter must be optional." + MyEnumVariant(@Deprecated »a«: Int, @Deprecated »b«: Int = 3) +} + +// $TEST$ error "A deprecated parameter must be optional." +// $TEST$ no error "A deprecated parameter must be optional." +fun myFunction(@Deprecated »a«: Int, @Deprecated »b«: Int = 3) + +// $TEST$ error "A deprecated parameter must be optional." +// $TEST$ no error "A deprecated parameter must be optional." +segment mySegment1(@Deprecated »a«: Int, @Deprecated »b«: Int = 3) {} + +// $TEST$ no error "A deprecated parameter must be optional." +// $TEST$ no error "A deprecated parameter must be optional." +segment mySegment2( + f: (@Deprecated »a«: Int, @Deprecated »b«: Int = 3) -> () +) { + + // $TEST$ no error "A deprecated parameter must be optional." + // $TEST$ no error "A deprecated parameter must be optional." + val g = (@Deprecated »a«: Int, @Deprecated »b«: Int = 3) {}; + + // $TEST$ no error "A deprecated parameter must be optional." + // $TEST$ no error "A deprecated parameter must be optional." + val h = (@Deprecated »a«: Int, @Deprecated »b«: Int = 3) -> 1; +} diff --git a/tests/resources/validation/builtins/annotations/expert/must not be used on required parameters/main.sdstest b/tests/resources/validation/builtins/annotations/expert/must not be used on required parameters/main.sdstest new file mode 100644 index 000000000..9fe2d13d6 --- /dev/null +++ b/tests/resources/validation/builtins/annotations/expert/must not be used on required parameters/main.sdstest @@ -0,0 +1,48 @@ +package tests.validation.builtins.expert.mustNotBeUsedOnRequiredParameters + +// $TEST$ error "An expert parameter must be optional." +// $TEST$ no error "An expert parameter must be optional." +annotation MyAnnotation(@Expert »a«: Int, @Expert »b«: Int = 3) + +// $TEST$ error "An expert parameter must be optional." +// $TEST$ no error "An expert parameter must be optional." +class MyClass(@Expert »a«: Int, @Expert »b«: Int = 3) { + + // $TEST$ error "An expert parameter must be optional." + // $TEST$ no error "An expert parameter must be optional." + class MyClass(@Expert »a«: Int, @Expert »b«: Int = 3) + + // $TEST$ error "An expert parameter must be optional." + // $TEST$ no error "An expert parameter must be optional." + fun myFunction(@Expert »a«: Int, @Expert »b«: Int = 3) +} + +enum MyEnum { + + // $TEST$ error "An expert parameter must be optional." + // $TEST$ no error "An expert parameter must be optional." + MyEnumVariant(@Expert »a«: Int, @Expert »b«: Int = 3) +} + +// $TEST$ error "An expert parameter must be optional." +// $TEST$ no error "An expert parameter must be optional." +fun myFunction(@Expert »a«: Int, @Expert »b«: Int = 3) + +// $TEST$ error "An expert parameter must be optional." +// $TEST$ no error "An expert parameter must be optional." +segment mySegment1(@Expert »a«: Int, @Expert »b«: Int = 3) {} + +// $TEST$ no error "An expert parameter must be optional." +// $TEST$ no error "An expert parameter must be optional." +segment mySegment2( + f: (@Expert »a«: Int, @Expert »b«: Int = 3) -> () +) { + + // $TEST$ no error "An expert parameter must be optional." + // $TEST$ no error "An expert parameter must be optional." + val g = (@Expert »a«: Int, @Expert »b«: Int = 3) {}; + + // $TEST$ no error "An expert parameter must be optional." + // $TEST$ no error "An expert parameter must be optional." + val h = (@Expert »a«: Int, @Expert »b«: Int = 3) -> 1; +} diff --git a/tests/resources/validation/other/declarations/annotation calls/must not be used on lambda parameters/main.sdstest b/tests/resources/validation/other/declarations/annotation calls/must not be used on lambda parameters/main.sdstest new file mode 100644 index 000000000..a589ae4bf --- /dev/null +++ b/tests/resources/validation/other/declarations/annotation calls/must not be used on lambda parameters/main.sdstest @@ -0,0 +1,16 @@ +package tests.validation.other.declarations.annotationCalls.mustNotBeUsedOnLambdaParameters + +annotation MyAnnotation + +pipeline myPipeline { + + // $TEST$ error "Lambda parameters must not be annotated." + // $TEST$ error "Lambda parameters must not be annotated." + // $TEST$ error "Lambda parameters must not be annotated." + val f = (»@MyAnnotation« »@MyAnnotation« a: Int, »@MyAnnotation« b: Int = 3) {}; + + // $TEST$ error "Lambda parameters must not be annotated." + // $TEST$ error "Lambda parameters must not be annotated." + // $TEST$ error "Lambda parameters must not be annotated." + val g = (»@MyAnnotation« »@MyAnnotation« a: Int, »@MyAnnotation« b: Int = 3) -> 1; +} diff --git a/tests/resources/validation/other/declarations/annotation calls/must not be used on parameters of callable types/main.sdstest b/tests/resources/validation/other/declarations/annotation calls/must not be used on parameters of callable types/main.sdstest new file mode 100644 index 000000000..03d930b8e --- /dev/null +++ b/tests/resources/validation/other/declarations/annotation calls/must not be used on parameters of callable types/main.sdstest @@ -0,0 +1,10 @@ +package tests.validation.other.declarations.annotationCalls.mustNotBeUsedOnParametersOfCallableTypes + +annotation MyAnnotation + +// $TEST$ error "Parameters of callable types must not be annotated." +// $TEST$ error "Parameters of callable types must not be annotated." +// $TEST$ error "Parameters of callable types must not be annotated." +segment mySegment( + f: (»@MyAnnotation« »@MyAnnotation« a: Int, »@MyAnnotation« b: Int = 3) -> () +) {} diff --git a/tests/resources/validation/other/declarations/annotation calls/must not be used on results of callable types/main.sdstest b/tests/resources/validation/other/declarations/annotation calls/must not be used on results of callable types/main.sdstest new file mode 100644 index 000000000..33657cbf6 --- /dev/null +++ b/tests/resources/validation/other/declarations/annotation calls/must not be used on results of callable types/main.sdstest @@ -0,0 +1,10 @@ +package tests.validation.other.declarations.annotationCalls.mustNotBeUsedOnResultsOfCallableTypes + +annotation MyAnnotation + +// $TEST$ error "Results of callable types must not be annotated." +// $TEST$ error "Results of callable types must not be annotated." +// $TEST$ error "Results of callable types must not be annotated." +segment mySegment( + f: () -> (»@MyAnnotation« »@MyAnnotation« a: Int, »@MyAnnotation« b: Int) +) {}