From a49b4b38c5ff16916a0a4467a480291653ed54d0 Mon Sep 17 00:00:00 2001 From: Lars Reimann Date: Wed, 22 Nov 2023 11:19:52 +0100 Subject: [PATCH] fix: wrong detection of useless statements that call parameters/unknown 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. --- .../safe-ds-lang/src/language/flow/model.ts | 8 +- .../flow/safe-ds-call-graph-computer.ts | 8 +- .../src/language/partialEvaluation/model.ts | 10 +- .../safe-ds-lang/src/language/purity/model.ts | 17 ++ .../purity/safe-ds-purity-computer.ts | 39 ++- .../language/partialEvaluation/model.test.ts | 4 +- .../tests/language/purity/model.test.ts | 9 + .../purity/safe-ds-purity-computer.test.ts | 285 +++++++++++++++++- .../call graph/callable type call.sdstest | 2 +- .../base cases/block lambdas/main.sdstest | 2 +- .../expression lambdas/main.sdstest | 2 +- .../infix operations/elvis/main.sdstest | 2 +- .../assignments/has no effect/main.sdstest | 8 + .../{hasNoEffect.sdstest => main.sdstest} | 8 + 14 files changed, 381 insertions(+), 23 deletions(-) rename packages/safe-ds-lang/tests/resources/validation/other/statements/expression statements/has no effect/{hasNoEffect.sdstest => main.sdstest} (91%) diff --git a/packages/safe-ds-lang/src/language/flow/model.ts b/packages/safe-ds-lang/src/language/flow/model.ts index 9e239cdcd..6a460e8e2 100644 --- a/packages/safe-ds-lang/src/language/flow/model.ts +++ b/packages/safe-ds-lang/src/language/flow/model.ts @@ -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, ) {} @@ -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 { + streamCalledCallables(): Stream { return stream(this.streamCalledCallablesGenerator()); } - private *streamCalledCallablesGenerator(): Generator { + private *streamCalledCallablesGenerator(): Generator { yield this.root; for (const child of this.children) { diff --git a/packages/safe-ds-lang/src/language/flow/safe-ds-call-graph-computer.ts b/packages/safe-ds-lang/src/language/flow/safe-ds-call-graph-computer.ts index cf080520c..6be573f37 100644 --- a/packages/safe-ds-lang/src/language/flow/safe-ds-call-graph-computer.ts +++ b/packages/safe-ds-lang/src/language/flow/safe-ds-call-graph-computer.ts @@ -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)) { @@ -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)) { @@ -314,7 +314,7 @@ export class SafeDsCallGraphComputer { args: SdsArgument[], substitutions: ParameterSubstitutions, ): ParameterSubstitutions { - if (!callable) { + if (!callable || isSdsParameter(callable.callable)) { return NO_SUBSTITUTIONS; } @@ -322,7 +322,7 @@ export class SafeDsCallGraphComputer { 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 diff --git a/packages/safe-ds-lang/src/language/partialEvaluation/model.ts b/packages/safe-ds-lang/src/language/partialEvaluation/model.ts index 87152e65a..87f3160ca 100644 --- a/packages/safe-ds-lang/src/language/partialEvaluation/model.ts +++ b/packages/safe-ds-lang/src/language/partialEvaluation/model.ts @@ -144,7 +144,9 @@ export const isConstant = (node: EvaluatedNode): node is Constant => { // Callables // ------------------------------------------------------------------------------------------------- -export abstract class EvaluatedCallable 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; @@ -175,7 +177,7 @@ export class BlockLambdaClosure extends EvaluatedCallable { } override toString(): string { - return `$BlockLambdaClosure`; + return `$blockLambdaClosure`; } } @@ -204,11 +206,11 @@ export class ExpressionLambdaClosure extends EvaluatedCallable extends EvaluatedCallable { +export class NamedCallable extends EvaluatedCallable { override readonly isFullyEvaluated: boolean = false; override readonly substitutionsOnCreation: ParameterSubstitutions = new Map(); diff --git a/packages/safe-ds-lang/src/language/purity/model.ts b/packages/safe-ds-lang/src/language/purity/model.ts index 790de94a7..ed3eebbe6 100644 --- a/packages/safe-ds-lang/src/language/purity/model.ts +++ b/packages/safe-ds-lang/src/language/purity/model.ts @@ -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. */ diff --git a/packages/safe-ds-lang/src/language/purity/safe-ds-purity-computer.ts b/packages/safe-ds-lang/src/language/purity/safe-ds-purity-computer.ts index 6b5f4ed71..dd3ad929c 100644 --- a/packages/safe-ds-lang/src/language/purity/safe-ds-purity-computer.ts +++ b/packages/safe-ds-lang/src/language/purity/safe-ds-purity-computer.ts @@ -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, @@ -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. * @@ -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); @@ -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; } diff --git a/packages/safe-ds-lang/tests/language/partialEvaluation/model.test.ts b/packages/safe-ds-lang/tests/language/partialEvaluation/model.test.ts index 349580f23..68ca8076a 100644 --- a/packages/safe-ds-lang/tests/language/partialEvaluation/model.test.ts +++ b/packages/safe-ds-lang/tests/language/partialEvaluation/model.test.ts @@ -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), diff --git a/packages/safe-ds-lang/tests/language/purity/model.test.ts b/packages/safe-ds-lang/tests/language/purity/model.test.ts index e5d33925d..46240ba02 100644 --- a/packages/safe-ds-lang/tests/language/purity/model.test.ts +++ b/packages/safe-ds-lang/tests/language/purity/model.test.ts @@ -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'; @@ -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, @@ -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', diff --git a/packages/safe-ds-lang/tests/language/purity/safe-ds-purity-computer.test.ts b/packages/safe-ds-lang/tests/language/purity/safe-ds-purity-computer.test.ts index 42ddd738f..5d86abfb5 100644 --- a/packages/safe-ds-lang/tests/language/purity/safe-ds-purity-computer.test.ts +++ b/packages/safe-ds-lang/tests/language/purity/safe-ds-purity-computer.test.ts @@ -1,7 +1,7 @@ import { describe, expect, it } from 'vitest'; import { NodeFileSystem } from 'langium/node'; import { createSafeDsServicesWithBuiltins } from '../../../src/language/index.js'; -import { isSdsCallable, isSdsExpression } from '../../../src/language/generated/ast.js'; +import { isSdsCallable, isSdsExpression, isSdsParameter } from '../../../src/language/generated/ast.js'; import { getNodeOfType } from '../../helpers/nodeFinder.js'; const services = (await createSafeDsServicesWithBuiltins(NodeFileSystem)).SafeDs; @@ -51,6 +51,37 @@ describe('SafeDsPurityComputer', async () => { `, expected: false, }, + { + testName: 'unknown callable', + code: ` + package test + + segment a() { + unresolved(); + } + `, + expected: false, + }, + { + testName: 'potentially impure parameter call', + code: ` + package test + + segment mySegment(param: () -> ()) { + param(); + } + `, + expected: false, + }, + { + testName: 'pure parameter call', + code: ` + package test + + class MyClass(param: () -> (result: Int), value: Int = param()) + `, + expected: true, + }, ])('should return whether a callable is pure ($testName)', async ({ code, expected }) => { const callable = await getNodeOfType(services, code, isSdsCallable); expect(purityComputer.isPureCallable(callable)).toBe(expected); @@ -126,12 +157,140 @@ describe('SafeDsPurityComputer', async () => { `, expected: false, }, + { + testName: 'unknown callable', + code: ` + package test + + segment a() { + unresolved(); + } + `, + expected: false, + }, + { + testName: 'potentially impure parameter call', + code: ` + package test + + segment mySegment(param: () -> ()) { + param(); + } + `, + expected: false, + }, + { + testName: 'pure parameter call', + code: ` + package test + + class MyClass(param: () -> (result: Int), value: Int = param()) + `, + expected: true, + }, ])('should return whether an expression is pure ($testName)', async ({ code, expected }) => { const expression = await getNodeOfType(services, code, isSdsExpression); expect(purityComputer.isPureExpression(expression)).toBe(expected); }); }); + describe('isPureExpression', () => { + it.each([ + { + testName: 'annotation parameter', + code: ` + package test + + annotation MyAnnotation(f: () -> ()) + `, + expected: true, + }, + { + testName: 'class parameter', + code: ` + package test + + class MyClass(f: () -> ()) + `, + expected: true, + }, + { + testName: 'enum variant parameter', + code: ` + package test + + enum MyEnum { + MyEnumVariant(f: () -> ()) + } + `, + expected: true, + }, + { + testName: 'pure function parameter', + code: ` + package test + + @Pure + fun myFunction(f: () -> ()) + `, + expected: true, + }, + { + testName: 'impure function parameter', + code: ` + package test + + @Impure([ImpurityReason.PotentiallyImpureParameterCall("f")]) + fun myFunction(f: () -> ()) + `, + expected: false, + }, + { + testName: 'callable type parameter', + code: ` + package test + + segment mySegment() -> (result: (f: () -> ()) -> ()) + `, + expected: false, + }, + { + testName: 'segment parameter', + code: ` + package test + + segment mySegment(f: () -> ()) + `, + expected: false, + }, + { + testName: 'block lambda parameter', + code: ` + package test + + pipeline myPipeline { + (f: () -> ()) {}; + } + `, + expected: false, + }, + { + testName: 'expression lambda parameter', + code: ` + package test + + pipeline myPipeline { + (f: () -> ()) -> 1; + } + `, + expected: false, + }, + ])('should return whether a parameter is pure ($testName)', async ({ code, expected }) => { + const parameter = await getNodeOfType(services, code, isSdsParameter); + expect(purityComputer.isPureParameter(parameter)).toBe(expected); + }); + }); + describe('callableHasSideEffects', () => { it.each([ { @@ -185,6 +344,37 @@ describe('SafeDsPurityComputer', async () => { `, expected: true, }, + { + testName: 'unknown callable', + code: ` + package test + + segment a() { + unresolved(); + } + `, + expected: true, + }, + { + testName: 'potentially impure parameter call', + code: ` + package test + + segment mySegment(param: () -> ()) { + param(); + } + `, + expected: true, + }, + { + testName: 'pure parameter call', + code: ` + package test + + class MyClass(param: () -> (result: Int), value: Int = param()) + `, + expected: false, + }, ])('should return whether a callable has side effects ($testName)', async ({ code, expected }) => { const callable = await getNodeOfType(services, code, isSdsCallable); expect(purityComputer.callableHasSideEffects(callable)).toBe(expected); @@ -274,6 +464,37 @@ describe('SafeDsPurityComputer', async () => { `, expected: true, }, + { + testName: 'unknown callable', + code: ` + package test + + segment a() { + unresolved(); + } + `, + expected: true, + }, + { + testName: 'potentially impure parameter call', + code: ` + package test + + segment mySegment(param: () -> ()) { + param(); + } + `, + expected: true, + }, + { + testName: 'pure parameter call', + code: ` + package test + + class MyClass(param: () -> (result: Int), value: Int = param()) + `, + expected: false, + }, ])('should return whether a call has side effects ($testName)', async ({ code, expected }) => { const expression = await getNodeOfType(services, code, isSdsExpression); expect(purityComputer.expressionHasSideEffects(expression)).toBe(expected); @@ -383,6 +604,37 @@ describe('SafeDsPurityComputer', async () => { `, expected: ['Endless recursion'], }, + { + testName: 'unknown callable', + code: ` + package test + + segment a() { + unresolved(); + } + `, + expected: ['Unknown callable call'], + }, + { + testName: 'potentially impure parameter call', + code: ` + package test + + segment mySegment(param: () -> ()) { + param(); + } + `, + expected: ['Potentially impure call of test.mySegment.param'], + }, + { + testName: 'pure parameter call', + code: ` + package test + + class MyClass(param: () -> (result: Int), value: Int = param()) + `, + expected: [], + }, ])('should return the impurity reasons of a callable ($testName)', async ({ code, expected }) => { const callable = await getNodeOfType(services, code, isSdsCallable); const actual = purityComputer.getImpurityReasonsForCallable(callable).map((reason) => reason.toString()); @@ -473,6 +725,37 @@ describe('SafeDsPurityComputer', async () => { `, expected: ['Endless recursion'], }, + { + testName: 'unknown callable', + code: ` + package test + + segment a() { + unresolved(); + } + `, + expected: ['Unknown callable call'], + }, + { + testName: 'potentially impure parameter call', + code: ` + package test + + segment mySegment(param: () -> ()) { + param(); + } + `, + expected: ['Potentially impure call of test.mySegment.param'], + }, + { + testName: 'pure parameter call', + code: ` + package test + + class MyClass(param: () -> (result: Int), value: Int = param()) + `, + expected: [], + }, ])('should return the impurity reasons of a callable ($testName)', async ({ code, expected }) => { const expression = await getNodeOfType(services, code, isSdsExpression); const actual = purityComputer diff --git a/packages/safe-ds-lang/tests/resources/call graph/callable type call.sdstest b/packages/safe-ds-lang/tests/resources/call graph/callable type call.sdstest index 9bb7b41a7..1ca186022 100644 --- a/packages/safe-ds-lang/tests/resources/call graph/callable type call.sdstest +++ b/packages/safe-ds-lang/tests/resources/call graph/callable type call.sdstest @@ -1,6 +1,6 @@ package tests.callGraph.callableTypeCall segment mySegment(param: () -> ()) { - // $TEST$ ["undefined"] + // $TEST$ ["param"] »param()«; } diff --git a/packages/safe-ds-lang/tests/resources/partial evaluation/base cases/block lambdas/main.sdstest b/packages/safe-ds-lang/tests/resources/partial evaluation/base cases/block lambdas/main.sdstest index e00137974..5e1a56dda 100644 --- a/packages/safe-ds-lang/tests/resources/partial evaluation/base cases/block lambdas/main.sdstest +++ b/packages/safe-ds-lang/tests/resources/partial evaluation/base cases/block lambdas/main.sdstest @@ -1,6 +1,6 @@ package tests.partialValidation.baseCases.expressionLambdas pipeline test { - // $TEST$ serialization $ExpressionLambdaClosure + // $TEST$ serialization $expressionLambdaClosure »() -> 1«; } diff --git a/packages/safe-ds-lang/tests/resources/partial evaluation/base cases/expression lambdas/main.sdstest b/packages/safe-ds-lang/tests/resources/partial evaluation/base cases/expression lambdas/main.sdstest index 8b3f1fdf6..686070ce9 100644 --- a/packages/safe-ds-lang/tests/resources/partial evaluation/base cases/expression lambdas/main.sdstest +++ b/packages/safe-ds-lang/tests/resources/partial evaluation/base cases/expression lambdas/main.sdstest @@ -1,6 +1,6 @@ package tests.partialValidation.baseCases.blockLambdas pipeline test { - // $TEST$ serialization $BlockLambdaClosure + // $TEST$ serialization $blockLambdaClosure »() {}«; } diff --git a/packages/safe-ds-lang/tests/resources/partial evaluation/recursive cases/infix operations/elvis/main.sdstest b/packages/safe-ds-lang/tests/resources/partial evaluation/recursive cases/infix operations/elvis/main.sdstest index 890d53628..ce7357a03 100644 --- a/packages/safe-ds-lang/tests/resources/partial evaluation/recursive cases/infix operations/elvis/main.sdstest +++ b/packages/safe-ds-lang/tests/resources/partial evaluation/recursive cases/infix operations/elvis/main.sdstest @@ -7,7 +7,7 @@ pipeline test { // $TEST$ serialization false »null ?: false«; - // $TEST$ serialization $ExpressionLambdaClosure + // $TEST$ serialization $expressionLambdaClosure »(() -> 1) ?: false«; // $TEST$ serialization ? diff --git a/packages/safe-ds-lang/tests/resources/validation/other/statements/assignments/has no effect/main.sdstest b/packages/safe-ds-lang/tests/resources/validation/other/statements/assignments/has no effect/main.sdstest index fa35045cc..107020009 100644 --- a/packages/safe-ds-lang/tests/resources/validation/other/statements/assignments/has no effect/main.sdstest +++ b/packages/safe-ds-lang/tests/resources/validation/other/statements/assignments/has no effect/main.sdstest @@ -19,3 +19,11 @@ segment mySegment() -> a: Int { »yield a = 1;« }; } + +segment mySegment2(f: () -> (r: Int)) { + // $TEST$ no warning "This statement does nothing." + »_ = f();« + + // $TEST$ no warning "This statement does nothing." + »_ = unresolved();« +} diff --git a/packages/safe-ds-lang/tests/resources/validation/other/statements/expression statements/has no effect/hasNoEffect.sdstest b/packages/safe-ds-lang/tests/resources/validation/other/statements/expression statements/has no effect/main.sdstest similarity index 91% rename from packages/safe-ds-lang/tests/resources/validation/other/statements/expression statements/has no effect/hasNoEffect.sdstest rename to packages/safe-ds-lang/tests/resources/validation/other/statements/expression statements/has no effect/main.sdstest index 640d48025..5dcf20fe1 100644 --- a/packages/safe-ds-lang/tests/resources/validation/other/statements/expression statements/has no effect/hasNoEffect.sdstest +++ b/packages/safe-ds-lang/tests/resources/validation/other/statements/expression statements/has no effect/main.sdstest @@ -81,3 +81,11 @@ segment mySegment() { // $TEST$ no warning "This statement does nothing." »recursiveA();« } + +segment mySegment2(f: () -> (r: Int)) { + // $TEST$ no warning "This statement does nothing." + »f();« + + // $TEST$ no warning "This statement does nothing." + »unresolved();« +}