[lint] new lint: unnecessary_ignore

pq · Commit Queue · commit 2dc6d31f8a6a · 2025-01-15T10:21:42.000-08:00
See: #35234 As a follow-up, I'll update the reporting to skip generated files. Change-Id: Icddcdf59269d82576eefd810671a3e992f2bcca5 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/404480 Reviewed-by: Brian Wilkerson <brianwilkerson@google.com> Commit-Queue: Phil Quitslund <pquitslund@google.com>
diff --git a/pkg/analysis_server/lib/src/services/correction/error_fix_status.yaml b/pkg/analysis_server/lib/src/services/correction/error_fix_status.yaml
@@ -2421,6 +2421,10 @@ LintCode.unnecessary_final_without_type:
   status: hasFix
 LintCode.unnecessary_getters_setters:
   status: hasFix
+LintCode.unnecessary_ignore:
+  status: needsFix
+  notes: |-
+    Remove the ignore comment (or one code in the comment).
 LintCode.unnecessary_lambdas:
   status: hasFix
 LintCode.unnecessary_late:
@@ -3751,10 +3755,6 @@ WarningCode.UNNECESSARY_CAST:
   status: hasFix
 WarningCode.UNNECESSARY_FINAL:
   status: hasFix
-WarningCode.UNNECESSARY_IGNORE:
-  status: needsFix
-  notes: |-
-    Remove the ignore comment (or one code in the comment).
 WarningCode.UNNECESSARY_NAN_COMPARISON_FALSE:
   status: hasFix
   notes: |-
diff --git a/pkg/analyzer/lib/src/dart/analysis/library_analyzer.dart b/pkg/analyzer/lib/src/dart/analysis/library_analyzer.dart
@@ -350,16 +350,20 @@ class LibraryAnalyzer {
 
     _checkForInconsistentLanguageVersionOverride();
 
+    var validateUnnecessaryIgnores =
+        _analysisOptions.isLintEnabled('unnecessary_ignore');
+
     // This must happen after all other diagnostics have been computed but
     // before the list of diagnostics has been filtered.
     for (var fileAnalysis in _libraryFiles.values) {
       IgnoreValidator(
-        fileAnalysis.errorReporter,
-        fileAnalysis.errorListener.errors,
-        fileAnalysis.ignoreInfo,
-        fileAnalysis.unit.lineInfo,
-        _analysisOptions.unignorableNames,
-      ).reportErrors();
+              fileAnalysis.errorReporter,
+              fileAnalysis.errorListener.errors,
+              fileAnalysis.ignoreInfo,
+              fileAnalysis.unit.lineInfo,
+              _analysisOptions.unignorableNames,
+              validateUnnecessaryIgnores)
+          .reportErrors();
     }
   }
 
diff --git a/pkg/analyzer/lib/src/error/codes.g.dart b/pkg/analyzer/lib/src/error/codes.g.dart
@@ -7467,17 +7467,6 @@ class WarningCode extends ErrorCode {
     hasPublishedDocs: true,
   );
 
-  ///  Parameters:
-  ///  0: the name of the diagnostic being ignored
-  static const WarningCode UNNECESSARY_IGNORE = WarningCode(
-    'UNNECESSARY_IGNORE',
-    "The diagnostic '{0}' isn't produced at this location so it doesn't need "
-        "to be ignored.",
-    correctionMessage:
-        "Try removing the name from the list, or removing the whole comment if "
-        "this is the only name in the list.",
-  );
-
   ///  No parameters.
   static const WarningCode UNNECESSARY_NAN_COMPARISON_FALSE = WarningCode(
     'UNNECESSARY_NAN_COMPARISON',
diff --git a/pkg/analyzer/lib/src/error/error_code_values.g.dart b/pkg/analyzer/lib/src/error/error_code_values.g.dart
@@ -1090,7 +1090,6 @@ const List<ErrorCode> errorCodeValues = [
   WarningCode.UNNECESSARY_CAST,
   WarningCode.UNNECESSARY_CAST_PATTERN,
   WarningCode.UNNECESSARY_FINAL,
-  WarningCode.UNNECESSARY_IGNORE,
   WarningCode.UNNECESSARY_NAN_COMPARISON_FALSE,
   WarningCode.UNNECESSARY_NAN_COMPARISON_TRUE,
   WarningCode.UNNECESSARY_NO_SUCH_METHOD,
diff --git a/pkg/analyzer/lib/src/error/ignore_validator.dart b/pkg/analyzer/lib/src/error/ignore_validator.dart
@@ -7,9 +7,21 @@ import 'package:analyzer/error/listener.dart';
 import 'package:analyzer/source/line_info.dart';
 import 'package:analyzer/src/error/codes.dart';
 import 'package:analyzer/src/ignore_comments/ignore_info.dart';
+import 'package:analyzer/src/lint/registry.dart';
+import 'package:analyzer/src/lint/state.dart';
 
 /// Used to validate the ignore comments in a single file.
 class IgnoreValidator {
+  /// A list of known error codes used to ensure we don't over-report
+  /// `unnecessary_ignore`s on error codes that may be contributed by a plugin.
+  static final Set<String> _validErrorCodeNames =
+      errorCodeValues.map((e) => e.name.toLowerCase()).toSet();
+
+  /// The error code used to report `unnecessary_ignore`s.
+  /// This code is set when the `UnnecessaryIgnore` lint rule is instantiated and
+  /// registered by the linter.
+  static late ErrorCode unnecessaryIgnoreLintCode;
+
   /// The error reporter to which errors are to be reported.
   final ErrorReporter _errorReporter;
 
@@ -28,11 +40,14 @@ class IgnoreValidator {
   /// use because we have no visibility of them here.
   final Set<String> _unignorableNames;
 
+  /// Whether to validate unnecessary ignores (enabled by the `unnecessary_ignore` lint).
+  final bool _validateUnnecessaryIgnores;
+
   /// Initialize a newly created validator to report any issues with ignore
   /// comments in the file being analyzed. The diagnostics will be reported to
   /// the [_errorReporter].
   IgnoreValidator(this._errorReporter, this._reportedErrors, this._ignoreInfo,
-      this._lineInfo, this._unignorableNames);
+      this._lineInfo, this._unignorableNames, this._validateUnnecessaryIgnores);
 
   /// Report any issues with ignore comments in the file being analyzed.
   void reportErrors() {
@@ -124,7 +139,7 @@ class IgnoreValidator {
     //         errorCode: WarningCode.UNIGNORABLE_IGNORE,
     //         offset: unignorableName.offset,
     //         length: name.length,
-    //         data: [name]);
+    //         arguments: [name]);
     //     list.remove(unignorableName);
     //   }
     // }
@@ -153,46 +168,49 @@ class IgnoreValidator {
   /// Report the [ignoredNames] as being unnecessary.
   void _reportUnnecessaryOrRemovedOrDeprecatedIgnores(
       List<IgnoredElement> ignoredNames) {
-    // TODO(pq): find the right way to roll-out enablement and uncomment
-    // https://github.com/dart-lang/sdk/issues/51214
-    // for (var ignoredName in ignoredNames) {
-    //   if (ignoredName is IgnoredDiagnosticName) {
-    //     var name = ignoredName.name;
-    //     var rule = Registry.ruleRegistry.getRule(name);
-    //     if (rule != null) {
-    //       var state = rule.state;
-    //       var since = state.since.toString();
-    //       if (state is DeprecatedState) {
-    //         // `todo`(pq): implement
-    //       } else if (state is RemovedState) {
-    //         var replacedBy = state.replacedBy;
-    //         if (replacedBy != null) {
-    //           _errorReporter.atOffset(
-    //               errorCode: WarningCode.REPLACED_LINT_USE,
-    //               offset: ignoredName.offset,
-    //               length: name.length,
-    //               data: [name, since, replacedBy]);
-    //           continue;
-    //         } else {
-    //           _errorReporter.atOffset(
-    //               errorCode: WarningCode.REMOVED_LINT_USE,
-    //               offset: ignoredName.offset,
-    //               length: name.length,
-    //               data: [name, since]);
-    //           continue;
-    //         }
-    //       }
-    //     }
-
-    //     // TODO(brianwilkerson): Uncomment the code below after the unnecessary
-    //     // ignores in the Flutter code base have been cleaned up.
-    //     _errorReporter.atOffset(
-    //         errorCode: WarningCode.UNNECESSARY_IGNORE,
-    //         offset: ignoredName.offset,
-    //         length: name.length,
-    //         data: [name]);
-    //   }
-    // }
+    if (!_validateUnnecessaryIgnores) return;
+
+    for (var ignoredName in ignoredNames) {
+      if (ignoredName is IgnoredDiagnosticName) {
+        var name = ignoredName.name;
+        var rule = Registry.ruleRegistry.getRule(name);
+        if (rule == null) {
+          // If a code is not a lint or a recognized error,
+          // don't report. (It could come from a plugin.)
+          // TODO(pq): consider another diagnostic that reports undefined codes
+          if (!_validErrorCodeNames.contains(name.toLowerCase())) continue;
+        } else {
+          var state = rule.state;
+          var since = state.since.toString();
+          if (state is DeprecatedState) {
+            // `todo`(pq): implement
+          } else if (state is RemovedState) {
+            var replacedBy = state.replacedBy;
+            if (replacedBy != null) {
+              _errorReporter.atOffset(
+                  errorCode: WarningCode.REPLACED_LINT_USE,
+                  offset: ignoredName.offset,
+                  length: name.length,
+                  arguments: [name, since, replacedBy]);
+              continue;
+            } else {
+              _errorReporter.atOffset(
+                  errorCode: WarningCode.REMOVED_LINT_USE,
+                  offset: ignoredName.offset,
+                  length: name.length,
+                  arguments: [name, since]);
+              continue;
+            }
+          }
+        }
+
+        _errorReporter.atOffset(
+            errorCode: unnecessaryIgnoreLintCode,
+            offset: ignoredName.offset,
+            length: name.length,
+            arguments: [name]);
+      }
+    }
   }
 }
 
diff --git a/pkg/analyzer/messages.yaml b/pkg/analyzer/messages.yaml
@@ -27022,13 +27022,6 @@ WarningCode:
         B(super.value);
       }
       ```
-  UNNECESSARY_IGNORE:
-    problemMessage: "The diagnostic '{0}' isn't produced at this location so it doesn't need to be ignored."
-    correctionMessage: Try removing the name from the list, or removing the whole comment if this is the only name in the list.
-    hasPublishedDocs: false
-    comment: |-
-      Parameters:
-      0: the name of the diagnostic being ignored
   UNNECESSARY_NAN_COMPARISON_FALSE:
     sharedName: UNNECESSARY_NAN_COMPARISON
     problemMessage: A double can't equal 'double.nan', so the condition is always 'false'.
diff --git a/pkg/analyzer/test/src/diagnostics/test_all.dart b/pkg/analyzer/test/src/diagnostics/test_all.dart
@@ -875,7 +875,6 @@ import 'unignorable_ignore_test.dart' as unignorable_ignore;
 import 'unnecessary_cast_pattern_test.dart' as unnecessary_cast_pattern;
 import 'unnecessary_cast_test.dart' as unnecessary_cast;
 import 'unnecessary_final_test.dart' as unnecessary_final;
-import 'unnecessary_ignore_test.dart' as unnecessary_ignore;
 import 'unnecessary_import_test.dart' as unnecessary_import;
 import 'unnecessary_nan_comparison_test.dart' as unnecessary_nan_comparison;
 import 'unnecessary_no_such_method_test.dart' as unnecessary_no_such_method;
@@ -1498,7 +1497,6 @@ main() {
     unnecessary_cast_pattern.main();
     unnecessary_cast.main();
     unnecessary_final.main();
-    unnecessary_ignore.main();
     unnecessary_nan_comparison.main();
     unnecessary_no_such_method.main();
     unnecessary_non_null_assertion.main();
diff --git a/pkg/analyzer/test/src/diagnostics/unnecessary_ignore_test.dart b/pkg/analyzer/test/src/diagnostics/unnecessary_ignore_test.dart
diff --git a/pkg/analyzer/tool/diagnostics/diagnostics.md b/pkg/analyzer/tool/diagnostics/diagnostics.md
@@ -28689,6 +28689,33 @@ class C {
 }
 ```
 
+### unnecessary_ignore
+
+_The diagnostic '{0}' isn't produced at this location so it doesn't need to be
+ignored._
+
+#### Description
+
+The analyzer produces this diagnostic when an ignore is specified to ignore a diagnostic that isn't produced.
+
+#### Example
+
+The following code produces this diagnostic because the `unused_local_variable`
+diagnostic isn't reported at the ignored location:
+
+```dart
+// ignore: [!unused_local_variable!]
+void f() {}
+```
+
+#### Common fixes
+
+Remove the ignore comment:
+
+```dart
+void f() {}
+```
+
 ### unnecessary_lambdas
 
 _Closure should be a tearoff._
diff --git a/pkg/linter/CHANGELOG.md b/pkg/linter/CHANGELOG.md
@@ -1,4 +1,8 @@
-# 3.7.0-wip
+# 3.8.0-wip
+
+- new _(experimental)_ lint: `unnecessary_ignore`
+
+# 3.7.0
 
 - new lint: `unnecessary_underscores`
 - new lint: `strict_top_level_inference`
diff --git a/pkg/linter/example/all.yaml b/pkg/linter/example/all.yaml
@@ -188,6 +188,7 @@ linter:
     - unnecessary_constructor_name
     - unnecessary_final
     - unnecessary_getters_setters
+    - unnecessary_ignore
     - unnecessary_lambdas
     - unnecessary_late
     - unnecessary_library_directive
diff --git a/pkg/linter/lib/src/lint_codes.g.dart b/pkg/linter/lib/src/lint_codes.g.dart
@@ -1636,6 +1636,16 @@ class LinterLintCode extends LintCode {
     hasPublishedDocs: true,
   );
 
+  static const LintCode unnecessary_ignore = LinterLintCode(
+    LintNames.unnecessary_ignore,
+    "The diagnostic '{0}' isn't produced at this location so it doesn't need "
+    "to be ignored.",
+    correctionMessage:
+        "Try removing the name from the list, or removing the whole comment if "
+        "this is the only name in the list.",
+    uniqueName: 'unnecessary_ignore',
+  );
+
   static const LintCode unnecessary_lambdas = LinterLintCode(
     LintNames.unnecessary_lambdas,
     "Closure should be a tearoff.",
diff --git a/pkg/linter/lib/src/lint_names.g.dart b/pkg/linter/lib/src/lint_names.g.dart
@@ -500,6 +500,8 @@ abstract final class LintNames {
   static const String unnecessary_getters_setters =
       'unnecessary_getters_setters';
 
+  static const String unnecessary_ignore = 'unnecessary_ignore';
+
   static const String unnecessary_lambdas = 'unnecessary_lambdas';
 
   static const String unnecessary_late = 'unnecessary_late';
diff --git a/pkg/linter/lib/src/rules.dart b/pkg/linter/lib/src/rules.dart
@@ -206,6 +206,7 @@ import 'rules/unnecessary_const.dart';
 import 'rules/unnecessary_constructor_name.dart';
 import 'rules/unnecessary_final.dart';
 import 'rules/unnecessary_getters_setters.dart';
+import 'rules/unnecessary_ignore.dart';
 import 'rules/unnecessary_lambdas.dart';
 import 'rules/unnecessary_late.dart';
 import 'rules/unnecessary_library_directive.dart';
@@ -455,6 +456,7 @@ void registerLintRules() {
     ..registerLintRule(UnnecessaryConstructorName())
     ..registerLintRule(UnnecessaryFinal())
     ..registerLintRule(UnnecessaryGettersSetters())
+    ..registerLintRule(UnnecessaryIgnore())
     ..registerLintRule(UnnecessaryLambdas())
     ..registerLintRule(UnnecessaryLate())
     ..registerLintRule(UnnecessaryLibraryDirective())
diff --git a/pkg/linter/lib/src/rules/unnecessary_ignore.dart b/pkg/linter/lib/src/rules/unnecessary_ignore.dart
diff --git a/pkg/linter/messages.yaml b/pkg/linter/messages.yaml
diff --git a/pkg/linter/test/rules/all.dart b/pkg/linter/test/rules/all.dart
diff --git a/pkg/linter/test/rules/unnecessary_ignore_test.dart b/pkg/linter/test/rules/unnecessary_ignore_test.dart
