From 5017759d7c03acdf854b451e7aa87509595cbe3b Mon Sep 17 00:00:00 2001 From: Lars Reimann Date: Fri, 24 Nov 2023 10:35:22 +0100 Subject: [PATCH] feat: error if index of indexed access is invalid (#796) Closes #16 ### Summary of Changes * Show an error if we know that a map key does not exist. * Show an error if we know that a list index is out of bounds. --- .../src/language/partialEvaluation/model.ts | 16 +++ .../safe-ds-partial-evaluator.ts | 98 +++++++++++-------- .../other/expressions/indexedAccess.ts | 59 +++++++++++ .../language/validation/safe-ds-validator.ts | 2 + .../list index out of bounds/main.sdstest | 40 ++++++++ .../map key does not exist/main.sdstest | 34 +++++++ 6 files changed, 209 insertions(+), 40 deletions(-) create mode 100644 packages/safe-ds-lang/src/language/validation/other/expressions/indexedAccess.ts create mode 100644 packages/safe-ds-lang/tests/resources/validation/other/expressions/indexed access/list index out of bounds/main.sdstest create mode 100644 packages/safe-ds-lang/tests/resources/validation/other/expressions/indexed access/map key does not exist/main.sdstest diff --git a/packages/safe-ds-lang/src/language/partialEvaluation/model.ts b/packages/safe-ds-lang/src/language/partialEvaluation/model.ts index 87f3160ca..49271ee35 100644 --- a/packages/safe-ds-lang/src/language/partialEvaluation/model.ts +++ b/packages/safe-ds-lang/src/language/partialEvaluation/model.ts @@ -297,6 +297,13 @@ export class EvaluatedList extends EvaluatedNode { return this.elements[index] ?? UnknownEvaluatedNode; } + /** + * Returns the size of the list. + */ + get size(): number { + return this.elements.length; + } + override equals(other: unknown): boolean { if (other === this) { return true; @@ -331,6 +338,15 @@ export class EvaluatedMap extends EvaluatedNode { return this.entries.findLast((it) => it.key.equals(key))?.value ?? UnknownEvaluatedNode; } + /** + * Returns whether the map contains the given key. + * + * @param key The key to look for. + */ + has(key: EvaluatedNode): boolean { + return this.entries.some((it) => it.key.equals(key)); + } + override equals(other: unknown): boolean { if (other === this) { return true; diff --git a/packages/safe-ds-lang/src/language/partialEvaluation/safe-ds-partial-evaluator.ts b/packages/safe-ds-lang/src/language/partialEvaluation/safe-ds-partial-evaluator.ts index f7157d6ed..7ff9acf64 100644 --- a/packages/safe-ds-lang/src/language/partialEvaluation/safe-ds-partial-evaluator.ts +++ b/packages/safe-ds-lang/src/language/partialEvaluation/safe-ds-partial-evaluator.ts @@ -1,10 +1,13 @@ -import { AstNode, AstNodeLocator, getDocument, WorkspaceCache } from 'langium'; +import { AstNode, AstNodeLocator, getContainerOfType, getDocument, WorkspaceCache } from 'langium'; import { isEmpty } from '../../helpers/collectionUtils.js'; import { isSdsArgument, + isSdsAssignee, + isSdsAssignment, isSdsBlockLambda, isSdsBoolean, isSdsCall, + isSdsDeclaration, isSdsEnumVariant, isSdsExpression, isSdsExpressionLambda, @@ -16,6 +19,7 @@ import { isSdsMap, isSdsMemberAccess, isSdsNull, + isSdsParameter, isSdsParenthesizedExpression, isSdsPrefixOperation, isSdsReference, @@ -25,15 +29,17 @@ import { isSdsTemplateStringEnd, isSdsTemplateStringInner, isSdsTemplateStringStart, + SdsAssignee, SdsCall, + SdsDeclaration, SdsExpression, SdsIndexedAccess, SdsInfixOperation, SdsList, SdsMap, SdsMemberAccess, + SdsParameter, SdsPrefixOperation, - SdsReference, SdsTemplateString, } from '../generated/ast.js'; import { getArguments, getParameters } from '../helpers/nodeProperties.js'; @@ -47,6 +53,7 @@ import { EvaluatedList, EvaluatedMap, EvaluatedMapEntry, + EvaluatedNamedTuple, EvaluatedNode, ExpressionLambdaClosure, FloatConstant, @@ -111,8 +118,42 @@ export class SafeDsPartialEvaluator { node: AstNode | undefined, substitutions: ParameterSubstitutions, ): EvaluatedNode { - if (isSdsExpression(node)) { + if (isSdsAssignee(node)) { + return this.evaluateAssignee(node, substitutions); + } else if (isSdsDeclaration(node)) { + return this.evaluateDeclaration(node, substitutions); + } else if (isSdsExpression(node)) { return this.evaluateExpression(node, substitutions); + } /* c8 ignore start */ else { + return UnknownEvaluatedNode; + } /* c8 ignore stop */ + } + + private evaluateAssignee(node: SdsAssignee, substitutions: ParameterSubstitutions): EvaluatedNode { + const containingAssignment = getContainerOfType(node, isSdsAssignment); + if (!containingAssignment) { + /* c8 ignore next 2 */ + return UnknownEvaluatedNode; + } + + const evaluatedExpression = this.evaluateWithSubstitutions(containingAssignment.expression, substitutions); + const nodeIndex = node.$containerIndex ?? -1; + if (evaluatedExpression instanceof EvaluatedNamedTuple) { + /* c8 ignore next */ // TODO test + return evaluatedExpression.getSubstitutionByIndex(nodeIndex); + } else if (nodeIndex === 0) { + return evaluatedExpression; + } else { + /* c8 ignore next 2 */ // TODO test + return UnknownEvaluatedNode; + } + } + + private evaluateDeclaration(node: SdsDeclaration, substitutions: ParameterSubstitutions): EvaluatedNode { + if (isSdsEnumVariant(node)) { + return new EvaluatedEnumVariant(node, undefined); + } else if (isSdsParameter(node)) { + return this.evaluateParameter(node, substitutions); } else if (isSdsSegment(node)) { return new NamedCallable(node); } else { @@ -120,6 +161,17 @@ export class SafeDsPartialEvaluator { } } + private evaluateParameter(node: SdsParameter, substitutions: ParameterSubstitutions): EvaluatedNode { + if (substitutions.has(node)) { + /* c8 ignore next */ // TODO test + return substitutions.get(node)!; + } else if (node.defaultValue) { + return this.evaluateWithSubstitutions(node.defaultValue, substitutions); + } else { + return UnknownEvaluatedNode; + } + } + private evaluateExpression(node: SdsExpression, substitutions: ParameterSubstitutions): EvaluatedNode { // Base cases if (isSdsBoolean(node)) { @@ -164,7 +216,7 @@ export class SafeDsPartialEvaluator { } else if (isSdsPrefixOperation(node)) { return this.evaluatePrefixOperation(node, substitutions); } else if (isSdsReference(node)) { - return this.evaluateReference(node, substitutions); + return this.evaluateWithSubstitutions(node.target.ref, substitutions); } else if (isSdsTemplateString(node)) { return this.evaluateTemplateString(node, substitutions); } /* c8 ignore start */ else { @@ -508,10 +560,10 @@ export class SafeDsPartialEvaluator { return UnknownEvaluatedNode; } - private evaluateMemberAccess(node: SdsMemberAccess, _substitutions: ParameterSubstitutions): EvaluatedNode { + private evaluateMemberAccess(node: SdsMemberAccess, substitutions: ParameterSubstitutions): EvaluatedNode { const member = node.member?.target?.ref; if (isSdsEnumVariant(member)) { - return new EvaluatedEnumVariant(member, undefined); + return this.evaluateWithSubstitutions(member, substitutions); } // return when (val simpleReceiver = receiver.evaluate(substitutions)) { @@ -525,40 +577,6 @@ export class SafeDsPartialEvaluator { return UnknownEvaluatedNode; } - private evaluateReference(_node: SdsReference, _substitutions: ParameterSubstitutions): EvaluatedNode { - // TODO: always call evaluateWithSubstitutions so caching works - // const target = node.target.ref; - - // is SdsPlaceholder -> declaration.evaluateAssignee(substitutions) - // is SdsParameter -> declaration.evaluateParameter(substitutions) - // else -> undefined - // } - return UnknownEvaluatedNode; - } - - // private fun SdsAbstractAssignee.evaluateAssignee(substitutions: ParameterSubstitutions): SdsSimplifiedExpression? { - // val simpleFullAssignedExpression = closestAncestorOrNull() - // ?.expression - // ?.evaluate(substitutions) - // ?: return undefined - // - // return when (simpleFullAssignedExpression) { - // is SdsIntermediateRecord -> simpleFullAssignedExpression.getSubstitutionByIndexOrNull(indexOrNull()) - // else -> when { - // indexOrNull() == 0 -> simpleFullAssignedExpression - // else -> undefined - // } - // } - // } - // - // private fun SdsParameter.evaluateParameter(substitutions: ParameterSubstitutions): SdsSimplifiedExpression? { - // return when { - // this in substitutions -> substitutions[this] - // isOptional() -> defaultValue?.evaluate(substitutions) - // else -> undefined - // } - // } - // ----------------------------------------------------------------------------------------------------------------- // canBeValueOfConstantParameter // ----------------------------------------------------------------------------------------------------------------- diff --git a/packages/safe-ds-lang/src/language/validation/other/expressions/indexedAccess.ts b/packages/safe-ds-lang/src/language/validation/other/expressions/indexedAccess.ts new file mode 100644 index 000000000..a01e0485e --- /dev/null +++ b/packages/safe-ds-lang/src/language/validation/other/expressions/indexedAccess.ts @@ -0,0 +1,59 @@ +import { SdsIndexedAccess } from '../../../generated/ast.js'; +import { ValidationAcceptor } from 'langium'; +import { SafeDsServices } from '../../../safe-ds-module.js'; +import { EvaluatedList, EvaluatedMap, IntConstant } from '../../../partialEvaluation/model.js'; + +export const CODE_INDEXED_ACCESS_INVALID_INDEX = 'indexed-access/invalid-index'; + +export const indexedAccessIndexMustBeValid = (services: SafeDsServices) => { + const coreTypes = services.types.CoreTypes; + const partialEvaluator = services.evaluation.PartialEvaluator; + const typeComputer = services.types.TypeComputer; + + return (node: SdsIndexedAccess, accept: ValidationAcceptor): void => { + const indexValue = partialEvaluator.evaluate(node.index); + if (!indexValue.isFullyEvaluated) { + return; + } + + const receiverValue = partialEvaluator.evaluate(node.receiver); + + // Check map key + if (receiverValue instanceof EvaluatedMap) { + if (!receiverValue.has(indexValue)) { + accept('error', `Map key '${indexValue}' does not exist.`, { + node, + property: 'index', + code: CODE_INDEXED_ACCESS_INVALID_INDEX, + }); + } + return; + } + + // Check list index + if (!(indexValue instanceof IntConstant)) { + return; + } + + if (receiverValue instanceof EvaluatedList) { + if (indexValue.value < 0 || indexValue.value >= receiverValue.size) { + accept('error', `List index '${indexValue}' is out of bounds.`, { + node, + property: 'index', + code: CODE_INDEXED_ACCESS_INVALID_INDEX, + }); + } + } + + const receiverType = typeComputer.computeType(node.receiver); + if (receiverType.equals(coreTypes.List)) { + if (indexValue.value < 0) { + accept('error', `List index '${indexValue}' is out of bounds.`, { + node, + property: 'index', + code: CODE_INDEXED_ACCESS_INVALID_INDEX, + }); + } + } + }; +}; diff --git a/packages/safe-ds-lang/src/language/validation/safe-ds-validator.ts b/packages/safe-ds-lang/src/language/validation/safe-ds-validator.ts index 1f0717c7a..936bbc8b0 100644 --- a/packages/safe-ds-lang/src/language/validation/safe-ds-validator.ts +++ b/packages/safe-ds-lang/src/language/validation/safe-ds-validator.ts @@ -178,6 +178,7 @@ import { yieldTypeMustMatchResultType, } from './types.js'; import { statementMustDoSomething } from './other/statements/statements.js'; +import { indexedAccessIndexMustBeValid } from './other/expressions/indexedAccess.js'; /** * Register custom validation checks. @@ -270,6 +271,7 @@ export const registerValidationChecks = function (services: SafeDsServices) { SdsImport: [importPackageMustExist(services), importPackageShouldNotBeEmpty(services)], SdsImportedDeclaration: [importedDeclarationAliasShouldDifferFromDeclarationName(services)], SdsIndexedAccess: [ + indexedAccessIndexMustBeValid(services), indexedAccessIndexMustHaveCorrectType(services), indexedAccessReceiverMustBeListOrMap(services), indexedAccessesShouldBeUsedWithCaution(services), diff --git a/packages/safe-ds-lang/tests/resources/validation/other/expressions/indexed access/list index out of bounds/main.sdstest b/packages/safe-ds-lang/tests/resources/validation/other/expressions/indexed access/list index out of bounds/main.sdstest new file mode 100644 index 000000000..729264583 --- /dev/null +++ b/packages/safe-ds-lang/tests/resources/validation/other/expressions/indexed access/list index out of bounds/main.sdstest @@ -0,0 +1,40 @@ +package tests.validation.other.indexedAccess.listIndexOutOfBounds + +@Pure +fun myFunction() -> resultList: List + +segment mySegment(parameterList: List, parameterIndex: Int) { + val placeholderList = [1]; + + // $TEST$ error "List index '-1' is out of bounds." + myFunction()[»-1«]; + // $TEST$ no error r"List index '.*' is out of bounds\." + myFunction()[»0«]; + // $TEST$ no error r"List index '.*' is out of bounds\." + myFunction()[»1«]; + // $TEST$ no error r"List index '.*' is out of bounds\." + myFunction()[»parameterIndex«]; + + // $TEST$ error "List index '-1' is out of bounds." + parameterList[»-1«]; + // $TEST$ no error r"List index '.*' is out of bounds\." + parameterList[»0«]; + // $TEST$ no error r"List index '.*' is out of bounds\." + parameterList[»1«]; + // $TEST$ no error r"List index '.*' is out of bounds\." + parameterList[»parameterIndex«]; + + // $TEST$ error "List index '-1' is out of bounds." + placeholderList[»-1«]; + // $TEST$ no error r"List index '.*' is out of bounds\." + placeholderList[»0«]; + // $TEST$ error "List index '1' is out of bounds." + placeholderList[»1«]; + // $TEST$ no error r"List index '.*' is out of bounds\." + placeholderList[»parameterIndex«]; + + // $TEST$ no error r"List index '.*' is out of bounds\." + unresolved[»-1«]; + // $TEST$ no error r"List index '.*' is out of bounds\." + placeholderList[»unresolved«]; +} diff --git a/packages/safe-ds-lang/tests/resources/validation/other/expressions/indexed access/map key does not exist/main.sdstest b/packages/safe-ds-lang/tests/resources/validation/other/expressions/indexed access/map key does not exist/main.sdstest new file mode 100644 index 000000000..b2978f1ec --- /dev/null +++ b/packages/safe-ds-lang/tests/resources/validation/other/expressions/indexed access/map key does not exist/main.sdstest @@ -0,0 +1,34 @@ +package tests.validation.other.indexedAccess.mapKeyDoesNotExist + +@Pure +fun myFunction() -> resultMap: Map + +segment mySegment(parameterMap: Map, parameterKey: String) { + val placeholderMap = {"key": 1}; + + // $TEST$ no error r"Map key '.*' does not exist\." + myFunction()[»"unknown"«]; + // $TEST$ no error r"Map key '.*' does not exist\." + myFunction()[»"key"«]; + // $TEST$ no error r"Map key '.*' does not exist\." + myFunction()[»parameterKey«]; + + // $TEST$ no error r"Map key '.*' does not exist\." + parameterMap[»"unknown"«]; + // $TEST$ no error r"Map key '.*' does not exist\." + parameterMap[»"key"«]; + // $TEST$ no error r"Map key '.*' does not exist\." + parameterMap[»parameterKey«]; + + // $TEST$ error "Map key '"unknown"' does not exist." + placeholderMap[»"unknown"«]; + // $TEST$ no error r"Map key '.*' does not exist\." + placeholderMap[»"key"«]; + // $TEST$ no error r"Map key '.*' does not exist\." + placeholderMap[»parameterKey«]; + + // $TEST$ no error r"Map key '.*' does not exist\." + unresolved[»"unknown"«]; + // $TEST$ no error r"Map key '.*' does not exist\." + placeholderMap[»unresolved«]; +}