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 <[email protected]>
Safe-DS · Oct 10, 2023 · 077daff · 077daff
1 parent e77037e
commit 077daff
Show file tree

Hide file tree

Showing 7 changed files with 101 additions and 16 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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/...s/resources/validation/other/expressions/member accesses/missing safe access/main.sdstest b/...s/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
@@ -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«();
 }