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>
Safe-DS · Oct 8, 2023 · 3a2e9cc · 3a2e9cc
1 parent 43b276f
commit 3a2e9cc
Show file tree

Hide file tree

Showing 9 changed files with 220 additions and 37 deletions.
diff --git 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<SdsReference> {
         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<SdsReference> {
         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<SdsYield> {
         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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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);
         });
     });
 });