From bf20c7c554a4c6de337a675364c86e8078544b8d Mon Sep 17 00:00:00 2001 From: Lars Reimann Date: Sun, 8 Oct 2023 17:13:34 +0200 Subject: [PATCH] feat: validation for results of segments (#613) Closes partially #543 ### Summary of Changes Ensure that a segment yields exactly one value for each result. --------- Co-authored-by: megalinter-bot <129584137+megalinter-bot@users.noreply.github.com> --- .../other/declarations/parameters.ts | 20 +------- .../validation/other/declarations/segments.ts | 48 +++++++++++++++++++ src/language/validation/safe-ds-validator.ts | 7 ++- .../in same file/to modules/main.sdstest | 6 +++ .../main.sdstest | 0 .../unused/main.sdstest | 2 +- .../segments/duplicate yield/main.sdstest | 20 ++++++++ .../segments/unassigned result/main.sdstest | 17 +++++++ .../unused parameter}/main.sdstest | 2 +- 9 files changed, 97 insertions(+), 25 deletions(-) create mode 100644 src/language/validation/other/declarations/segments.ts create mode 100644 tests/resources/scoping/references/in same file/to modules/main.sdstest rename tests/resources/validation/other/declarations/{parameter => parameters}/variadic must not be optional/main.sdstest (100%) rename tests/resources/validation/other/declarations/{placeholder => placeholders}/unused/main.sdstest (95%) create mode 100644 tests/resources/validation/other/declarations/segments/duplicate yield/main.sdstest create mode 100644 tests/resources/validation/other/declarations/segments/unassigned result/main.sdstest rename tests/resources/validation/other/declarations/{parameter/unused => segments/unused parameter}/main.sdstest (95%) diff --git a/src/language/validation/other/declarations/parameters.ts b/src/language/validation/other/declarations/parameters.ts index 9a2b3763e..473db1763 100644 --- a/src/language/validation/other/declarations/parameters.ts +++ b/src/language/validation/other/declarations/parameters.ts @@ -1,9 +1,6 @@ -import { SdsParameter, SdsSegment } from '../../../generated/ast.js'; +import { SdsParameter } from '../../../generated/ast.js'; import { ValidationAcceptor } from 'langium'; -import { parametersOrEmpty } from '../../../helpers/nodeProperties.js'; -import { SafeDsServices } from '../../../safe-ds-module.js'; -export const CODE_PARAMETER_UNUSED = 'parameter/unused'; export const CODE_PARAMETER_VARIADIC_AND_OPTIONAL = 'parameter/variadic-and-optional'; export const parameterMustNotBeVariadicAndOptional = (node: SdsParameter, accept: ValidationAcceptor) => { @@ -15,18 +12,3 @@ export const parameterMustNotBeVariadicAndOptional = (node: SdsParameter, accept }); } }; - -export const segmentParameterShouldBeUsed = - (services: SafeDsServices) => (node: SdsSegment, accept: ValidationAcceptor) => { - for (const parameter of parametersOrEmpty(node)) { - const usages = services.helpers.NodeMapper.parameterToReferences(parameter); - - if (usages.isEmpty()) { - accept('warning', 'This parameter is unused and can be removed.', { - node: parameter, - property: 'name', - code: CODE_PARAMETER_UNUSED, - }); - } - } - }; diff --git a/src/language/validation/other/declarations/segments.ts b/src/language/validation/other/declarations/segments.ts new file mode 100644 index 000000000..8c0bf1ffc --- /dev/null +++ b/src/language/validation/other/declarations/segments.ts @@ -0,0 +1,48 @@ +import { SdsSegment } from '../../../generated/ast.js'; +import { ValidationAcceptor } from 'langium'; +import { parametersOrEmpty, resultsOrEmpty } from '../../../helpers/nodeProperties.js'; +import { SafeDsServices } from '../../../safe-ds-module.js'; + +export const CODE_SEGMENT_DUPLICATE_YIELD = 'segment/duplicate-yield'; +export const CODE_SEGMENT_UNASSIGNED_RESULT = 'segment/unassigned-result'; +export const CODE_SEGMENT_UNUSED_PARAMETER = 'segment/unused-parameter'; + +export const segmentResultMustBeAssignedExactlyOnce = + (services: SafeDsServices) => (node: SdsSegment, accept: ValidationAcceptor) => { + const results = resultsOrEmpty(node.resultList); + for (const result of results) { + const yields = services.helpers.NodeMapper.resultToYields(result); + if (yields.isEmpty()) { + accept('error', 'Nothing is assigned to this result.', { + node: result, + property: 'name', + code: CODE_SEGMENT_UNASSIGNED_RESULT, + }); + continue; + } + + const duplicateYields = yields.tail(1); + for (const duplicate of duplicateYields) { + accept('error', `The result '${result.name}' has been assigned already.`, { + node: duplicate, + property: 'result', + code: CODE_SEGMENT_DUPLICATE_YIELD, + }); + } + } + }; + +export const segmentParameterShouldBeUsed = + (services: SafeDsServices) => (node: SdsSegment, accept: ValidationAcceptor) => { + for (const parameter of parametersOrEmpty(node)) { + const usages = services.helpers.NodeMapper.parameterToReferences(parameter); + + if (usages.isEmpty()) { + accept('warning', 'This parameter is unused and can be removed.', { + node: parameter, + property: 'name', + code: CODE_SEGMENT_UNUSED_PARAMETER, + }); + } + } + }; diff --git a/src/language/validation/safe-ds-validator.ts b/src/language/validation/safe-ds-validator.ts index 4ed3dc78d..221dd1660 100644 --- a/src/language/validation/safe-ds-validator.ts +++ b/src/language/validation/safe-ds-validator.ts @@ -45,10 +45,7 @@ import { unionTypeMustHaveTypeArguments } from './other/types/unionTypes.js'; import { callableTypeMustNotHaveOptionalParameters } from './other/types/callableTypes.js'; import { typeArgumentListMustNotHavePositionalArgumentsAfterNamedArguments } from './other/types/typeArgumentLists.js'; import { argumentListMustNotHavePositionalArgumentsAfterNamedArguments } from './other/argumentLists.js'; -import { - parameterMustNotBeVariadicAndOptional, - segmentParameterShouldBeUsed, -} from './other/declarations/parameters.js'; +import { parameterMustNotBeVariadicAndOptional } from './other/declarations/parameters.js'; import { referenceTargetMustNotBeAnnotationPipelineOrSchema } from './other/expressions/references.js'; import { annotationCallAnnotationShouldNotBeDeprecated, @@ -65,6 +62,7 @@ import { referenceTargetShouldNotExperimental, } from './builtins/experimental.js'; import { placeholderShouldBeUsed } from './other/declarations/placeholders.js'; +import { segmentParameterShouldBeUsed, segmentResultMustBeAssignedExactlyOnce } from './other/declarations/segments.js'; /** * Register custom validation checks. @@ -125,6 +123,7 @@ export const registerValidationChecks = function (services: SafeDsServices) { SdsSegment: [ segmentMustContainUniqueNames, segmentParameterShouldBeUsed(services), + segmentResultMustBeAssignedExactlyOnce(services), segmentResultListShouldNotBeEmpty, ], SdsTemplateString: [templateStringMustHaveExpressionBetweenTwoStringParts], diff --git a/tests/resources/scoping/references/in same file/to modules/main.sdstest b/tests/resources/scoping/references/in same file/to modules/main.sdstest new file mode 100644 index 000000000..2904f0e21 --- /dev/null +++ b/tests/resources/scoping/references/in same file/to modules/main.sdstest @@ -0,0 +1,6 @@ +package tests + +pipeline myPipeline { + // $TEST$ unresolved + »tests«; +} diff --git a/tests/resources/validation/other/declarations/parameter/variadic must not be optional/main.sdstest b/tests/resources/validation/other/declarations/parameters/variadic must not be optional/main.sdstest similarity index 100% rename from tests/resources/validation/other/declarations/parameter/variadic must not be optional/main.sdstest rename to tests/resources/validation/other/declarations/parameters/variadic must not be optional/main.sdstest diff --git a/tests/resources/validation/other/declarations/placeholder/unused/main.sdstest b/tests/resources/validation/other/declarations/placeholders/unused/main.sdstest similarity index 95% rename from tests/resources/validation/other/declarations/placeholder/unused/main.sdstest rename to tests/resources/validation/other/declarations/placeholders/unused/main.sdstest index 49dde3b9d..1527cb7ac 100644 --- a/tests/resources/validation/other/declarations/placeholder/unused/main.sdstest +++ b/tests/resources/validation/other/declarations/placeholders/unused/main.sdstest @@ -1,4 +1,4 @@ -package tests.validation.declarations.placeholders.unused +package tests.validation.other.declarations.placeholders.unused fun f() -> (r1: Int, r2: Int) diff --git a/tests/resources/validation/other/declarations/segments/duplicate yield/main.sdstest b/tests/resources/validation/other/declarations/segments/duplicate yield/main.sdstest new file mode 100644 index 000000000..f6677b703 --- /dev/null +++ b/tests/resources/validation/other/declarations/segments/duplicate yield/main.sdstest @@ -0,0 +1,20 @@ +package tests.validation.other.declarations.segments.duplicateYield + +segment mySegment() -> (a: Int, b: Int, c: Int) { + // $TEST$ no error r"The result '\w*' has been assigned already\." + yield »a« = 1; + + // $TEST$ no error r"The result '\w*' has been assigned already\." + yield »b« = 1; + // $TEST$ error "The result 'b' has been assigned already." + yield »b« = 1; + + // $TEST$ no error r"The result '\w*' has been assigned already\." + // $TEST$ error "The result 'c' has been assigned already." + yield »c«, yield »c« = 1; + + // $TEST$ no error r"The result '\w*' has been assigned already\." + yield »unresolved« = 1; + // $TEST$ no error r"The result '\w*' has been assigned already\." + yield »unresolved« = 1; +} diff --git a/tests/resources/validation/other/declarations/segments/unassigned result/main.sdstest b/tests/resources/validation/other/declarations/segments/unassigned result/main.sdstest new file mode 100644 index 000000000..f4ef66806 --- /dev/null +++ b/tests/resources/validation/other/declarations/segments/unassigned result/main.sdstest @@ -0,0 +1,17 @@ +package tests.validation.other.declarations.segments.unassignedResult + +// $TEST$ no error "Nothing is assigned to this result." +// $TEST$ no error "Nothing is assigned to this result." +// $TEST$ no error "Nothing is assigned to this result." +// $TEST$ no error "Nothing is assigned to this result." +// $TEST$ error "Nothing is assigned to this result." +segment mySegment() -> (»a«: Int, »b«: Int, »c«: Int, »d«: Int, »e«: Int) { + yield b = 1; + yield a = 1; + + yield c = 1; + yield c = 1; + + // While nothing is assigned to d, the programmer still states their intention to do so. We already show another error for this. + _, yield d = 1; +} diff --git a/tests/resources/validation/other/declarations/parameter/unused/main.sdstest b/tests/resources/validation/other/declarations/segments/unused parameter/main.sdstest similarity index 95% rename from tests/resources/validation/other/declarations/parameter/unused/main.sdstest rename to tests/resources/validation/other/declarations/segments/unused parameter/main.sdstest index 205dca661..6dc16013e 100644 --- a/tests/resources/validation/other/declarations/parameter/unused/main.sdstest +++ b/tests/resources/validation/other/declarations/segments/unused parameter/main.sdstest @@ -1,4 +1,4 @@ -package tests.validation.declarations.parameters.unused +package tests.validation.other.declarations.segments.unusedParameters segment mySegment( // $TEST$ warning "This parameter is unused and can be removed."