Improve diagnostic message when short circuiting (issue 42599)

bwilkerson · commit-bot@chromium.org · commit d4ac7066b7ac · 2020-07-15T21:19:49.000Z
Change-Id: I456bc50792dfdb30c281578d0d54ffa9e2af6474 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/154161 Reviewed-by: Konstantin Shcheglov <scheglov@google.com> Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
diff --git a/pkg/analyzer/lib/error/error.dart b/pkg/analyzer/lib/error/error.dart
@@ -765,6 +765,7 @@ const List<ErrorCode> errorCodeValues = [
   StaticWarningCode.IMPORT_OF_NON_LIBRARY,
   StaticWarningCode.INSTANTIATE_ABSTRACT_CLASS,
   StaticWarningCode.INVALID_NULL_AWARE_OPERATOR,
+  StaticWarningCode.INVALID_NULL_AWARE_OPERATOR_AFTER_SHORT_CIRCUIT,
   StaticWarningCode.INVALID_OVERRIDE_DIFFERENT_DEFAULT_VALUES_NAMED,
   StaticWarningCode.INVALID_OVERRIDE_DIFFERENT_DEFAULT_VALUES_POSITIONAL,
   StaticWarningCode.INVALID_USE_OF_NULL_VALUE,
diff --git a/pkg/analyzer/lib/src/diagnostic/diagnostic_factory.dart b/pkg/analyzer/lib/src/diagnostic/diagnostic_factory.dart
@@ -3,6 +3,7 @@
 // BSD-style license that can be found in the LICENSE file.
 
 import 'package:analyzer/dart/ast/ast.dart';
+import 'package:analyzer/dart/ast/token.dart';
 import 'package:analyzer/dart/element/element.dart';
 import 'package:analyzer/diagnostic/diagnostic.dart';
 import 'package:analyzer/error/error.dart';
@@ -47,6 +48,25 @@ class DiagnosticFactory {
     ]);
   }
 
