Include the deprecation message in diagnostics (issue 38667)

Change-Id: Ib1fa24fa4b112b9675d1f1900cbdda15319080a5
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/119780
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>

Author: bwilkerson

pkg/analyzer/lib/error/error.dart

@@ -325,6 +325,8 @@ const List<ErrorCode> errorCodeValues = const [
   HintCode.DEPRECATED_FUNCTION_CLASS_DECLARATION,
   HintCode.DEPRECATED_MEMBER_USE,
   HintCode.DEPRECATED_MEMBER_USE_FROM_SAME_PACKAGE,
+  HintCode.DEPRECATED_MEMBER_USE_FROM_SAME_PACKAGE_WITH_MESSAGE,
+  HintCode.DEPRECATED_MEMBER_USE_WITH_MESSAGE,
   HintCode.DEPRECATED_MIXIN_FUNCTION,
   HintCode.DIVISION_OPTIMIZATION,
   HintCode.DUPLICATE_IMPORT,

pkg/analyzer/lib/src/dart/error/hint_codes.dart

@@ -132,6 +132,34 @@ class HintCode extends AnalyzerErrorCode {
               "replacement.",
           hasPublishedDocs: true);
 
+  /**
+   * Parameters:
+   * 0: the name of the member
+   * 1: message details
+   */
+  static const HintCode DEPRECATED_MEMBER_USE_FROM_SAME_PACKAGE_WITH_MESSAGE =
+      const HintCodeWithUniqueName(
+          'DEPRECATED_MEMBER_USE_FROM_SAME_PACKAGE',
+          'HintCode.DEPRECATED_MEMBER_USE_FROM_SAME_PACKAGE_WITH_MESSAGE',
+          "'{0}' is deprecated and shouldn't be used. {1}.",
+          correction: "Try replacing the use of the deprecated member with the "
+              "replacement.",
+          hasPublishedDocs: true);
+
+  /**
+   * Parameters:
+   * 0: the name of the member
+   * 1: message details
+   */
+  static const HintCode DEPRECATED_MEMBER_USE_WITH_MESSAGE =
+      const HintCodeWithUniqueName(
+          'DEPRECATED_MEMBER_USE',
+          'HintCode.DEPRECATED_MEMBER_USE_WITH_MESSAGE',
+          "'{0}' is deprecated and shouldn't be used. {1}.",
+          correction: "Try replacing the use of the deprecated member with the "
+              "replacement.",
+          hasPublishedDocs: true);
+
   /**
    * `Function` should not be mixed in anymore.
    */
@@ -785,7 +813,7 @@ class HintCode extends AnalyzerErrorCode {
   // #### Common fixes
   //
   // If you don't need to support older versions of the SDK, then you can
-  // ncrease the SDK constraint to allow the expression to be used:
+  // increase the SDK constraint to allow the expression to be used:
   //
   // ```yaml
   // environment:
@@ -1584,3 +1612,13 @@ class HintCode extends AnalyzerErrorCode {
   @override
   ErrorType get type => ErrorType.HINT;
 }
+
+class HintCodeWithUniqueName extends HintCode {
+  @override
+  final String uniqueName;
+
+  const HintCodeWithUniqueName(String name, this.uniqueName, String message,
+      {String correction, bool hasPublishedDocs})
+      : super(name, message,
+            correction: correction, hasPublishedDocs: hasPublishedDocs);
+}

pkg/analyzer/lib/src/generated/resolver.dart

@@ -585,12 +585,9 @@ class BestPracticesVerifier extends RecursiveAstVisitor<void> {
     return false;
   }
 
-  /// Given some [Element], look at the associated metadata and report the use
-  /// of the member if it is declared as deprecated.
-  ///
-  /// @param element some element to check for deprecated use of
-  /// @param node the node use for the location of the error
-  /// See [HintCode.DEPRECATED_MEMBER_USE].
+  /// Given some [element], look at the associated metadata and report the use
+  /// of the member if it is declared as deprecated. If a diagnostic is reported
+  /// it should be reported at the given [node].
   void _checkForDeprecatedMemberUse(Element element, AstNode node) {
     bool isDeprecated(Element element) {
       if (element is PropertyAccessorElement && element.isSynthetic) {
@@ -650,10 +647,19 @@ class BestPracticesVerifier extends RecursiveAstVisitor<void> {
       }
       LibraryElement library =
           element is LibraryElement ? element : element.library;
-      HintCode hintCode = _isLibraryInWorkspacePackage(library)
-          ? HintCode.DEPRECATED_MEMBER_USE_FROM_SAME_PACKAGE
-          : HintCode.DEPRECATED_MEMBER_USE;
-      _errorReporter.reportErrorForNode(hintCode, node, [displayName]);
+      String message = _deprecatedMessage(element);
+      if (message == null || message.isEmpty) {
+        HintCode hintCode = _isLibraryInWorkspacePackage(library)
+            ? HintCode.DEPRECATED_MEMBER_USE_FROM_SAME_PACKAGE
+            : HintCode.DEPRECATED_MEMBER_USE;
+        _errorReporter.reportErrorForNode(hintCode, node, [displayName]);
+      } else {
+        HintCode hintCode = _isLibraryInWorkspacePackage(library)
+            ? HintCode.DEPRECATED_MEMBER_USE_FROM_SAME_PACKAGE_WITH_MESSAGE
+            : HintCode.DEPRECATED_MEMBER_USE_WITH_MESSAGE;
+        _errorReporter
+            .reportErrorForNode(hintCode, node, [displayName, message]);
+      }
     }
   }
 
@@ -1234,6 +1240,22 @@ class BestPracticesVerifier extends RecursiveAstVisitor<void> {
     return _workspacePackage.contains(library.source);
   }
 
+  /// Return the message in the deprecated annotation on the given [element], or
+  /// `null` if the element doesn't have a deprecated annotation or if the
+  /// annotation does not have a message.
+  static String _deprecatedMessage(Element element) {
+    ElementAnnotationImpl annotation = element.metadata.firstWhere(
+      (e) => e.isDeprecated,
+      orElse: () => null,
+    );
+    if (annotation == null || annotation.element is PropertyAccessorElement) {
+      return null;
+    }
+    DartObject constantValue = annotation.computeConstantValue();
+    return constantValue?.getField('message')?.toStringValue() ??
+        constantValue?.getField('expires')?.toStringValue();
+  }
+
   /// Check for the passed class declaration for the
   /// [HintCode.OVERRIDE_EQUALS_BUT_NOT_HASH_CODE] hint code.
   ///

pkg/analyzer/test/src/diagnostics/deprecated_member_use_test.dart

@@ -306,7 +306,7 @@ class A {
   m() {}
   n() {m();}
 }
-''', [HintCode.DEPRECATED_MEMBER_USE_FROM_SAME_PACKAGE]);
+''', [HintCode.DEPRECATED_MEMBER_USE_FROM_SAME_PACKAGE_WITH_MESSAGE]);
   }
 
   test_operator() async {
@@ -614,7 +614,8 @@ class A {
     await assertErrorsInCode(r'''
 import 'package:foo/foo.dart';
 void main() => A().m();
-''', [HintCode.DEPRECATED_MEMBER_USE], sourceName: '/pkg1/lib/lib1.dart');
+''', [HintCode.DEPRECATED_MEMBER_USE_WITH_MESSAGE],
+        sourceName: '/pkg1/lib/lib1.dart');
   }
 
   test_packageBuildWorkspace() async {

pkg/analyzer/test/src/task/options_test.dart

@@ -11,7 +11,6 @@ import 'package:analyzer/source/error_processor.dart';
 import 'package:analyzer/src/analysis_options/analysis_options_provider.dart';
 import 'package:analyzer/src/analysis_options/error/option_codes.dart';
 import 'package:analyzer/src/dart/error/hint_codes.dart';
-import 'package:analyzer/src/dart/error/syntactic_errors.dart';
 import 'package:analyzer/src/error/codes.dart';
 import 'package:analyzer/src/file_system/file_system.dart';
 import 'package:analyzer/src/generated/engine.dart';
@@ -149,14 +148,21 @@ analyzer:
 @reflectiveTest
 class ErrorCodeValuesTest {
   test_errorCodes() {
+    // Now that we're using unique names for comparison, the only reason to
+    // split the codes by class is to find all of the classes that need to be
+    // checked against `errorCodeValues`.
     var errorTypeMap = <Type, List<ErrorCode>>{};
     for (ErrorCode code in errorCodeValues) {
-      errorTypeMap.putIfAbsent(code.runtimeType, () => <ErrorCode>[]).add(code);
+      Type type = code.runtimeType;
+      if (type == HintCodeWithUniqueName) {
+        type = HintCode;
+      }
+      errorTypeMap.putIfAbsent(type, () => <ErrorCode>[]).add(code);
     }
 
-    int missingErrorCodeCount = 0;
+    StringBuffer missingCodes = StringBuffer();
     errorTypeMap.forEach((Type errorType, List<ErrorCode> codes) {
-      var listedNames = codes.map((ErrorCode code) => code.name).toSet();
+      var listedNames = codes.map((ErrorCode code) => code.uniqueName).toSet();
 
       var declaredNames = reflectClass(errorType)
           .declarations
@@ -165,115 +171,23 @@ class ErrorCodeValuesTest {
         String name = declarationMirror.simpleName.toString();
         //TODO(danrubel): find a better way to extract the text from the symbol
         assert(name.startsWith('Symbol("') && name.endsWith('")'));
-        return name.substring(8, name.length - 2);
+        return errorType.toString() + '.' + name.substring(8, name.length - 2);
       }).where((String name) {
         return name == name.toUpperCase();
       }).toList();
 
-      // Remove declared names that are not supposed to be in errorCodeValues
-
-      if (errorType == AnalysisOptionsErrorCode) {
-        declaredNames
-            .remove(AnalysisOptionsErrorCode.INCLUDED_FILE_PARSE_ERROR.name);
-      } else if (errorType == AnalysisOptionsWarningCode) {
-        declaredNames
-            .remove(AnalysisOptionsWarningCode.INCLUDE_FILE_NOT_FOUND.name);
-        declaredNames
-            .remove(AnalysisOptionsWarningCode.INCLUDED_FILE_WARNING.name);
-        declaredNames
-            .remove(AnalysisOptionsWarningCode.INVALID_SECTION_FORMAT.name);
-      } else if (errorType == StaticWarningCode) {
-        declaredNames.remove(
-            StaticWarningCode.FINAL_NOT_INITIALIZED_CONSTRUCTOR_3_PLUS.name +
-                '_PLUS');
-        declaredNames.remove('EXTRA_POSITIONAL_ARGUMENTS');
-        declaredNames.remove('EXTRA_POSITIONAL_ARGUMENTS_COULD_BE_NAMED');
-        declaredNames.remove('IMPORT_OF_NON_LIBRARY');
-        declaredNames.remove('NOT_ENOUGH_REQUIRED_ARGUMENTS');
-        declaredNames.remove('REDIRECT_TO_MISSING_CONSTRUCTOR');
-        declaredNames.remove('REDIRECT_TO_NON_CLASS');
-        declaredNames.remove('UNDEFINED_CLASS');
-        declaredNames.remove('UNDEFINED_NAMED_PARAMETER');
-      } else if (errorType == StrongModeCode) {
-        void removeCode(StrongModeCode code) {
-          declaredNames.remove(code.name);
-        }
-
-        removeCode(StrongModeCode.DOWN_CAST_COMPOSITE);
-        removeCode(StrongModeCode.DOWN_CAST_IMPLICIT);
-        removeCode(StrongModeCode.DOWN_CAST_IMPLICIT_ASSIGN);
-        removeCode(StrongModeCode.DYNAMIC_CAST);
-        removeCode(StrongModeCode.ASSIGNMENT_CAST);
-        removeCode(StrongModeCode.INVALID_PARAMETER_DECLARATION);
-        removeCode(StrongModeCode.COULD_NOT_INFER);
-        removeCode(StrongModeCode.INFERRED_TYPE);
-        removeCode(StrongModeCode.INFERRED_TYPE_LITERAL);
-        removeCode(StrongModeCode.INFERRED_TYPE_ALLOCATION);
-        removeCode(StrongModeCode.INFERRED_TYPE_CLOSURE);
-        removeCode(StrongModeCode.INVALID_CAST_LITERAL);
-        removeCode(StrongModeCode.INVALID_CAST_LITERAL_LIST);
-        removeCode(StrongModeCode.INVALID_CAST_LITERAL_MAP);
-        removeCode(StrongModeCode.INVALID_CAST_LITERAL_SET);
-        removeCode(StrongModeCode.INVALID_CAST_FUNCTION_EXPR);
-        removeCode(StrongModeCode.INVALID_CAST_NEW_EXPR);
-        removeCode(StrongModeCode.INVALID_CAST_METHOD);
-        removeCode(StrongModeCode.INVALID_CAST_FUNCTION);
-        removeCode(StrongModeCode.INVALID_SUPER_INVOCATION);
-        removeCode(StrongModeCode.NON_GROUND_TYPE_CHECK_INFO);
-        removeCode(StrongModeCode.DYNAMIC_INVOKE);
-        removeCode(StrongModeCode.IMPLICIT_DYNAMIC_PARAMETER);
-        removeCode(StrongModeCode.IMPLICIT_DYNAMIC_RETURN);
-        removeCode(StrongModeCode.IMPLICIT_DYNAMIC_VARIABLE);
-        removeCode(StrongModeCode.IMPLICIT_DYNAMIC_FIELD);
-        removeCode(StrongModeCode.IMPLICIT_DYNAMIC_TYPE);
-        removeCode(StrongModeCode.IMPLICIT_DYNAMIC_LIST_LITERAL);
-        removeCode(StrongModeCode.IMPLICIT_DYNAMIC_MAP_LITERAL);
-        removeCode(StrongModeCode.IMPLICIT_DYNAMIC_FUNCTION);
-        removeCode(StrongModeCode.IMPLICIT_DYNAMIC_METHOD);
-        removeCode(StrongModeCode.IMPLICIT_DYNAMIC_INVOKE);
-        removeCode(StrongModeCode.NOT_INSTANTIATED_BOUND);
-        removeCode(StrongModeCode.TOP_LEVEL_CYCLE);
-        removeCode(StrongModeCode.TOP_LEVEL_FUNCTION_LITERAL_BLOCK);
-        removeCode(StrongModeCode.TOP_LEVEL_IDENTIFIER_NO_TYPE);
-        removeCode(StrongModeCode.TOP_LEVEL_INSTANCE_GETTER);
-        removeCode(StrongModeCode.TOP_LEVEL_INSTANCE_METHOD);
-      } else if (errorType == TodoCode) {
-        declaredNames.remove('TODO_REGEX');
-      } else if (errorType == CompileTimeErrorCode) {
-        declaredNames.remove('ANNOTATION_WITH_TYPE_ARGUMENTS');
-        declaredNames.remove('NOT_ENOUGH_REQUIRED_ARGUMENTS');
-      } else if (errorType == ParserErrorCode) {
-        declaredNames.remove('CONST_AFTER_FACTORY');
-        declaredNames.remove('CONST_AND_COVARIANT');
-        declaredNames.remove('CONST_AND_VAR');
-        declaredNames.remove('COVARIANT_AFTER_FINAL');
-        declaredNames.remove('COVARIANT_AFTER_VAR');
-        declaredNames.remove('EXTERNAL_AFTER_CONST');
-        declaredNames.remove('EXTERNAL_AFTER_FACTORY');
-        declaredNames.remove('EXTERNAL_AFTER_STATIC');
-        declaredNames.remove('MISSING_CLASS_BODY');
-        declaredNames.remove('STATIC_AFTER_CONST');
-        declaredNames.remove('STATIC_AFTER_FINAL');
-        declaredNames.remove('STATIC_AFTER_VAR');
-      }
-
-      // Assert that all remaining declared names are in errorCodeValues
+      // Assert that all declared names are in errorCodeValues
 
       for (String declaredName in declaredNames) {
         if (!listedNames.contains(declaredName)) {
-          ++missingErrorCodeCount;
-          print('   errorCodeValues is missing $errorType $declaredName');
+          missingCodes.writeln();
+          missingCodes.write('  $declaredName');
         }
       }
     });
-    expect(missingErrorCodeCount, 0, reason: 'missing error code names');
-
-    // Apparently, duplicate error codes are allowed
-    //    expect(
-    //      ErrorFilterOptionValidator.errorCodes.length,
-    //      errorCodeValues.length,
-    //      reason: 'some errorCodeValues have the same name',
-    //    );
+    if (missingCodes.isNotEmpty) {
+      fail('Missing error codes:$missingCodes');
+    }
   }
 }
 

pkg/analyzer/tool/diagnostics/diagnostics.md

@@ -1937,7 +1937,7 @@ const int i = [!n as int!];
 #### Common fixes
 
 If you don't need to support older versions of the SDK, then you can
-ncrease the SDK constraint to allow the expression to be used:
+increase the SDK constraint to allow the expression to be used:
 
 ```yaml
 environment: