From 3a2e9cca48fd10c6793c8c9ceaf57362e9a650e4 Mon Sep 17 00:00:00 2001 From: Lars Reimann Date: Sun, 8 Oct 2023 15:56:37 +0200 Subject: [PATCH] feat: warn if parameters or placeholders are unused (#612) Closes partially #543 ### Summary of Changes Show a warning if a parameter or placeholder is unused and suggest removing it. --------- Co-authored-by: megalinter-bot <129584137+megalinter-bot@users.noreply.github.com> --- src/language/helpers/safe-ds-node-mapper.ts | 29 +++++------ .../other/declarations/parameters.ts | 20 +++++++- .../other/declarations/placeholders.ts | 29 +++++++++++ src/language/validation/safe-ds-validator.ts | 13 ++++- .../parameterToReferences.test.ts | 31 ++++++++++-- .../placeholdersToReferences.test.ts | 34 +++++++++---- .../resultToYields.test.ts | 8 +-- .../parameter/unused/main.sdstest | 49 +++++++++++++++++++ .../placeholder/unused/main.sdstest | 44 +++++++++++++++++ 9 files changed, 220 insertions(+), 37 deletions(-) create mode 100644 src/language/validation/other/declarations/placeholders.ts create mode 100644 tests/resources/validation/other/declarations/parameter/unused/main.sdstest create mode 100644 tests/resources/validation/other/declarations/placeholder/unused/main.sdstest diff --git a/src/language/helpers/safe-ds-node-mapper.ts b/src/language/helpers/safe-ds-node-mapper.ts index f3dcbc21b..0b3191532 100644 --- a/src/language/helpers/safe-ds-node-mapper.ts +++ b/src/language/helpers/safe-ds-node-mapper.ts @@ -27,7 +27,7 @@ import { SdsYield, } from '../generated/ast.js'; import { CallableType, StaticType } from '../typing/model.js'; -import { findLocalReferences, getContainerOfType } from 'langium'; +import { findLocalReferences, getContainerOfType, Stream, stream } from 'langium'; import { abstractResultsOrEmpty, argumentsOrEmpty, @@ -148,62 +148,59 @@ export class SafeDsNodeMapper { /** * Returns all references that target the given parameter. */ - parameterToReferences(node: SdsParameter | undefined): SdsReference[] { + parameterToReferences(node: SdsParameter | undefined): Stream { if (!node) { - return []; + return stream(); } const containingCallable = getContainerOfType(node, isSdsCallable); /* c8 ignore start */ if (!containingCallable) { - return []; + return stream(); } /* c8 ignore stop */ return findLocalReferences(node, containingCallable) .map((it) => it.$refNode?.astNode) - .filter(isSdsReference) - .toArray(); + .filter(isSdsReference); } /** * Returns all references that target the given placeholder. */ - placeholderToReferences(node: SdsPlaceholder | undefined): SdsReference[] { + placeholderToReferences(node: SdsPlaceholder | undefined): Stream { if (!node) { - return []; + return stream(); } const containingBlock = getContainerOfType(node, isSdsBlock); /* c8 ignore start */ if (!containingBlock) { - return []; + return stream(); } /* c8 ignore stop */ return findLocalReferences(node, containingBlock) .map((it) => it.$refNode?.astNode) - .filter(isSdsReference) - .toArray(); + .filter(isSdsReference); } /** * Returns all yields that assign to the given result. */ - resultToYields(node: SdsResult | undefined): SdsYield[] { + resultToYields(node: SdsResult | undefined): Stream { if (!node) { - return []; + return stream(); } const containingSegment = getContainerOfType(node, isSdsSegment); if (!containingSegment) { - return []; + return stream(); } return findLocalReferences(node, containingSegment) .map((it) => it.$refNode?.astNode) - .filter(isSdsYield) - .toArray(); + .filter(isSdsYield); } /** diff --git a/src/language/validation/other/declarations/parameters.ts b/src/language/validation/other/declarations/parameters.ts index 473db1763..9a2b3763e 100644 --- a/src/language/validation/other/declarations/parameters.ts +++ b/src/language/validation/other/declarations/parameters.ts @@ -1,6 +1,9 @@ -import { SdsParameter } from '../../../generated/ast.js'; +import { SdsParameter, SdsSegment } 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) => { @@ -12,3 +15,18 @@ 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/placeholders.ts b/src/language/validation/other/declarations/placeholders.ts new file mode 100644 index 000000000..7453fb788 --- /dev/null +++ b/src/language/validation/other/declarations/placeholders.ts @@ -0,0 +1,29 @@ +import { isSdsBlock, isSdsStatement, SdsPlaceholder } from '../../../generated/ast.js'; +import { getContainerOfType, ValidationAcceptor } from 'langium'; +import { SafeDsServices } from '../../../safe-ds-module.js'; +import { statementsOrEmpty } from '../../../helpers/nodeProperties.js'; +import { last } from 'radash'; + +export const CODE_PLACEHOLDER_UNUSED = 'placeholder/unused'; + +export const placeholderShouldBeUsed = + (services: SafeDsServices) => (node: SdsPlaceholder, accept: ValidationAcceptor) => { + const usages = services.helpers.NodeMapper.placeholderToReferences(node); + if (!usages.isEmpty()) { + return; + } + + // Don't show a warning if the placeholder is declared in the last statement in the block + const containingStatement = getContainerOfType(node, isSdsStatement); + const containingBlock = getContainerOfType(containingStatement, isSdsBlock); + const allStatementsInBlock = statementsOrEmpty(containingBlock); + if (last(allStatementsInBlock) === containingStatement) { + return; + } + + accept('warning', 'This placeholder is unused and can be removed.', { + node, + property: 'name', + code: CODE_PLACEHOLDER_UNUSED, + }); + }; diff --git a/src/language/validation/safe-ds-validator.ts b/src/language/validation/safe-ds-validator.ts index db15a5a3b..4ed3dc78d 100644 --- a/src/language/validation/safe-ds-validator.ts +++ b/src/language/validation/safe-ds-validator.ts @@ -45,7 +45,10 @@ 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 } from './other/declarations/parameters.js'; +import { + parameterMustNotBeVariadicAndOptional, + segmentParameterShouldBeUsed, +} from './other/declarations/parameters.js'; import { referenceTargetMustNotBeAnnotationPipelineOrSchema } from './other/expressions/references.js'; import { annotationCallAnnotationShouldNotBeDeprecated, @@ -61,6 +64,7 @@ import { namedTypeDeclarationShouldNotBeExperimental, referenceTargetShouldNotExperimental, } from './builtins/experimental.js'; +import { placeholderShouldBeUsed } from './other/declarations/placeholders.js'; /** * Register custom validation checks. @@ -111,13 +115,18 @@ export const registerValidationChecks = function (services: SafeDsServices) { parameterListVariadicParameterMustBeLast, ], SdsPipeline: [pipelineMustContainUniqueNames], + SdsPlaceholder: [placeholderShouldBeUsed(services)], SdsReference: [ referenceTargetMustNotBeAnnotationPipelineOrSchema, referenceTargetShouldNotBeDeprecated(services), referenceTargetShouldNotExperimental(services), ], SdsResult: [resultMustHaveTypeHint], - SdsSegment: [segmentMustContainUniqueNames, segmentResultListShouldNotBeEmpty], + SdsSegment: [ + segmentMustContainUniqueNames, + segmentParameterShouldBeUsed(services), + segmentResultListShouldNotBeEmpty, + ], SdsTemplateString: [templateStringMustHaveExpressionBetweenTwoStringParts], SdsTypeArgumentList: [typeArgumentListMustNotHavePositionalArgumentsAfterNamedArguments], SdsTypeParameterConstraint: [typeParameterConstraintLeftOperandMustBeOwnTypeParameter], diff --git a/tests/language/helpers/safe-ds-node-mapper/parameterToReferences.test.ts b/tests/language/helpers/safe-ds-node-mapper/parameterToReferences.test.ts index e0cebbaea..1ab825b97 100644 --- a/tests/language/helpers/safe-ds-node-mapper/parameterToReferences.test.ts +++ b/tests/language/helpers/safe-ds-node-mapper/parameterToReferences.test.ts @@ -15,7 +15,7 @@ describe('SafeDsNodeMapper', () => { describe('parameterToReferences', () => { it('should return an empty list if passed undefined', async () => { - expect(nodeMapper.parameterToReferences(undefined)).toStrictEqual([]); + expect(nodeMapper.parameterToReferences(undefined).toArray()).toStrictEqual([]); }); it('should return references in default values', async () => { @@ -24,7 +24,7 @@ describe('SafeDsNodeMapper', () => { `; const parameter = await getNodeOfType(services, code, isSdsParameter); - expect(nodeMapper.parameterToReferences(parameter)).toHaveLength(1); + expect(nodeMapper.parameterToReferences(parameter).toArray()).toHaveLength(1); }); it('should return references directly in body', async () => { @@ -36,7 +36,7 @@ describe('SafeDsNodeMapper', () => { `; const parameter = await getNodeOfType(services, code, isSdsParameter); - expect(nodeMapper.parameterToReferences(parameter)).toHaveLength(2); + expect(nodeMapper.parameterToReferences(parameter).toArray()).toHaveLength(2); }); it('should return references nested in body', async () => { @@ -50,7 +50,28 @@ describe('SafeDsNodeMapper', () => { `; const parameter = await getNodeOfType(services, code, isSdsParameter); - expect(nodeMapper.parameterToReferences(parameter)).toHaveLength(2); + expect(nodeMapper.parameterToReferences(parameter).toArray()).toHaveLength(2); + }); + + it('should return references in own parameter list', async () => { + const code = ` + segment mySegment(p1: Int, p2: Int = p1) {}; + `; + + const parameter = await getNodeOfType(services, code, isSdsParameter); + expect(nodeMapper.parameterToReferences(parameter).toArray()).toHaveLength(1); + }); + + it('should return references in nested parameter list', async () => { + const code = ` + segment mySegment(p1: Int) { + (p2: Int = p1) {}; + (p2: Int = p1) -> 1; + }; + `; + + const parameter = await getNodeOfType(services, code, isSdsParameter); + expect(nodeMapper.parameterToReferences(parameter).toArray()).toHaveLength(2); }); it('should not return references to other parameters', async () => { @@ -62,7 +83,7 @@ describe('SafeDsNodeMapper', () => { `; const parameter = await getNodeOfType(services, code, isSdsParameter); - expect(nodeMapper.parameterToReferences(parameter)).toHaveLength(1); + expect(nodeMapper.parameterToReferences(parameter).toArray()).toHaveLength(1); }); }); }); diff --git a/tests/language/helpers/safe-ds-node-mapper/placeholdersToReferences.test.ts b/tests/language/helpers/safe-ds-node-mapper/placeholdersToReferences.test.ts index 8caffb349..4d4c3d036 100644 --- a/tests/language/helpers/safe-ds-node-mapper/placeholdersToReferences.test.ts +++ b/tests/language/helpers/safe-ds-node-mapper/placeholdersToReferences.test.ts @@ -15,7 +15,7 @@ describe('SafeDsNodeMapper', () => { describe('placeholderToReferences', () => { it('should return an empty list if passed undefined', async () => { - expect(nodeMapper.placeholderToReferences(undefined)).toStrictEqual([]); + expect(nodeMapper.placeholderToReferences(undefined).toArray()).toStrictEqual([]); }); it('should return references in default values', async () => { @@ -27,7 +27,7 @@ describe('SafeDsNodeMapper', () => { `; const placeholder = await getNodeOfType(services, code, isSdsPlaceholder); - expect(nodeMapper.placeholderToReferences(placeholder)).toHaveLength(1); + expect(nodeMapper.placeholderToReferences(placeholder).toArray()).toHaveLength(1); }); it('should return references directly in body', async () => { @@ -41,23 +41,39 @@ describe('SafeDsNodeMapper', () => { `; const placeholder = await getNodeOfType(services, code, isSdsPlaceholder); - expect(nodeMapper.placeholderToReferences(placeholder)).toHaveLength(2); + expect(nodeMapper.placeholderToReferences(placeholder).toArray()).toHaveLength(2); }); it('should return references nested in body', async () => { const code = ` segment mySegment() { - val a1 = 1; - () { - a1; + val a1 = 1; + + () { + a1; + }; + () -> a1; }; - () -> a1; }; `; const placeholder = await getNodeOfType(services, code, isSdsPlaceholder); - expect(nodeMapper.placeholderToReferences(placeholder)).toHaveLength(2); + expect(nodeMapper.placeholderToReferences(placeholder).toArray()).toHaveLength(2); + }); + + it('should return references in nested parameter list', async () => { + const code = ` + segment mySegment(p1: Int) { + val a1 = 1; + + (p2: Int = a1) {}; + (p2: Int = a1) -> 1; + }; + `; + + const placeholder = await getNodeOfType(services, code, isSdsPlaceholder); + expect(nodeMapper.placeholderToReferences(placeholder).toArray()).toHaveLength(2); }); it('should not return references to other placeholders', async () => { @@ -72,7 +88,7 @@ describe('SafeDsNodeMapper', () => { `; const placeholder = await getNodeOfType(services, code, isSdsPlaceholder); - expect(nodeMapper.placeholderToReferences(placeholder)).toHaveLength(1); + expect(nodeMapper.placeholderToReferences(placeholder).toArray()).toHaveLength(1); }); }); }); diff --git a/tests/language/helpers/safe-ds-node-mapper/resultToYields.test.ts b/tests/language/helpers/safe-ds-node-mapper/resultToYields.test.ts index 289920025..45821f38e 100644 --- a/tests/language/helpers/safe-ds-node-mapper/resultToYields.test.ts +++ b/tests/language/helpers/safe-ds-node-mapper/resultToYields.test.ts @@ -15,14 +15,14 @@ describe('SafeDsNodeMapper', () => { describe('resultToYields', () => { it('should return an empty list if passed undefined', async () => { - expect(nodeMapper.resultToYields(undefined)).toStrictEqual([]); + expect(nodeMapper.resultToYields(undefined).toArray()).toStrictEqual([]); }); it('should return an empty list if result is not in a segment', async () => { const code = `fun myFunction() -> r1: Int`; const result = await getNodeOfType(services, code, isSdsResult); - expect(nodeMapper.resultToYields(result)).toStrictEqual([]); + expect(nodeMapper.resultToYields(result).toArray()).toStrictEqual([]); }); it('should return all yields that refer to a result', async () => { @@ -34,7 +34,7 @@ describe('SafeDsNodeMapper', () => { `; const result = await getNodeOfType(services, code, isSdsResult); - expect(nodeMapper.resultToYields(result)).toHaveLength(2); + expect(nodeMapper.resultToYields(result).toArray()).toHaveLength(2); }); it('should not return yields that refer to another result', async () => { @@ -47,7 +47,7 @@ describe('SafeDsNodeMapper', () => { `; const result = await getNodeOfType(services, code, isSdsResult); - expect(nodeMapper.resultToYields(result)).toHaveLength(1); + expect(nodeMapper.resultToYields(result).toArray()).toHaveLength(1); }); }); }); diff --git a/tests/resources/validation/other/declarations/parameter/unused/main.sdstest b/tests/resources/validation/other/declarations/parameter/unused/main.sdstest new file mode 100644 index 000000000..205dca661 --- /dev/null +++ b/tests/resources/validation/other/declarations/parameter/unused/main.sdstest @@ -0,0 +1,49 @@ +package tests.validation.declarations.parameters.unused + +segment mySegment( + // $TEST$ warning "This parameter is unused and can be removed." + »unused«: Int, + // $TEST$ no warning "This parameter is unused and can be removed." + »used«: Int +) { + used; + + /* + * Since we only allow lambdas as arguments, there signature is predefined, so no warning should be emitted. + */ + + ( + // $TEST$ no warning "This parameter is unused and can be removed." + »unused«: Int, + // $TEST$ no warning "This parameter is unused and can be removed." + »used«: Int + ) { + used; + }; + + ( + // $TEST$ no warning "This parameter is unused and can be removed." + »unused«: Int, + // $TEST$ no warning "This parameter is unused and can be removed." + »used«: Int + ) -> used; +} + +// $TEST$ no warning "This parameter is unused and can be removed." +annotation MyAnnotation(»unused«: Int) + +// $TEST$ no warning "This parameter is unused and can be removed." +class MyClass(»unused«: Int) + +enum MyEnum { + // $TEST$ no warning "This parameter is unused and can be removed." + MyEnumVariant(»unused«: Int) +} + +fun myFunction( + // $TEST$ no warning "This parameter is unused and can be removed." + »unused«: Int, + + // $TEST$ no warning "This parameter is unused and can be removed." + myCallableType: (»unused«: Int) -> (), +) diff --git a/tests/resources/validation/other/declarations/placeholder/unused/main.sdstest b/tests/resources/validation/other/declarations/placeholder/unused/main.sdstest new file mode 100644 index 000000000..49dde3b9d --- /dev/null +++ b/tests/resources/validation/other/declarations/placeholder/unused/main.sdstest @@ -0,0 +1,44 @@ +package tests.validation.declarations.placeholders.unused + +fun f() -> (r1: Int, r2: Int) + +segment mySegment() { + // $TEST$ warning "This placeholder is unused and can be removed." + val »unused« = 1; + + // $TEST$ no warning "This placeholder is unused and can be removed." + val »used« = 1; + used; + + // $TEST$ no warning "This placeholder is unused and can be removed." + // $TEST$ no warning "This placeholder is unused and can be removed." + val »last1«, val »last2« = f(); +} + +pipeline myPipeline1 { + // $TEST$ warning "This placeholder is unused and can be removed." + val »unused« = 1; + + // $TEST$ no warning "This placeholder is unused and can be removed." + val »used« = 1; + used; + + // $TEST$ no warning "This placeholder is unused and can be removed." + // $TEST$ no warning "This placeholder is unused and can be removed." + val »last1«, val »last2« = f(); +} + +pipeline myPipeline2 { + () { + // $TEST$ warning "This placeholder is unused and can be removed." + val »unused« = 1; + + // $TEST$ no warning "This placeholder is unused and can be removed." + val »used« = 1; + used; + + // $TEST$ no warning "This placeholder is unused and can be removed." + // $TEST$ no warning "This placeholder is unused and can be removed." + val »last1«, val »last2« = f(); + }; +}