+  /// Return a diagnostic indicating that the [duplicateKey] (in a constant map)
+  /// is a duplicate of the [originalKey].
+  AnalysisError invalidNullAwareAfterShortCircuit(Source source, int offset,
+      int length, List<Object> arguments, Token previousToken) {
+    var lexeme = previousToken.lexeme;
+    return AnalysisError(
+        source,
+        offset,
+        length,
+        StaticWarningCode.INVALID_NULL_AWARE_OPERATOR_AFTER_SHORT_CIRCUIT,
+        arguments, [
+      DiagnosticMessageImpl(
+          filePath: source.fullName,
+          message: "The operator '$lexeme' is causing the short circuiting.",
+          offset: previousToken.offset,
+          length: previousToken.length)
+    ]);
+  }
+
   /// Return a diagnostic indicating that the given [identifier] was referenced
   /// before it was declared.
   AnalysisError referencedBeforeDeclaration(
diff --git a/pkg/analyzer/lib/src/error/codes.dart b/pkg/analyzer/lib/src/error/codes.dart
@@ -9343,6 +9343,22 @@ class StaticWarningCode extends AnalyzerErrorCode {
           errorSeverity: ErrorSeverity.WARNING,
           hasPublishedDocs: true);
 
+  /**
+   * Parameters:
+   * 0: The null-aware operator that is invalid
+   * 1: The non-null-aware operator that can replace the invalid operator
+   */
+  static const StaticWarningCode
+      INVALID_NULL_AWARE_OPERATOR_AFTER_SHORT_CIRCUIT =
+      StaticWarningCodeWithUniqueName(
+          'INVALID_NULL_AWARE_OPERATOR',
+          'INVALID_NULL_AWARE_OPERATOR_AFTER_SHORT_CIRCUIT',
+          "The target expression can't be null because of short-circuiting, so "
+              "the null-aware operator '{0}' can't be used.",
+          correction: "Try replace the operator '{0}' with '{1}'.",
+          errorSeverity: ErrorSeverity.WARNING,
+          hasPublishedDocs: true);
+
   /**
    * 7.1 Instance Methods: It is a static warning if an instance method
    * <i>m1</i> overrides an instance member <i>m2</i>, the signature of
@@ -10632,10 +10648,12 @@ class StaticWarningCodeWithUniqueName extends StaticWarningCode {
   const StaticWarningCodeWithUniqueName(
       String name, this.uniqueName, String message,
       {String correction,
+      ErrorSeverity errorSeverity = ErrorSeverity.ERROR,
       bool hasPublishedDocs,
       bool isUnresolvedIdentifier = false})
       : super(name, message,
             correction: correction,
+            errorSeverity: errorSeverity,
             hasPublishedDocs: hasPublishedDocs,
             isUnresolvedIdentifier: isUnresolvedIdentifier);
 }
diff --git a/pkg/analyzer/lib/src/generated/error_verifier.dart b/pkg/analyzer/lib/src/generated/error_verifier.dart
@@ -4203,7 +4203,54 @@ class ErrorVerifier extends RecursiveAstVisitor<void> {
       return;
     }
 
+    /// If the operator is not valid because the target already makes use of a
+    /// null aware operator, return the null aware operator from the target.
+    Token previousShortCircuitingOperator(Expression target) {
+      if (target is PropertyAccess) {
+        var operator = target.operator;
+        var type = operator.type;
+        if (type == TokenType.QUESTION_PERIOD) {
+          var realTarget = target.realTarget;
+          if (_isExpressionWithType(realTarget)) {
+            return previousShortCircuitingOperator(realTarget) ?? operator;
+          }
+        }
+      } else if (target is IndexExpression) {
+        if (target.question != null) {
+          var realTarget = target.realTarget;
+          if (_isExpressionWithType(realTarget)) {
+            return previousShortCircuitingOperator(realTarget) ??
+                target.question;
+          }
+        }
+      } else if (target is MethodInvocation) {
+        var operator = target.operator;
+        var type = operator.type;
+        if (type == TokenType.QUESTION_PERIOD) {
+          var realTarget = target.realTarget;
+          if (_isExpressionWithType(realTarget)) {
+            return previousShortCircuitingOperator(realTarget) ?? operator;
+          }
+          return operator;
+        }
+      }
+      return null;
+    }
+
     if (_typeSystem.isStrictlyNonNullable(target.staticType)) {
+      if (errorCode == StaticWarningCode.INVALID_NULL_AWARE_OPERATOR) {
+        var previousOperator = previousShortCircuitingOperator(target);
+        if (previousOperator != null) {
+          _errorReporter.reportError(DiagnosticFactory()
+              .invalidNullAwareAfterShortCircuit(
+                  _errorReporter.source,
+                  operator.offset,
+                  endToken.end - operator.offset,
+                  arguments,
+                  previousOperator));
+          return;
+        }
+      }
       _errorReporter.reportErrorForOffset(
         errorCode,
         operator.offset,
diff --git a/pkg/analyzer/test/src/diagnostics/equal_elements_in_const_set_test.dart b/pkg/analyzer/test/src/diagnostics/equal_elements_in_const_set_test.dart
@@ -24,9 +24,7 @@ class EqualElementsInConstSetTest extends DriverResolutionTest {
 var c = const {1, 2, 1};
 ''', [
       error(CompileTimeErrorCode.EQUAL_ELEMENTS_IN_CONST_SET, 21, 1,
-          contextMessages: [
-            message(resourceProvider.convertPath('/test/lib/test.dart'), 15, 1)
-          ]),
+          contextMessages: [message('/test/lib/test.dart', 15, 1)]),
     ]);
   }
 
@@ -38,12 +36,7 @@ var c = const {1, if (1 < 0) 2 else 1};
         analysisOptions.experimentStatus.constant_update_2018
             ? [
                 error(CompileTimeErrorCode.EQUAL_ELEMENTS_IN_CONST_SET, 36, 1,
-                    contextMessages: [
-                      message(
-                          resourceProvider.convertPath('/test/lib/test.dart'),
-                          15,
-                          1)
-                    ]),
+                    contextMessages: [message('/test/lib/test.dart', 15, 1)]),
               ]
             : [
                 error(CompileTimeErrorCode.NON_CONSTANT_SET_ELEMENT, 18, 19),
@@ -106,12 +99,7 @@ var c = const {1, if (0 < 1) 1};
         analysisOptions.experimentStatus.constant_update_2018
             ? [
                 error(CompileTimeErrorCode.EQUAL_ELEMENTS_IN_CONST_SET, 29, 1,
-                    contextMessages: [
-                      message(
-                          resourceProvider.convertPath('/test/lib/test.dart'),
-                          15,
-                          1)
-                    ]),
+                    contextMessages: [message('/test/lib/test.dart', 15, 1)]),
               ]
             : [
                 error(CompileTimeErrorCode.NON_CONSTANT_SET_ELEMENT, 18, 12),
@@ -127,9 +115,7 @@ class A<T> {
 var c = const {const A<int>(), const A<int>()};
 ''', [
       error(CompileTimeErrorCode.EQUAL_ELEMENTS_IN_CONST_SET, 60, 14,
-          contextMessages: [
-            message(resourceProvider.convertPath('/test/lib/test.dart'), 44, 14)
-          ]),
+          contextMessages: [message('/test/lib/test.dart', 44, 14)]),
     ]);
   }
 
@@ -164,12 +150,7 @@ var c = const {1, ...{1}};
         analysisOptions.experimentStatus.constant_update_2018
             ? [
                 error(CompileTimeErrorCode.EQUAL_ELEMENTS_IN_CONST_SET, 21, 3,
-                    contextMessages: [
-                      message(
-                          resourceProvider.convertPath('/test/lib/test.dart'),
-                          15,
-                          1)
-                    ]),
+                    contextMessages: [message('/test/lib/test.dart', 15, 1)]),
               ]
             : [
                 error(CompileTimeErrorCode.NON_CONSTANT_SET_ELEMENT, 18, 6),
diff --git a/pkg/analyzer/test/src/diagnostics/invalid_null_aware_operator_test.dart b/pkg/analyzer/test/src/diagnostics/invalid_null_aware_operator_test.dart
@@ -12,10 +12,72 @@ import '../dart/resolution/driver_resolution.dart';
 
 main() {
   defineReflectiveSuite(() {
+    defineReflectiveTests(InvalidNullAwareOperatorAfterShortCircuitTest);
     defineReflectiveTests(InvalidNullAwareOperatorTest);
   });
 }
 
+@reflectiveTest
+class InvalidNullAwareOperatorAfterShortCircuitTest
+    extends DriverResolutionTest {
+  @override
+  AnalysisOptionsImpl get analysisOptions => AnalysisOptionsImpl()
+    ..contextFeatures = FeatureSet.fromEnableFlags(
+      [EnableString.non_nullable],
+    );
+
+  Future<void> test_getter_previousTarget() async {
+    await assertErrorsInCode('''
+void f(String? s) {
+  s?.length?.isEven;
+}
+''', [
+      error(StaticWarningCode.INVALID_NULL_AWARE_OPERATOR_AFTER_SHORT_CIRCUIT,
+          31, 2,
+          contextMessages: [message('/test/lib/test.dart', 23, 2)]),
+    ]);
+  }
+
+  Future<void> test_index_previousTarget() async {
+    await assertErrorsInCode('''
+void f(String? s) {
+  s?[4]?.length;
+}
+''', [
+      error(StaticWarningCode.INVALID_NULL_AWARE_OPERATOR_AFTER_SHORT_CIRCUIT,
+          27, 2,
+          contextMessages: [message('/test/lib/test.dart', 23, 1)]),
+    ]);
+  }
+
+  Future<void> test_methodInvocation_previousTarget() async {
+    await assertErrorsInCode('''
+void f(String? s) {
+  s?.substring(0, 5)?.length;
+}
+''', [
+      error(StaticWarningCode.INVALID_NULL_AWARE_OPERATOR_AFTER_SHORT_CIRCUIT,
+          40, 2,
+          contextMessages: [message('/test/lib/test.dart', 23, 2)]),
+    ]);
+  }
+
+  Future<void> test_methodInvocation_previousTwoTargets() async {
+    await assertErrorsInCode('''
+void f(String? s) {
+  s?.substring(0, 5)?.toLowerCase()?.length;
+}
+''', [
+      error(StaticWarningCode.INVALID_NULL_AWARE_OPERATOR_AFTER_SHORT_CIRCUIT,
+          40, 2,
+          contextMessages: [message('/test/lib/test.dart', 23, 2)]),
+      error(StaticWarningCode.INVALID_NULL_AWARE_OPERATOR_AFTER_SHORT_CIRCUIT,
+          55, 2,
+          contextMessages: [message('/test/lib/test.dart', 23, 2)]),
+    ]);
+  }
+}
+
 @reflectiveTest
 class InvalidNullAwareOperatorTest extends DriverResolutionTest {
   @override
diff --git a/pkg/analyzer/tool/diagnostics/diagnostics.md b/pkg/analyzer/tool/diagnostics/diagnostics.md
@@ -3439,6 +3439,9 @@ var x;
 
 ### invalid_null_aware_operator
 
+_The target expression can't be null because of short-circuiting, so the
+null-aware operator '{0}' can't be used._
+
 _The target expression can't be null, so the null-aware operator '{0}' can't be
 used._
 
