Skip to content

Commit

Permalink
feat: warn if statement has no effect (#787)
Browse files Browse the repository at this point in the history
Closes #664

### Summary of Changes

Show a warning if a statement has no effect.
  • Loading branch information
lars-reimann authored Nov 21, 2023
1 parent cd4f2c1 commit 6f45dc4
Show file tree
Hide file tree
Showing 6 changed files with 102 additions and 161 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -61,13 +61,19 @@ export class SafeDsCallGraphComputer {
*/
private readonly callCache: WorkspaceCache<string, SdsCall[]>;

/**
* Stores the call graph for the callable with the given ID if it is called without substitutions.
*/
private readonly callGraphCache: WorkspaceCache<string, CallGraph>;

constructor(services: SafeDsServices) {
this.astNodeLocator = services.workspace.AstNodeLocator;
this.nodeMapper = services.helpers.NodeMapper;
this.partialEvaluator = services.evaluation.PartialEvaluator;
this.typeComputer = services.types.TypeComputer;

this.callCache = new WorkspaceCache(services.shared);
this.callGraphCache = new WorkspaceCache(services.shared);
}

/**
Expand Down Expand Up @@ -98,6 +104,22 @@ export class SafeDsCallGraphComputer {
* of any containing callables, i.e. the context of the call/callable.
*/
getCallGraph(node: SdsCall | SdsCallable, substitutions: ParameterSubstitutions = NO_SUBSTITUTIONS): CallGraph {
// Cache the result if no substitutions are given
if (isEmpty(substitutions)) {
const key = this.getNodeId(node);
return this.callGraphCache.get(key, () => {
return this.doGetCallGraph(node, substitutions);
});
} else {
/* c8 ignore next 2 */
return this.doGetCallGraph(node, substitutions);
}
}

private doGetCallGraph(
node: SdsCall | SdsCallable,
substitutions: ParameterSubstitutions = NO_SUBSTITUTIONS,
): CallGraph {
if (isSdsCall(node)) {
const call = this.createSyntheticCallForCall(node, substitutions);
return this.getCallGraphWithRecursionCheck(call, []);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
import {
isSdsAssignment,
isSdsExpressionStatement,
isSdsWildcard,
SdsExpression,
SdsStatement,
} from '../../../generated/ast.js';
import { ValidationAcceptor } from 'langium';
import { SafeDsServices } from '../../../safe-ds-module.js';
import { getAssignees } from '../../../helpers/nodeProperties.js';

export const CODE_STATEMENT_HAS_NO_EFFECT = 'statement/has-no-effect';

export const statementMustDoSomething = (services: SafeDsServices) => {
const statementDoesSomething = statementDoesSomethingProvider(services);

return (node: SdsStatement, accept: ValidationAcceptor): void => {
if (!statementDoesSomething(node)) {
accept('warning', 'This statement does nothing.', {
node,
code: CODE_STATEMENT_HAS_NO_EFFECT,
});
}
};
};

const statementDoesSomethingProvider = (services: SafeDsServices) => {
const statementExpressionDoesSomething = statementExpressionDoesSomethingProvider(services);

return (node: SdsStatement): boolean => {
if (isSdsAssignment(node)) {
return !getAssignees(node).every(isSdsWildcard) || statementExpressionDoesSomething(node.expression);
} else if (isSdsExpressionStatement(node)) {
return statementExpressionDoesSomething(node.expression);
} else {
/* c8 ignore next 2 */
return false;
}
};
};

const statementExpressionDoesSomethingProvider = (services: SafeDsServices) => {
const callGraphComputer = services.flow.CallGraphComputer;
const purityComputer = services.purity.PurityComputer;

return (node: SdsExpression | undefined): boolean => {
if (!node) {
/* c8 ignore next 2 */
return false;
}

return (
callGraphComputer.getAllContainedCalls(node).some((it) => callGraphComputer.isRecursive(it)) ||
!purityComputer.isPureExpression(node)
);
};
};
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ import {
resultMustHaveTypeHint,
yieldTypeMustMatchResultType,
} from './types.js';
import { statementMustDoSomething } from './other/statements/statements.js';

/**
* Register custom validation checks.
Expand Down Expand Up @@ -339,6 +340,7 @@ export const registerValidationChecks = function (services: SafeDsServices) {
segmentResultListShouldNotBeEmpty,
segmentShouldBeUsed(services),
],
SdsStatement: [statementMustDoSomething(services)],
SdsTemplateString: [templateStringMustHaveExpressionBetweenTwoStringParts],
SdsTypeArgumentList: [typeArgumentListsShouldBeUsedWithCaution],
SdsTypeParameter: [
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
step myFunction() -> a: Int {
package tests.validation.other.statements.assignments.hasNoEffect

segment mySegment() -> a: Int {
// $TEST$ warning "This statement does nothing."
»_ = 1 + 2;«

Expand Down
Original file line number Diff line number Diff line change
@@ -1,30 +1,35 @@
package tests.validation.statements.expressionStatements.hasNoEffect
package tests.validation.other.statements.expressionStatements.hasNoEffect

@Impure([ImpurityReason.Other])
fun impureFunction()
@Pure fun pureFunction() -> a: Int

@Pure
fun pureFunction() -> a: Int

class MyClass() {
@Impure([ImpurityReason.Other])
fun impureFunction()

@Pure fun pureFunction()
}

step pureStep() {
segment pureSegment() {
val a = pureFunction();
}

step impureStep() {
segment impureSegment() {
impureFunction();
}

step recursiveA() {
segment recursiveA() {
recursiveB();
}

step recursiveB() {
segment recursiveB() {
recursiveA();
}

step myStep() {
segment mySegment() {
// $TEST$ warning "This statement does nothing."
»1 + 2;«
// $TEST$ warning "This statement does nothing."
Expand All @@ -51,15 +56,18 @@ step myStep() {
»MyClass().impureFunction();«
};

// $TEST$ warning "This statement does nothing."
»pureSegment();«

// $TEST$ no warning "This statement does nothing."
»impureSegment();«

// $TEST$ warning "This statement does nothing."
»(() {
pureFunction();
MyClass().pureFunction();
})();«

// $TEST$ warning "This statement does nothing."
»pureStep();«

// $TEST$ no warning "This statement does nothing."
»(() {
impureFunction();
Expand All @@ -70,9 +78,6 @@ step myStep() {
MyClass().impureFunction();
})();«

// $TEST$ no warning "This statement does nothing."
»impureStep();«

// $TEST$ no warning "This statement does nothing."
»recursiveA();«
}

This file was deleted.

0 comments on commit 6f45dc4

Please sign in to comment.