Skip to content

Commit 928999a

Browse files
lars-reimannlars-reimann
committed
feat: warn if parameters or placeholders are unused
1 parent de863f8 commit 928999a

File tree

9 files changed

+182
-37
lines changed

9 files changed

+182
-37
lines changed

‎src/language/helpers/safe-ds-node-mapper.ts‎

Lines changed: 13 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ import {
2727
SdsYield,
2828
} from '../generated/ast.js';
2929
import { CallableType, StaticType } from '../typing/model.js';
30-
import { findLocalReferences, getContainerOfType } from 'langium';
30+
import { findLocalReferences, getContainerOfType, Stream, stream } from 'langium';
3131
import {
3232
abstractResultsOrEmpty,
3333
argumentsOrEmpty,
@@ -148,62 +148,59 @@ export class SafeDsNodeMapper {
148148
/**
149149
* Returns all references that target the given parameter.
150150
*/
151-
parameterToReferences(node: SdsParameter | undefined): SdsReference[] {
151+
parameterToReferences(node: SdsParameter | undefined): Stream<SdsReference> {
152152
if (!node) {
153-
return [];
153+
return stream();
154154
}
155155

156156
const containingCallable = getContainerOfType(node, isSdsCallable);
157157
/* c8 ignore start */
158158
if (!containingCallable) {
159-
return [];
159+
return stream();
160160
}
161161
/* c8 ignore stop */
162162

163163
return findLocalReferences(node, containingCallable)
164164
.map((it) => it.$refNode?.astNode)
165-
.filter(isSdsReference)
166-
.toArray();
165+
.filter(isSdsReference);
167166
}
168167

169168
/**
170169
* Returns all references that target the given placeholder.
171170
*/
172-
placeholderToReferences(node: SdsPlaceholder | undefined): SdsReference[] {
171+
placeholderToReferences(node: SdsPlaceholder | undefined): Stream<SdsReference> {
173172
if (!node) {
174-
return [];
173+
return stream();
175174
}
176175

177176
const containingBlock = getContainerOfType(node, isSdsBlock);
178177
/* c8 ignore start */
179178
if (!containingBlock) {
180-
return [];
179+
return stream();
181180
}
182181
/* c8 ignore stop */
183182

184183
return findLocalReferences(node, containingBlock)
185184
.map((it) => it.$refNode?.astNode)
186-
.filter(isSdsReference)
187-
.toArray();
185+
.filter(isSdsReference);
188186
}
189187

190188
/**
191189
* Returns all yields that assign to the given result.
192190
*/
193-
resultToYields(node: SdsResult | undefined): SdsYield[] {
191+
resultToYields(node: SdsResult | undefined): Stream<SdsYield> {
194192
if (!node) {
195-
return [];
193+
return stream();
196194
}
197195

198196
const containingSegment = getContainerOfType(node, isSdsSegment);
199197
if (!containingSegment) {
200-
return [];
198+
return stream();
201199
}
202200

203201
return findLocalReferences(node, containingSegment)
204202
.map((it) => it.$refNode?.astNode)
205-
.filter(isSdsYield)
206-
.toArray();
203+
.filter(isSdsYield);
207204
}
208205

209206
/**

‎src/language/validation/other/declarations/parameters.ts‎

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
1-
import { SdsParameter } from '../../../generated/ast.js';
1+
import { SdsParameter, SdsSegment } from '../../../generated/ast.js';
22
import { ValidationAcceptor } from 'langium';
3+
import { parametersOrEmpty } from '../../../helpers/nodeProperties.js';
4+
import { SafeDsServices } from '../../../safe-ds-module.js';
35

6+
export const CODE_PARAMETER_UNUSED = 'parameter/unused';
47
export const CODE_PARAMETER_VARIADIC_AND_OPTIONAL = 'parameter/variadic-and-optional';
58

69
export const parameterMustNotBeVariadicAndOptional = (node: SdsParameter, accept: ValidationAcceptor) => {
@@ -12,3 +15,18 @@ export const parameterMustNotBeVariadicAndOptional = (node: SdsParameter, accept
1215
});
1316
}
1417
};
18+
19+
export const segmentParameterShouldBeUsed =
20+
(services: SafeDsServices) => (node: SdsSegment, accept: ValidationAcceptor) => {
21+
for (const parameter of parametersOrEmpty(node)) {
22+
const usages = services.helpers.NodeMapper.parameterToReferences(parameter);
23+
24+
if (usages.isEmpty()) {
25+
accept('warning', 'This parameter is unused and can be removed.', {
26+
node: parameter,
27+
property: 'name',
28+
code: CODE_PARAMETER_UNUSED,
29+
});
30+
}
31+
}
32+
};

‎src/language/validation/other/declarations/placeholders.ts‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
import {isSdsBlock, isSdsStatement, SdsPlaceholder} from '../../../generated/ast.js';
2+
import {getContainerOfType, ValidationAcceptor} from 'langium';
3+
import { SafeDsServices } from '../../../safe-ds-module.js';
4+
import {statementsOrEmpty} from "../../../helpers/nodeProperties.js";
5+
import {last} from "radash";
6+
7+
export const CODE_PLACEHOLDER_UNUSED = 'placeholder/unused';
8+
9+
export const placeholderShouldBeUsed =
10+
(services: SafeDsServices) => (node: SdsPlaceholder, accept: ValidationAcceptor) => {
11+
const usages = services.helpers.NodeMapper.placeholderToReferences(node);
12+
if (!usages.isEmpty()) {
13+
return;
14+
}
15+
16+
// Don't show a warning if the placeholder is declared in the last statement in the block
17+
const containingStatement = getContainerOfType(node, isSdsStatement);
18+
const containingBlock = getContainerOfType(containingStatement, isSdsBlock);
19+
const allStatementsInBlock = statementsOrEmpty(containingBlock);
20+
if (last(allStatementsInBlock) === containingStatement) {
21+
return;
22+
}
23+
24+
accept('warning', 'This placeholder is unused and can be removed.', {
25+
node,
26+
property: 'name',
27+
code: CODE_PLACEHOLDER_UNUSED,
28+
});
29+
};

‎src/language/validation/safe-ds-validator.ts‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ import { unionTypeMustHaveTypeArguments } from './other/types/unionTypes.js';
4545
import { callableTypeMustNotHaveOptionalParameters } from './other/types/callableTypes.js';
4646
import { typeArgumentListMustNotHavePositionalArgumentsAfterNamedArguments } from './other/types/typeArgumentLists.js';
4747
import { argumentListMustNotHavePositionalArgumentsAfterNamedArguments } from './other/argumentLists.js';
48-
import { parameterMustNotBeVariadicAndOptional } from './other/declarations/parameters.js';
48+
import {parameterMustNotBeVariadicAndOptional, segmentParameterShouldBeUsed} from './other/declarations/parameters.js';
4949
import { referenceTargetMustNotBeAnnotationPipelineOrSchema } from './other/expressions/references.js';
5050
import {
5151
annotationCallAnnotationShouldNotBeDeprecated,
@@ -61,6 +61,7 @@ import {
6161
namedTypeDeclarationShouldNotBeExperimental,
6262
referenceTargetShouldNotExperimental,
6363
} from './builtins/experimental.js';
64+
import {placeholderShouldBeUsed} from "./other/declarations/placeholders.js";
6465

6566
/**
6667
* Register custom validation checks.
@@ -111,13 +112,20 @@ export const registerValidationChecks = function (services: SafeDsServices) {
111112
parameterListVariadicParameterMustBeLast,
112113
],
113114
SdsPipeline: [pipelineMustContainUniqueNames],
115+
SdsPlaceholder: [
116+
placeholderShouldBeUsed(services),
117+
],
114118
SdsReference: [
115119
referenceTargetMustNotBeAnnotationPipelineOrSchema,
116120
referenceTargetShouldNotBeDeprecated(services),
117121
referenceTargetShouldNotExperimental(services),
118122
],
119123
SdsResult: [resultMustHaveTypeHint],
120-
SdsSegment: [segmentMustContainUniqueNames, segmentResultListShouldNotBeEmpty],
124+
SdsSegment: [
125+
segmentMustContainUniqueNames,
126+
segmentParameterShouldBeUsed(services),
127+
segmentResultListShouldNotBeEmpty
128+
],
121129
SdsTemplateString: [templateStringMustHaveExpressionBetweenTwoStringParts],
122130
SdsTypeArgumentList: [typeArgumentListMustNotHavePositionalArgumentsAfterNamedArguments],
123131
SdsTypeParameterConstraint: [typeParameterConstraintLeftOperandMustBeOwnTypeParameter],

‎tests/language/helpers/safe-ds-node-mapper/parameterToReferences.test.ts‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ describe('SafeDsNodeMapper', () => {
1515

1616
describe('parameterToReferences', () => {
1717
it('should return an empty list if passed undefined', async () => {
18-
expect(nodeMapper.parameterToReferences(undefined)).toStrictEqual([]);
18+
expect(nodeMapper.parameterToReferences(undefined).toArray()).toStrictEqual([]);
1919
});
2020

2121
it('should return references in default values', async () => {
@@ -24,7 +24,7 @@ describe('SafeDsNodeMapper', () => {
2424
`;
2525

2626
const parameter = await getNodeOfType(services, code, isSdsParameter);
27-
expect(nodeMapper.parameterToReferences(parameter)).toHaveLength(1);
27+
expect(nodeMapper.parameterToReferences(parameter).toArray()).toHaveLength(1);
2828
});
2929

3030
it('should return references directly in body', async () => {
@@ -36,7 +36,7 @@ describe('SafeDsNodeMapper', () => {
3636
`;
3737

3838
const parameter = await getNodeOfType(services, code, isSdsParameter);
39-
expect(nodeMapper.parameterToReferences(parameter)).toHaveLength(2);
39+
expect(nodeMapper.parameterToReferences(parameter).toArray()).toHaveLength(2);
4040
});
4141

4242
it('should return references nested in body', async () => {
@@ -50,7 +50,7 @@ describe('SafeDsNodeMapper', () => {
5050
`;
5151

5252
const parameter = await getNodeOfType(services, code, isSdsParameter);
53-
expect(nodeMapper.parameterToReferences(parameter)).toHaveLength(2);
53+
expect(nodeMapper.parameterToReferences(parameter).toArray()).toHaveLength(2);
5454
});
5555

5656
it('should return references in own parameter list', async () => {
@@ -59,7 +59,7 @@ describe('SafeDsNodeMapper', () => {
5959
`;
6060

6161
const parameter = await getNodeOfType(services, code, isSdsParameter);
62-
expect(nodeMapper.parameterToReferences(parameter)).toHaveLength(1);
62+
expect(nodeMapper.parameterToReferences(parameter).toArray()).toHaveLength(1);
6363
});
6464

6565
it('should return references in nested parameter list', async () => {
@@ -71,7 +71,7 @@ describe('SafeDsNodeMapper', () => {
7171
`;
7272

7373
const parameter = await getNodeOfType(services, code, isSdsParameter);
74-
expect(nodeMapper.parameterToReferences(parameter)).toHaveLength(2);
74+
expect(nodeMapper.parameterToReferences(parameter).toArray()).toHaveLength(2);
7575
});
7676

7777
it('should not return references to other parameters', async () => {
@@ -83,7 +83,7 @@ describe('SafeDsNodeMapper', () => {
8383
`;
8484

8585
const parameter = await getNodeOfType(services, code, isSdsParameter);
86-
expect(nodeMapper.parameterToReferences(parameter)).toHaveLength(1);
86+
expect(nodeMapper.parameterToReferences(parameter).toArray()).toHaveLength(1);
8787
});
8888
});
8989
});

‎tests/language/helpers/safe-ds-node-mapper/placeholdersToReferences.test.ts‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import { createSafeDsServices } from '../../../../src/language/safe-ds-module.js
33
import { clearDocuments } from 'langium/test';
44
import { EmptyFileSystem } from 'langium';
55
import { getNodeOfType } from '../../../helpers/nodeFinder.js';
6-
import {isSdsParameter, isSdsPlaceholder} from '../../../../src/language/generated/ast.js';
6+
import { isSdsPlaceholder } from '../../../../src/language/generated/ast.js';
77

88
const services = createSafeDsServices(EmptyFileSystem).SafeDs;
99
const nodeMapper = services.helpers.NodeMapper;
@@ -15,7 +15,7 @@ describe('SafeDsNodeMapper', () => {
1515

1616
describe('placeholderToReferences', () => {
1717
it('should return an empty list if passed undefined', async () => {
18-
expect(nodeMapper.placeholderToReferences(undefined)).toStrictEqual([]);
18+
expect(nodeMapper.placeholderToReferences(undefined).toArray()).toStrictEqual([]);
1919
});
2020

2121
it('should return references in default values', async () => {
@@ -27,7 +27,7 @@ describe('SafeDsNodeMapper', () => {
2727
`;
2828

2929
const placeholder = await getNodeOfType(services, code, isSdsPlaceholder);
30-
expect(nodeMapper.placeholderToReferences(placeholder)).toHaveLength(1);
30+
expect(nodeMapper.placeholderToReferences(placeholder).toArray()).toHaveLength(1);
3131
});
3232

3333
it('should return references directly in body', async () => {
@@ -41,7 +41,7 @@ describe('SafeDsNodeMapper', () => {
4141
`;
4242

4343
const placeholder = await getNodeOfType(services, code, isSdsPlaceholder);
44-
expect(nodeMapper.placeholderToReferences(placeholder)).toHaveLength(2);
44+
expect(nodeMapper.placeholderToReferences(placeholder).toArray()).toHaveLength(2);
4545
});
4646

4747
it('should return references nested in body', async () => {
@@ -59,7 +59,7 @@ describe('SafeDsNodeMapper', () => {
5959
`;
6060

6161
const placeholder = await getNodeOfType(services, code, isSdsPlaceholder);
62-
expect(nodeMapper.placeholderToReferences(placeholder)).toHaveLength(2);
62+
expect(nodeMapper.placeholderToReferences(placeholder).toArray()).toHaveLength(2);
6363
});
6464

6565
it('should return references in nested parameter list', async () => {
@@ -73,7 +73,7 @@ describe('SafeDsNodeMapper', () => {
7373
`;
7474

7575
const placeholder = await getNodeOfType(services, code, isSdsPlaceholder);
76-
expect(nodeMapper.placeholderToReferences(placeholder)).toHaveLength(2);
76+
expect(nodeMapper.placeholderToReferences(placeholder).toArray()).toHaveLength(2);
7777
});
7878

7979
it('should not return references to other placeholders', async () => {
@@ -88,7 +88,7 @@ describe('SafeDsNodeMapper', () => {
8888
`;
8989

9090
const placeholder = await getNodeOfType(services, code, isSdsPlaceholder);
91-
expect(nodeMapper.placeholderToReferences(placeholder)).toHaveLength(1);
91+
expect(nodeMapper.placeholderToReferences(placeholder).toArray()).toHaveLength(1);
9292
});
9393
});
9494
});

‎tests/language/helpers/safe-ds-node-mapper/resultToYields.test.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,14 +15,14 @@ describe('SafeDsNodeMapper', () => {
1515

1616
describe('resultToYields', () => {
1717
it('should return an empty list if passed undefined', async () => {
18-
expect(nodeMapper.resultToYields(undefined)).toStrictEqual([]);
18+
expect(nodeMapper.resultToYields(undefined).toArray()).toStrictEqual([]);
1919
});
2020

2121
it('should return an empty list if result is not in a segment', async () => {
2222
const code = `fun myFunction() -> r1: Int`;
2323

2424
const result = await getNodeOfType(services, code, isSdsResult);
25-
expect(nodeMapper.resultToYields(result)).toStrictEqual([]);
25+
expect(nodeMapper.resultToYields(result).toArray()).toStrictEqual([]);
2626
});
2727

2828
it('should return all yields that refer to a result', async () => {
@@ -34,7 +34,7 @@ describe('SafeDsNodeMapper', () => {
3434
`;
3535

3636
const result = await getNodeOfType(services, code, isSdsResult);
37-
expect(nodeMapper.resultToYields(result)).toHaveLength(2);
37+
expect(nodeMapper.resultToYields(result).toArray()).toHaveLength(2);
3838
});
3939

4040
it('should not return yields that refer to another result', async () => {
@@ -47,7 +47,7 @@ describe('SafeDsNodeMapper', () => {
4747
`;
4848

4949
const result = await getNodeOfType(services, code, isSdsResult);
50-
expect(nodeMapper.resultToYields(result)).toHaveLength(1);
50+
expect(nodeMapper.resultToYields(result).toArray()).toHaveLength(1);
5151
});
5252
});
5353
});

‎tests/resources/validation/other/declarations/parameter/unused/main.sdstest‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
package tests.validation.declarations.parameters.unused
2+
3+
segment mySegment(
4+
// $TEST$ warning "This parameter is unused and can be removed."
5+
»unused«: Int,
6+
// $TEST$ no warning "This parameter is unused and can be removed."
7+
»used«: Int
8+
) {
9+
used;
10+
11+
/*
12+
* Since we only allow lambdas as arguments, there signature is predefined, so no warning should be emitted.
13+
*/
14+
15+
(
16+
// $TEST$ no warning "This parameter is unused and can be removed."
17+
»unused«: Int,
18+
// $TEST$ no warning "This parameter is unused and can be removed."
19+
»used«: Int
20+
) {
21+
used;
22+
};
23+
24+
(
25+
// $TEST$ no warning "This parameter is unused and can be removed."
26+
»unused«: Int,
27+
// $TEST$ no warning "This parameter is unused and can be removed."
28+
»used«: Int
29+
) -> used;
30+
}
31+
32+
// $TEST$ no warning "This parameter is unused and can be removed."
33+
annotation MyAnnotation(»unused«: Int)
34+
35+
// $TEST$ no warning "This parameter is unused and can be removed."
36+
class MyClass(»unused«: Int)
37+
38+
enum MyEnum {
39+
// $TEST$ no warning "This parameter is unused and can be removed."
40+
MyEnumVariant(»unused«: Int)
41+
}
42+
43+
fun myFunction(
44+
// $TEST$ no warning "This parameter is unused and can be removed."
45+
»unused«: Int,
46+
47+
// $TEST$ no warning "This parameter is unused and can be removed."
48+
myCallableType: (»unused«: Int) -> (),
49+
)

0 commit comments

Comments
 (0)