From 077daff349b28d0f8142f3bdf3afc89bbc34cc39 Mon Sep 17 00:00:00 2001 From: Lars Reimann Date: Tue, 10 Oct 2023 15:40:39 +0200 Subject: [PATCH] feat: error if member access must be null safe but isn't (#626) Closes partially #543 ### Summary of Changes * If a member access is not null safe, but the receiver is nullable, an error is shown now. * The scope provider is now more relaxed and points to the correct class members, even if the member access is missing null safety. --------- Co-authored-by: megalinter-bot <129584137+megalinter-bot@users.noreply.github.com> --- .../scoping/safe-ds-scope-provider.ts | 3 -- .../other/expressions/memberAccesses.ts | 26 ++++++++++++ src/language/validation/safe-ds-validator.ts | 6 ++- .../instance attributes/main.sdstest | 6 +-- .../instance methods/main.sdstest | 6 +-- .../missing safe access/main.sdstest | 40 +++++++++++++++++++ .../unnecessary safe access/main.sdstest | 30 +++++++++++--- 7 files changed, 101 insertions(+), 16 deletions(-) create mode 100644 src/language/validation/other/expressions/memberAccesses.ts create mode 100644 tests/resources/validation/other/expressions/member accesses/missing safe access/main.sdstest diff --git a/src/language/scoping/safe-ds-scope-provider.ts b/src/language/scoping/safe-ds-scope-provider.ts index 9c9ec45a2..1839a2d09 100644 --- a/src/language/scoping/safe-ds-scope-provider.ts +++ b/src/language/scoping/safe-ds-scope-provider.ts @@ -205,9 +205,6 @@ export class SafeDsScopeProvider extends DefaultScopeProvider { // Members let receiverType = this.typeComputer.computeType(node.receiver); - if (receiverType.isNullable && !node.isNullSafe) { - return resultScope; - } if (receiverType instanceof ClassType) { const ownInstanceMembers = classMembersOrEmpty(receiverType.sdsClass, (it) => !isStatic(it)); diff --git a/src/language/validation/other/expressions/memberAccesses.ts b/src/language/validation/other/expressions/memberAccesses.ts new file mode 100644 index 000000000..7a4d19066 --- /dev/null +++ b/src/language/validation/other/expressions/memberAccesses.ts @@ -0,0 +1,26 @@ +import { SafeDsServices } from '../../../safe-ds-module.js'; +import { SdsMemberAccess } from '../../../generated/ast.js'; +import { ValidationAcceptor } from 'langium'; +import { UnknownType } from '../../../typing/model.js'; + +export const CODE_MEMBER_ACCESS_MISSING_NULL_SAFETY = 'member-access/missing-null-safety'; + +export const memberAccessMustBeNullSafeIfReceiverIsNullable = + (services: SafeDsServices) => + (node: SdsMemberAccess, accept: ValidationAcceptor): void => { + if (node.isNullSafe) { + return; + } + + const receiverType = services.types.TypeComputer.computeType(node.receiver); + if (receiverType === UnknownType) { + return; + } + + if (receiverType.isNullable) { + accept('error', 'The receiver can be null so a safe access must be used.', { + node, + code: CODE_MEMBER_ACCESS_MISSING_NULL_SAFETY, + }); + } + }; diff --git a/src/language/validation/safe-ds-validator.ts b/src/language/validation/safe-ds-validator.ts index 444240c2b..f6d837666 100644 --- a/src/language/validation/safe-ds-validator.ts +++ b/src/language/validation/safe-ds-validator.ts @@ -71,6 +71,7 @@ import { callableTypeResultsMustNotBeAnnotated, lambdaParametersMustNotBeAnnotated, } from './other/declarations/annotationCalls.js'; +import { memberAccessMustBeNullSafeIfReceiverIsNullable } from './other/expressions/memberAccesses.js'; /** * Register custom validation checks. @@ -119,7 +120,10 @@ export const registerValidationChecks = function (services: SafeDsServices) { SdsFunction: [functionMustContainUniqueNames, functionResultListShouldNotBeEmpty], SdsIndexedAccess: [indexedAccessesShouldBeUsedWithCaution], SdsLambda: [lambdaParametersMustNotBeAnnotated, lambdaParameterMustNotHaveConstModifier], - SdsMemberAccess: [memberAccessNullSafetyShouldBeNeeded(services)], + SdsMemberAccess: [ + memberAccessMustBeNullSafeIfReceiverIsNullable(services), + memberAccessNullSafetyShouldBeNeeded(services), + ], SdsModule: [moduleDeclarationsMustMatchFileKind, moduleWithDeclarationsMustStatePackage], SdsNamedType: [ namedTypeDeclarationShouldNotBeDeprecated(services), diff --git a/tests/resources/scoping/member accesses/to class members/instance attributes/main.sdstest b/tests/resources/scoping/member accesses/to class members/instance attributes/main.sdstest index f7699f7a9..4a5ddbcb2 100644 --- a/tests/resources/scoping/member accesses/to class members/instance attributes/main.sdstest +++ b/tests/resources/scoping/member accesses/to class members/instance attributes/main.sdstest @@ -87,6 +87,9 @@ pipeline myPipeline { // $TEST$ references declaredPreviouslyAsStaticMethod MyClass().»declaredPreviouslyAsStaticMethod«; + // $TEST$ references myInstanceAttribute + nullableMyClass().»myInstanceAttribute«; + // $TEST$ references myInstanceAttribute nullableMyClass()?.»myInstanceAttribute«; @@ -97,9 +100,6 @@ pipeline myPipeline { // $TEST$ unresolved AnotherClass().»myInstanceAttribute«; - // $TEST$ unresolved - nullableMyClass().»myInstanceAttribute«; - // $TEST$ unresolved unresolved.»myInstanceAttribute«; diff --git a/tests/resources/scoping/member accesses/to class members/instance methods/main.sdstest b/tests/resources/scoping/member accesses/to class members/instance methods/main.sdstest index 693bdbf49..109f228e3 100644 --- a/tests/resources/scoping/member accesses/to class members/instance methods/main.sdstest +++ b/tests/resources/scoping/member accesses/to class members/instance methods/main.sdstest @@ -87,6 +87,9 @@ pipeline myPipeline { // $TEST$ references declaredPreviouslyAsStaticMethod MyClass().»declaredPreviouslyAsStaticMethod«(); + // $TEST$ references myInstanceMethod + nullableMyClass().»myInstanceMethod«; + // $TEST$ references myInstanceMethod nullableMyClass()?.»myInstanceMethod«(); @@ -97,9 +100,6 @@ pipeline myPipeline { // $TEST$ unresolved AnotherClass().»myInstanceMethod«; - // $TEST$ unresolved - nullableMyClass().»myInstanceAttribute«; - // $TEST$ unresolved unresolved.»myInstanceMethod«; diff --git a/tests/resources/validation/other/expressions/member accesses/missing safe access/main.sdstest b/tests/resources/validation/other/expressions/member accesses/missing safe access/main.sdstest new file mode 100644 index 000000000..a16559d4c --- /dev/null +++ b/tests/resources/validation/other/expressions/member accesses/missing safe access/main.sdstest @@ -0,0 +1,40 @@ +package validation.other.expressions.memberAccesses.missingSafeAccess + +class MyClass() { + attr a: Int +} + +fun nullableMyClass() -> instance: MyClass? + +pipeline test { + // $TEST$ no error "The receiver can be null so a safe access must be used." + »1.a«(); + + // $TEST$ error "The receiver can be null so a safe access must be used." + »null.a«(); + + // $TEST$ no error "The receiver can be null so a safe access must be used." + »nullableMyClass().a«(); // This should be where the error is reported. Remove the "no" on the bug in Langium is fixed. + + // $TEST$ error "The receiver can be null so a safe access must be used." + nullableMyClass()».a«(); // Langium currently computes the wrong range for a member access + + // $TEST$ no error "The receiver can be null so a safe access must be used." + »unresolved.a«(); + + + // $TEST$ no error "The receiver can be null so a safe access must be used." + »1?.a«(); + + // $TEST$ no error "The receiver can be null so a safe access must be used." + »null?.a«(); + + // $TEST$ no error "The receiver can be null so a safe access must be used." + »nullableMyClass()?.a«(); + + // $TEST$ no error "The receiver can be null so a safe access must be used." + nullableMyClass()»?.a«(); // Langium currently computes the wrong range for a member access + + // $TEST$ no error "The receiver can be null so a safe access must be used." + »unresolved?.a«(); +} diff --git a/tests/resources/validation/style/unnecessary safe access/main.sdstest b/tests/resources/validation/style/unnecessary safe access/main.sdstest index 35154efb8..e84742fb0 100644 --- a/tests/resources/validation/style/unnecessary safe access/main.sdstest +++ b/tests/resources/validation/style/unnecessary safe access/main.sdstest @@ -1,22 +1,40 @@ package tests.validation.style.unnecessarySafeAccess -class MyClass +class MyClass { + attr a: Int +} fun nullableMyClass() -> result: MyClass? pipeline test { // $TEST$ info "The receiver is never null, so the safe access is unnecessary." - »1?.toString«(); + »1?.a«(); + + // $TEST$ no info "The receiver is never null, so the safe access is unnecessary." + »null?.a«(); + + // $TEST$ no info "The receiver is never null, so the safe access is unnecessary." + »nullableMyClass()?.a«(); + + // $TEST$ no info "The receiver is never null, so the safe access is unnecessary." + nullableMyClass()»?.a«(); // Langium currently computes the wrong range for a member access + + // $TEST$ no info "The receiver is never null, so the safe access is unnecessary." + »unresolved?.a«(); + + + // $TEST$ no info "The receiver is never null, so the safe access is unnecessary." + »1.a«(); // $TEST$ no info "The receiver is never null, so the safe access is unnecessary." - »null?.toString«(); + »null.a«(); // $TEST$ no info "The receiver is never null, so the safe access is unnecessary." - »nullableMyClass()?.toString«(); + »nullableMyClass().a«(); // $TEST$ no info "The receiver is never null, so the safe access is unnecessary." - nullableMyClass()»?.toString«(); // Langium currently computes the wrong range for a member access + nullableMyClass()».a«(); // Langium currently computes the wrong range for a member access // $TEST$ no info "The receiver is never null, so the safe access is unnecessary." - »unresolved?.toString«(); + »unresolved.a«(); }