Skip to content

Commit

Permalink
fix: wrong detection of useless statements that call parameters/unkno…
Browse files Browse the repository at this point in the history
…wn callables (#790)

### Summary of Changes

Statements that called parameters (with unknown value) or unknown
callables were wrongly marked as unused. Because if this, an incorrect
warning was shown and no code was generated for them.
  • Loading branch information
lars-reimann authored Nov 22, 2023
1 parent 9d9f4b7 commit a49b4b3
Show file tree
Hide file tree
Showing 14 changed files with 381 additions and 23 deletions.
8 changes: 4 additions & 4 deletions packages/safe-ds-lang/src/language/flow/model.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import { SdsCallable } from '../generated/ast.js';
import { SdsCallable, SdsParameter } from '../generated/ast.js';
import { stream, Stream } from 'langium';

export class CallGraph {
constructor(
readonly root: SdsCallable | undefined,
readonly root: SdsCallable | SdsParameter | undefined,
readonly children: CallGraph[],
readonly isRecursive: boolean = false,
) {}
Expand All @@ -12,11 +12,11 @@ export class CallGraph {
* Traverses the call graph depth-first in pre-order and returns a stream of all callables that are called directly
* or indirectly.
*/
streamCalledCallables(): Stream<SdsCallable | undefined> {
streamCalledCallables(): Stream<SdsCallable | SdsParameter | undefined> {
return stream(this.streamCalledCallablesGenerator());
}

private *streamCalledCallablesGenerator(): Generator<SdsCallable | undefined, void> {
private *streamCalledCallablesGenerator(): Generator<SdsCallable | SdsParameter | undefined, void> {
yield this.root;

for (const child of this.children) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ export class SafeDsCallGraphComputer {
return this.getExecutedCallsInCallable(syntheticCall.callable.callable, syntheticCall.substitutions);
}

private getExecutedCallsInCallable(callable: SdsCallable, substitutions: ParameterSubstitutions) {
private getExecutedCallsInCallable(callable: SdsCallable | SdsParameter, substitutions: ParameterSubstitutions) {
if (isSdsBlockLambda(callable) || isSdsExpressionLambda(callable) || isSdsSegment(callable)) {
return this.getExecutedCallsInPipelineCallable(callable, substitutions);
} else if (isSdsClass(callable) || isSdsEnumVariant(callable) || isSdsFunction(callable)) {
Expand Down Expand Up @@ -279,7 +279,7 @@ export class SafeDsCallGraphComputer {

// Parameter might have a default value
if (!callableOrParameter.defaultValue) {
return undefined;
return new NamedCallable(callableOrParameter);
}
return this.getEvaluatedCallable(callableOrParameter.defaultValue, substitutions);
} else if (isNamed(callableOrParameter)) {
Expand Down Expand Up @@ -314,15 +314,15 @@ export class SafeDsCallGraphComputer {
args: SdsArgument[],
substitutions: ParameterSubstitutions,
): ParameterSubstitutions {
if (!callable) {
if (!callable || isSdsParameter(callable.callable)) {
return NO_SUBSTITUTIONS;
}

// Substitutions on creation
const substitutionsOnCreation = callable.substitutionsOnCreation;

// Substitutions on call
const parameters = getParameters(callable?.callable);
const parameters = getParameters(callable.callable);
const substitutionsOnCall = new Map(
args.flatMap((it) => {
// Ignore arguments that don't get assigned to a parameter
Expand Down
10 changes: 6 additions & 4 deletions packages/safe-ds-lang/src/language/partialEvaluation/model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,9 @@ export const isConstant = (node: EvaluatedNode): node is Constant => {
// Callables
// -------------------------------------------------------------------------------------------------

export abstract class EvaluatedCallable<out T extends SdsCallable = SdsCallable> extends EvaluatedNode {
export abstract class EvaluatedCallable<
out T extends SdsCallable | SdsParameter = SdsCallable | SdsParameter,
> extends EvaluatedNode {
abstract readonly callable: T;
abstract readonly substitutionsOnCreation: ParameterSubstitutions;
override readonly isFullyEvaluated: boolean = false;
Expand Down Expand Up @@ -175,7 +177,7 @@ export class BlockLambdaClosure extends EvaluatedCallable<SdsBlockLambda> {
}

override toString(): string {
return `$BlockLambdaClosure`;
return `$blockLambdaClosure`;
}
}

Expand Down Expand Up @@ -204,11 +206,11 @@ export class ExpressionLambdaClosure extends EvaluatedCallable<SdsExpressionLamb
}

override toString(): string {
return '$ExpressionLambdaClosure';
return '$expressionLambdaClosure';
}
}

export class NamedCallable<T extends SdsCallable & NamedAstNode> extends EvaluatedCallable<T> {
export class NamedCallable<T extends (SdsCallable & NamedAstNode) | SdsParameter> extends EvaluatedCallable<T> {
override readonly isFullyEvaluated: boolean = false;
override readonly substitutionsOnCreation: ParameterSubstitutions = new Map();

Expand Down
17 changes: 17 additions & 0 deletions packages/safe-ds-lang/src/language/purity/model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,23 @@ export class PotentiallyImpureParameterCall extends ImpurityReason {
}
}

/**
* The function calls an unknown callable.
*/
class UnknownCallableCallClass extends ImpurityReason {
override isSideEffect = true;

override equals(other: unknown): boolean {
return other instanceof UnknownCallableCallClass;
}

override toString(): string {
return 'Unknown callable call';
}
}

export const UnknownCallableCall = new UnknownCallableCallClass();

/**
* A function contains a call that leads to endless recursion.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,18 @@ import {
type ImpurityReason,
OtherImpurityReason,
PotentiallyImpureParameterCall,
UnknownCallableCall,
} from './model.js';
import {
isSdsAnnotation,
isSdsAssignment,
isSdsCallable,
isSdsClass,
isSdsEnumVariant,
isSdsExpressionStatement,
isSdsFunction,
isSdsLambda,
isSdsParameter,
isSdsWildcard,
SdsCall,
SdsCallable,
Expand Down Expand Up @@ -86,6 +92,29 @@ export class SafeDsPurityComputer {
return isEmpty(this.getImpurityReasonsForExpression(node, substitutions));
}

/**
* Returns whether the given parameter is pure, i.e. only accepts pure callables.
*
* @param node
* The parameter to check.
*/
isPureParameter(node: SdsParameter | undefined): boolean {
const containingCallable = getContainerOfType(node, isSdsCallable);
if (
!containingCallable ||
isSdsAnnotation(containingCallable) ||
isSdsClass(containingCallable) ||
isSdsEnumVariant(containingCallable)
) {
return true;
} else if (isSdsFunction(containingCallable)) {
const expectedImpurityReason = new PotentiallyImpureParameterCall(node);
return !this.getImpurityReasons(containingCallable).some((it) => it.equals(expectedImpurityReason));
} else {
return false;
}
}

/**
* Returns whether the given callable has side effects.
*
Expand Down Expand Up @@ -184,9 +213,7 @@ export class SafeDsPurityComputer {
// Cache the result if no substitutions are given
if (isEmpty(substitutions)) {
const key = this.getNodeId(node);
return this.reasonsCache.get(key, () => {
return this.doGetImpurityReasons(node, substitutions);
});
return this.reasonsCache.get(key, () => this.doGetImpurityReasons(node, substitutions));
} else {
/* c8 ignore next 2 */
return this.doGetImpurityReasons(node, substitutions);
Expand All @@ -202,8 +229,12 @@ export class SafeDsPurityComputer {
}

const otherImpurityReasons = callGraph.streamCalledCallables().flatMap((it) => {
if (isSdsFunction(it)) {
if (!it) {
return [UnknownCallableCall];
} else if (isSdsFunction(it)) {
return this.getImpurityReasonsForFunction(it);
} else if (isSdsParameter(it) && !this.isPureParameter(it)) {
return [new PotentiallyImpureParameterCall(it)];
} else {
return EMPTY_STREAM;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -187,11 +187,11 @@ describe('partial evaluation model', async () => {
},
{
value: new BlockLambdaClosure(blockLambda1, new Map()),
expectedString: '$BlockLambdaClosure',
expectedString: '$blockLambdaClosure',
},
{
value: new ExpressionLambdaClosure(expressionLambda1, new Map()),
expectedString: '$ExpressionLambdaClosure',
expectedString: '$expressionLambdaClosure',
},
{
value: new NamedCallable(segment1),
Expand Down
9 changes: 9 additions & 0 deletions packages/safe-ds-lang/tests/language/purity/model.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
type ImpurityReason,
OtherImpurityReason,
PotentiallyImpureParameterCall,
UnknownCallableCall,
} from '../../../src/language/purity/model.js';
import { getNodeOfType } from '../../helpers/nodeFinder.js';
import { type EqualsTest, ToStringTest } from '../../helpers/testDescription.js';
Expand All @@ -33,6 +34,10 @@ describe('purity model', async () => {
unequalValueOfSameType: () => new PotentiallyImpureParameterCall(parameter),
valueOfOtherType: () => new FileRead('test.txt'),
},
{
value: () => UnknownCallableCall,
valueOfOtherType: () => EndlessRecursion,
},
{
value: () => EndlessRecursion,
valueOfOtherType: () => OtherImpurityReason,
Expand Down Expand Up @@ -99,6 +104,10 @@ describe('purity model', async () => {
value: new PotentiallyImpureParameterCall(parameter),
expectedString: 'Potentially impure call of f.p',
},
{
value: UnknownCallableCall,
expectedString: 'Unknown callable call',
},
{
value: EndlessRecursion,
expectedString: 'Endless recursion',
Expand Down
Loading

0 comments on commit a49b4b3

Please sign in to comment.