From 616ff25d19b2d2393c007712a3edd89ba4ee8763 Mon Sep 17 00:00:00 2001 From: Lars Reimann Date: Tue, 14 Nov 2023 14:12:01 +0100 Subject: [PATCH 1/2] refactor: extract validity checks for annotation targets and impurity reasons --- .../language/builtins/safe-ds-annotations.ts | 27 +++++-------------- .../src/language/builtins/safe-ds-enums.ts | 9 ++++++- .../language/validation/builtins/impure.ts | 13 ++++----- .../language/validation/builtins/target.ts | 10 +++---- 4 files changed, 22 insertions(+), 37 deletions(-) diff --git a/packages/safe-ds-lang/src/language/builtins/safe-ds-annotations.ts b/packages/safe-ds-lang/src/language/builtins/safe-ds-annotations.ts index 6bc20b10f..255c1c3a6 100644 --- a/packages/safe-ds-lang/src/language/builtins/safe-ds-annotations.ts +++ b/packages/safe-ds-lang/src/language/builtins/safe-ds-annotations.ts @@ -1,8 +1,7 @@ -import { EMPTY_STREAM, getContainerOfType, Stream, stream, URI } from 'langium'; +import { EMPTY_STREAM, Stream, stream, URI } from 'langium'; import { resourceNameToUri } from '../../helpers/resources.js'; import { isSdsAnnotation, - isSdsEnum, SdsAnnotatedObject, SdsAnnotation, SdsEnumVariant, @@ -12,13 +11,7 @@ import { } from '../generated/ast.js'; import { findFirstAnnotationCallOf, getEnumVariants, hasAnnotationCallOf } from '../helpers/nodeProperties.js'; import { SafeDsNodeMapper } from '../helpers/safe-ds-node-mapper.js'; -import { - EvaluatedEnumVariant, - EvaluatedList, - EvaluatedNode, - StringConstant, - UnknownEvaluatedNode, -} from '../partialEvaluation/model.js'; +import { EvaluatedList, EvaluatedNode, StringConstant, UnknownEvaluatedNode } from '../partialEvaluation/model.js'; import { SafeDsPartialEvaluator } from '../partialEvaluation/safe-ds-partial-evaluator.js'; import { SafeDsServices } from '../safe-ds-module.js'; import { SafeDsEnums } from './safe-ds-enums.js'; @@ -80,12 +73,8 @@ export class SafeDsAnnotations extends SafeDsModuleMembers { // Otherwise, filter the elements of the list and keep only variants of the ImpurityReason enum return stream(value.elements) - .filter( - (it) => - it instanceof EvaluatedEnumVariant && - getContainerOfType(it.variant, isSdsEnum) === this.builtinEnums.ImpurityReason, - ) - .map((it) => (it).variant); + .filter(this.builtinEnums.isEvaluatedImpurityReason) + .map((it) => it.variant); } get Impure(): SdsAnnotation | undefined { @@ -161,12 +150,8 @@ export class SafeDsAnnotations extends SafeDsModuleMembers { // Otherwise, filter the elements of the list and keep only variants of the AnnotationTarget enum return stream(value.elements) - .filter( - (it) => - it instanceof EvaluatedEnumVariant && - getContainerOfType(it.variant, isSdsEnum) === this.builtinEnums.AnnotationTarget, - ) - .map((it) => (it).variant); + .filter(this.builtinEnums.isEvaluatedAnnotationTarget) + .map((it) => it.variant); } get Target(): SdsAnnotation | undefined { diff --git a/packages/safe-ds-lang/src/language/builtins/safe-ds-enums.ts b/packages/safe-ds-lang/src/language/builtins/safe-ds-enums.ts index 7872039ba..ed98860da 100644 --- a/packages/safe-ds-lang/src/language/builtins/safe-ds-enums.ts +++ b/packages/safe-ds-lang/src/language/builtins/safe-ds-enums.ts @@ -1,6 +1,7 @@ -import { URI } from 'langium'; +import { getContainerOfType, URI } from 'langium'; import { resourceNameToUri } from '../../helpers/resources.js'; import { isSdsEnum, SdsEnum } from '../generated/ast.js'; +import { EvaluatedEnumVariant, EvaluatedNode } from '../partialEvaluation/model.js'; import { SafeDsModuleMembers } from './safe-ds-module-members.js'; const ANNOTATION_USAGE_URI = resourceNameToUri('builtins/safeds/lang/annotationUsage.sdsstub'); @@ -11,10 +12,16 @@ export class SafeDsEnums extends SafeDsModuleMembers { return this.getEnum(ANNOTATION_USAGE_URI, 'AnnotationTarget'); } + isEvaluatedAnnotationTarget = (node: EvaluatedNode): node is EvaluatedEnumVariant => + node instanceof EvaluatedEnumVariant && getContainerOfType(node.variant, isSdsEnum) === this.AnnotationTarget; + get ImpurityReason(): SdsEnum | undefined { return this.getEnum(PURITY_URI, 'ImpurityReason'); } + isEvaluatedImpurityReason = (node: EvaluatedNode): node is EvaluatedEnumVariant => + node instanceof EvaluatedEnumVariant && getContainerOfType(node.variant, isSdsEnum) === this.ImpurityReason; + private getEnum(uri: URI, name: string): SdsEnum | undefined { return this.getModuleMember(uri, name, isSdsEnum); } diff --git a/packages/safe-ds-lang/src/language/validation/builtins/impure.ts b/packages/safe-ds-lang/src/language/validation/builtins/impure.ts index 34f948171..571b4e051 100644 --- a/packages/safe-ds-lang/src/language/validation/builtins/impure.ts +++ b/packages/safe-ds-lang/src/language/validation/builtins/impure.ts @@ -1,8 +1,8 @@ -import { getContainerOfType, stream, ValidationAcceptor } from 'langium'; -import { isSdsCall, isSdsEnum, isSdsList, SdsFunction } from '../../generated/ast.js'; -import type { SafeDsServices } from '../../safe-ds-module.js'; +import { stream, ValidationAcceptor } from 'langium'; +import { isSdsCall, isSdsList, SdsFunction } from '../../generated/ast.js'; import { findFirstAnnotationCallOf, getArguments, getParameters } from '../../helpers/nodeProperties.js'; -import { EvaluatedEnumVariant, StringConstant } from '../../partialEvaluation/model.js'; +import { StringConstant } from '../../partialEvaluation/model.js'; +import type { SafeDsServices } from '../../safe-ds-module.js'; export const CODE_IMPURE_PARAMETER_NAME = 'impure/parameter-name'; @@ -38,10 +38,7 @@ export const impurityReasonParameterNameMustBelongToParameter = (services: SafeD // Check whether the reason is valid const evaluatedReason = partialEvaluator.evaluate(reason); - if ( - !(evaluatedReason instanceof EvaluatedEnumVariant) || - getContainerOfType(evaluatedReason.variant, isSdsEnum) !== builtinEnums.ImpurityReason - ) { + if (!builtinEnums.isEvaluatedImpurityReason(evaluatedReason)) { continue; } diff --git a/packages/safe-ds-lang/src/language/validation/builtins/target.ts b/packages/safe-ds-lang/src/language/validation/builtins/target.ts index f7a1365fa..d38f914a9 100644 --- a/packages/safe-ds-lang/src/language/validation/builtins/target.ts +++ b/packages/safe-ds-lang/src/language/validation/builtins/target.ts @@ -1,4 +1,4 @@ -import { getContainerOfType, ValidationAcceptor } from 'langium'; +import { ValidationAcceptor } from 'langium'; import { isSdsAnnotation, isSdsAttribute, @@ -17,9 +17,8 @@ import { SdsAnnotationCall, SdsEnumVariant, } from '../../generated/ast.js'; -import { SafeDsServices } from '../../safe-ds-module.js'; import { findFirstAnnotationCallOf, getAnnotationCallTarget } from '../../helpers/nodeProperties.js'; -import { EvaluatedEnumVariant } from '../../partialEvaluation/model.js'; +import { SafeDsServices } from '../../safe-ds-module.js'; export const CODE_TARGET_DUPLICATE_TARGET = 'target/duplicate-target'; export const CODE_TARGET_WRONG_TARGET = 'target/wrong-target'; @@ -44,10 +43,7 @@ export const targetShouldNotHaveDuplicateEntries = (services: SafeDsServices) => const knownTargets = new Set(); for (const target of targets.elements) { const evaluatedTarget = partialEvaluator.evaluate(target); - if ( - !(evaluatedTarget instanceof EvaluatedEnumVariant) || - getContainerOfType(evaluatedTarget.variant, isSdsEnum) !== builtinEnums.AnnotationTarget - ) { + if (!builtinEnums.isEvaluatedAnnotationTarget(evaluatedTarget)) { continue; } From 8ce979634beb4aa7842b44d60c0c8e7df2110dac Mon Sep 17 00:00:00 2001 From: Lars Reimann Date: Tue, 14 Nov 2023 14:36:20 +0100 Subject: [PATCH 2/2] feat: warn about duplicate impurity reasons --- .../language/builtins/safe-ds-annotations.ts | 14 +++-- .../language/validation/builtins/impure.ts | 43 ++++++++++++- .../language/validation/safe-ds-validator.ts | 6 +- .../impure/duplicate reason/main.sdstest | 62 +++++++++++++++++++ .../no impure annotation.sdstest | 5 ++ 5 files changed, 123 insertions(+), 7 deletions(-) create mode 100644 packages/safe-ds-lang/tests/resources/validation/builtins/annotations/impure/duplicate reason/main.sdstest create mode 100644 packages/safe-ds-lang/tests/resources/validation/builtins/annotations/impure/duplicate reason/no impure annotation.sdstest diff --git a/packages/safe-ds-lang/src/language/builtins/safe-ds-annotations.ts b/packages/safe-ds-lang/src/language/builtins/safe-ds-annotations.ts index 255c1c3a6..406f1c301 100644 --- a/packages/safe-ds-lang/src/language/builtins/safe-ds-annotations.ts +++ b/packages/safe-ds-lang/src/language/builtins/safe-ds-annotations.ts @@ -11,7 +11,13 @@ import { } from '../generated/ast.js'; import { findFirstAnnotationCallOf, getEnumVariants, hasAnnotationCallOf } from '../helpers/nodeProperties.js'; import { SafeDsNodeMapper } from '../helpers/safe-ds-node-mapper.js'; -import { EvaluatedList, EvaluatedNode, StringConstant, UnknownEvaluatedNode } from '../partialEvaluation/model.js'; +import { + EvaluatedEnumVariant, + EvaluatedList, + EvaluatedNode, + StringConstant, + UnknownEvaluatedNode, +} from '../partialEvaluation/model.js'; import { SafeDsPartialEvaluator } from '../partialEvaluation/safe-ds-partial-evaluator.js'; import { SafeDsServices } from '../safe-ds-module.js'; import { SafeDsEnums } from './safe-ds-enums.js'; @@ -64,7 +70,7 @@ export class SafeDsAnnotations extends SafeDsModuleMembers { return hasAnnotationCallOf(node, this.Impure); } - streamImpurityReasons(node: SdsFunction | undefined): Stream { + streamImpurityReasons(node: SdsFunction | undefined): Stream { // If allReasons are specified, but we could not evaluate them to a list, no reasons apply const value = this.getParameterValue(node, this.Impure, 'allReasons'); if (!(value instanceof EvaluatedList)) { @@ -72,9 +78,7 @@ export class SafeDsAnnotations extends SafeDsModuleMembers { } // Otherwise, filter the elements of the list and keep only variants of the ImpurityReason enum - return stream(value.elements) - .filter(this.builtinEnums.isEvaluatedImpurityReason) - .map((it) => it.variant); + return stream(value.elements).filter(this.builtinEnums.isEvaluatedImpurityReason); } get Impure(): SdsAnnotation | undefined { diff --git a/packages/safe-ds-lang/src/language/validation/builtins/impure.ts b/packages/safe-ds-lang/src/language/validation/builtins/impure.ts index 571b4e051..f87e0d0ad 100644 --- a/packages/safe-ds-lang/src/language/validation/builtins/impure.ts +++ b/packages/safe-ds-lang/src/language/validation/builtins/impure.ts @@ -1,9 +1,10 @@ import { stream, ValidationAcceptor } from 'langium'; import { isSdsCall, isSdsList, SdsFunction } from '../../generated/ast.js'; import { findFirstAnnotationCallOf, getArguments, getParameters } from '../../helpers/nodeProperties.js'; -import { StringConstant } from '../../partialEvaluation/model.js'; +import { EvaluatedEnumVariant, StringConstant } from '../../partialEvaluation/model.js'; import type { SafeDsServices } from '../../safe-ds-module.js'; +export const CODE_IMPURE_DUPLICATE_REASON = 'impure/duplicate-reason'; export const CODE_IMPURE_PARAMETER_NAME = 'impure/parameter-name'; export const impurityReasonParameterNameMustBelongToParameter = (services: SafeDsServices) => { @@ -67,3 +68,43 @@ export const impurityReasonParameterNameMustBelongToParameter = (services: SafeD } }; }; + +export const impurityReasonShouldNotBeSetMultipleTimes = (services: SafeDsServices) => { + const builtinAnnotations = services.builtins.Annotations; + const builtinEnums = services.builtins.Enums; + const nodeMapper = services.helpers.NodeMapper; + const partialEvaluator = services.evaluation.PartialEvaluator; + + return (node: SdsFunction, accept: ValidationAcceptor) => { + const annotationCall = findFirstAnnotationCallOf(node, builtinAnnotations.Impure); + + // Don't further validate if the function is marked as impure and as pure + if (!annotationCall || builtinAnnotations.isPure(node)) { + return; + } + + // Check whether allReasons is valid + const allReasons = nodeMapper.callToParameterValue(annotationCall, 'allReasons'); + if (!isSdsList(allReasons)) { + return; + } + + const knownReasons: EvaluatedEnumVariant[] = []; + for (const reason of allReasons.elements) { + // Check whether the reason is valid + const evaluatedReason = partialEvaluator.evaluate(reason); + if (!builtinEnums.isEvaluatedImpurityReason(evaluatedReason)) { + continue; + } + + if (knownReasons.some((it) => it.equals(evaluatedReason))) { + accept('warning', `The impurity reason '${evaluatedReason}' was set already.`, { + node: reason, + code: CODE_IMPURE_DUPLICATE_REASON, + }); + } else { + knownReasons.push(evaluatedReason); + } + } + }; +}; diff --git a/packages/safe-ds-lang/src/language/validation/safe-ds-validator.ts b/packages/safe-ds-lang/src/language/validation/safe-ds-validator.ts index 7ce00ee1b..3d2987d7b 100644 --- a/packages/safe-ds-lang/src/language/validation/safe-ds-validator.ts +++ b/packages/safe-ds-lang/src/language/validation/safe-ds-validator.ts @@ -17,6 +17,10 @@ import { referenceTargetShouldNotExperimental, } from './builtins/experimental.js'; import { requiredParameterMustNotBeExpert } from './builtins/expert.js'; +import { + impurityReasonParameterNameMustBelongToParameter, + impurityReasonShouldNotBeSetMultipleTimes, +} from './builtins/impure.js'; import { pureParameterMustHaveCallableType } from './builtins/pure.js'; import { pythonCallMustOnlyContainValidTemplateExpressions } from './builtins/pythonCall.js'; import { pythonModuleShouldDifferFromSafeDsPackage } from './builtins/pythonModule.js'; @@ -168,7 +172,6 @@ import { resultMustHaveTypeHint, yieldTypeMustMatchResultType, } from './types.js'; -import { impurityReasonParameterNameMustBelongToParameter } from './builtins/impure.js'; /** * Register custom validation checks. @@ -251,6 +254,7 @@ export const registerValidationChecks = function (services: SafeDsServices) { functionResultListShouldNotBeEmpty, functionPurityMustBeSpecified(services), impurityReasonParameterNameMustBelongToParameter(services), + impurityReasonShouldNotBeSetMultipleTimes(services), pythonCallMustOnlyContainValidTemplateExpressions(services), pythonNameMustNotBeSetIfPythonCallIsSet(services), ], diff --git a/packages/safe-ds-lang/tests/resources/validation/builtins/annotations/impure/duplicate reason/main.sdstest b/packages/safe-ds-lang/tests/resources/validation/builtins/annotations/impure/duplicate reason/main.sdstest new file mode 100644 index 000000000..229d981ed --- /dev/null +++ b/packages/safe-ds-lang/tests/resources/validation/builtins/annotations/impure/duplicate reason/main.sdstest @@ -0,0 +1,62 @@ +package tests.validation.builtins.annotations.impure.duplicateReason + +@Impure([ + // $TEST$ no warning "The impurity reason 'FileReadFromConstantPath("text.txt")' was set already." + »ImpurityReason.FileReadFromConstantPath("text.txt")« +]) + +/* + * We already show another error if the `@Impure` annotation is called multiple times. + */ + +@Impure([ + // $TEST$ no warning "The impurity reason 'FileReadFromConstantPath("text.txt")' was set already." + »ImpurityReason.FileReadFromConstantPath("text.txt")«, + // $TEST$ no warning "The impurity reason 'FileReadFromConstantPath("text.txt")' was set already." + »ImpurityReason.FileReadFromConstantPath("text.txt")«, +]) +fun testFunction1() + +@Impure([ + // $TEST$ no warning "The impurity reason 'FileReadFromConstantPath("text.txt")' was set already." + »ImpurityReason.FileReadFromConstantPath("text.txt")«, + // $TEST$ warning "The impurity reason 'FileReadFromConstantPath("text.txt")' was set already." + »ImpurityReason.FileReadFromConstantPath("text.txt")«, +]) +fun testFunction2() + +@Impure([ + // $TEST$ no warning "The impurity reason 'FileWriteToConstantPath("text.txt")' was set already." + »ImpurityReason.FileWriteToConstantPath("text.txt")«, + // $TEST$ warning "The impurity reason 'FileWriteToConstantPath("text.txt")' was set already." + »ImpurityReason.FileWriteToConstantPath("text.txt")«, + // $TEST$ warning "The impurity reason 'FileWriteToConstantPath("text.txt")' was set already." + »ImpurityReason.FileWriteToConstantPath("text.txt")«, + // $TEST$ no warning "The impurity reason 'FileReadFromConstantPath("text.txt")' was set already." + »ImpurityReason.FileReadFromConstantPath("text.txt")«, + // $TEST$ warning "The impurity reason 'FileReadFromConstantPath("text.txt")' was set already." + »ImpurityReason.FileReadFromConstantPath("text.txt")«, + // $TEST$ warning "The impurity reason 'FileReadFromConstantPath("text.txt")' was set already." + »ImpurityReason.FileReadFromConstantPath("text.txt")«, +]) +fun testFunction3() + +/* + * We already show another error if an annotation is called with arguments of invalid type. + */ + +@Impure( + // $TEST$ no warning "The impurity reason 'FileWriteToConstantPath("text.txt")' was set already." + »ImpurityReason.FileWriteToConstantPath("text.txt")«, + // $TEST$ no warning "The impurity reason 'FileWriteToConstantPath("text.txt")' was set already." + »ImpurityReason.FileWriteToConstantPath("text.txt")« +) +fun testFunction4() + +@Impure([ + // $TEST$ no warning r"The impurity reason '.*' was set already\." + »1«, + // $TEST$ no warning r"The impurity reason '.*' was set already\." + »1«, +]) +fun testFunction5() diff --git a/packages/safe-ds-lang/tests/resources/validation/builtins/annotations/impure/duplicate reason/no impure annotation.sdstest b/packages/safe-ds-lang/tests/resources/validation/builtins/annotations/impure/duplicate reason/no impure annotation.sdstest new file mode 100644 index 000000000..6436664cb --- /dev/null +++ b/packages/safe-ds-lang/tests/resources/validation/builtins/annotations/impure/duplicate reason/no impure annotation.sdstest @@ -0,0 +1,5 @@ +package tests.validation.builtins.annotations.impure.duplicateReason + +// $TEST$ no warning r"The impurity reason '.*' was set already\." + +fun testFunction6()