linter: new rule for documenting ignore comments

In order to land this new rule, I add a new `IgnoredElement` subclass,
`IgnoredDiagnosticComment`, for any trailing comment text. This seems
like a pretty clean implementation. The text and offset are not used,
but it seems like it would be inconsistent to not offer them.

I also make a few IgnoreInfo fields private that are unused outside
the library.

Fixes https://github.com/dart-lang/linter/issues/4860

Change-Id: Id6f949a317929dae8833e404d4c9f3c8e4f7bc90
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/365488
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Phil Quitslund <pquitslund@google.com>

pkg/analysis_server/lib/src/services/correction/error_fix_status.yaml

@@ -2092,6 +2092,11 @@ LintCode.discarded_futures:
   status: hasFix
 LintCode.do_not_use_environment:
   status: noFix
+LintCode.document_ignores:
+  status: needsFix
+  notes: |-
+    We of course cannot generate a fix including appropriate text, but we can
+    insert a stub comment in the correct position.
 LintCode.empty_catches:
   status: hasFix
 LintCode.empty_constructor_bodies:

pkg/analyzer/lib/src/ignore_comments/ignore_info.dart

@@ -9,6 +9,19 @@ import 'package:analyzer/source/line_info.dart';
 import 'package:analyzer/src/dart/ast/token.dart';
 import 'package:analyzer/src/utilities/extensions/string.dart';
 
+/// The text and location of trailing unstructured comment text in an ignore
+/// comment.
+class IgnoredDiagnosticComment implements IgnoredElement {
+  final String text;
+
+  final int offset;
+
+  IgnoredDiagnosticComment(this.text, this.offset);
+
+  @override
+  bool matches(ErrorCode errorCode) => false;
+}
+
 /// The name and location of a diagnostic name in an ignore comment.
 class IgnoredDiagnosticName implements IgnoredElement {
   /// The name of the diagnostic being ignored.
@@ -16,11 +29,8 @@ class IgnoredDiagnosticName implements IgnoredElement {
 
   final int offset;
 
-  /// Initialize a newly created diagnostic name to have the given [name] and
-  /// [offset].
   IgnoredDiagnosticName(String name, this.offset) : name = name.toLowerCase();
 
-  /// Returns whether this diagnostic name matches the given error code.
   @override
   bool matches(ErrorCode errorCode) {
     if (name == errorCode.name.toLowerCase()) {
@@ -35,6 +45,7 @@ class IgnoredDiagnosticName implements IgnoredElement {
   }
 }
 
+/// The name and location of a diagnostic type in an ignore comment.
 class IgnoredDiagnosticType implements IgnoredElement {
   final String type;
 
@@ -51,6 +62,7 @@ class IgnoredDiagnosticType implements IgnoredElement {
 }
 
 sealed class IgnoredElement {
+  /// Returns whether this matches the given [errorCode].
   bool matches(ErrorCode errorCode);
 }
 
@@ -70,13 +82,13 @@ class IgnoreInfo {
   /// A regular expression for matching 'ignore' comments in a .yaml file.
   ///
   /// Resulting codes may be in a list ('error_code_1,error_code2').
-  static final RegExp yamlIgnoreMatcher =
+  static final RegExp _yamlIgnoreMatcher =
       RegExp(r'^(?<before>.*)#+[ ]*ignore:(?<ignored>.*)', multiLine: true);
 
   /// A regular expression for matching 'ignore_for_file' comments.
   ///
   /// Resulting codes may be in a list ('error_code_1,error_code2').
-  static final RegExp yamlIgnoreForFileMatcher =
+  static final RegExp _yamlIgnoreForFileMatcher =
       RegExp(r'#[ ]*ignore_for_file:(?<ignored>.*)');
 
   static final _trimmedCommaSeparatedMatcher = RegExp(r'[^\s,]([^,]*[^\s,])?');
@@ -89,20 +101,20 @@ class IgnoreInfo {
   /// that are ignored for the whole file.
   final List<IgnoredElement> _ignoredForFile = [];
 
-  final LineInfo lineInfo;
+  final LineInfo _lineInfo;
 
-  IgnoreInfo.empty() : lineInfo = LineInfo([]);
+  IgnoreInfo.empty() : _lineInfo = LineInfo([]);
 
   /// Initializes a newly created instance of this class to represent the ignore
   /// comments in the given compilation [unit].
   IgnoreInfo.forDart(CompilationUnit unit, String content)
-      : lineInfo = unit.lineInfo {
+      : _lineInfo = unit.lineInfo {
     for (var comment in unit.ignoreComments) {
       var lexeme = comment.lexeme;
       if (lexeme.contains('ignore:')) {
-        var location = lineInfo.getLocation(comment.offset);
+        var location = _lineInfo.getLocation(comment.offset);
         var lineNumber = location.lineNumber;
-        var offsetOfLine = lineInfo.getOffsetOfLine(lineNumber - 1);
+        var offsetOfLine = _lineInfo.getOffsetOfLine(lineNumber - 1);
         var beforeMatch = content.substring(
             offsetOfLine, offsetOfLine + location.columnNumber - 1);
         if (beforeMatch.trim().isEmpty) {
@@ -120,7 +132,7 @@ class IgnoreInfo {
 
   /// Initializes a newly created instance of this class to represent the ignore
   /// comments in the given YAML file.
-  IgnoreInfo.forYaml(String content, this.lineInfo) {
+  IgnoreInfo.forYaml(String content, this._lineInfo) {
     Iterable<IgnoredDiagnosticName> diagnosticNamesInMatch(RegExpMatch match) {
       var ignored = match.namedGroup('ignored')!;
       var offset = match.start;
@@ -129,11 +141,11 @@ class IgnoreInfo {
           .map((m) => IgnoredDiagnosticName(m[0]!, offset + m.start));
     }
 
-    for (var match in yamlIgnoreForFileMatcher.allMatches(content)) {
+    for (var match in _yamlIgnoreForFileMatcher.allMatches(content)) {
       _ignoredForFile.addAll(diagnosticNamesInMatch(match));
     }
-    for (var match in yamlIgnoreMatcher.allMatches(content)) {
-      var lineNumber = lineInfo.getLocation(match.start).lineNumber;
+    for (var match in _yamlIgnoreMatcher.allMatches(content)) {
+      var lineNumber = _lineInfo.getLocation(match.start).lineNumber;
       var beforeComment = match.namedGroup('before')!;
       var nextLine = beforeComment.trim().isEmpty;
       _ignoredOnLine
@@ -142,16 +154,16 @@ class IgnoreInfo {
     }
   }
 
-  /// Return `true` if there are any ignore comments in the file.
+  /// Whether there are any ignore comments in the file.
   bool get hasIgnores =>
       _ignoredOnLine.isNotEmpty || _ignoredForFile.isNotEmpty;
 
-  /// Return a list containing all of the diagnostics that are ignored for the
-  /// whole file.
+  /// A list containing all of the diagnostics that are ignored for the whole
+  /// file.
   List<IgnoredElement> get ignoredForFile => _ignoredForFile.toList();
 
-  /// Return a table mapping line numbers to the diagnostics that are ignored on
-  /// that line.
+  /// A table mapping line numbers to the diagnostics that are ignored on that
+  /// line.
   Map<int, List<IgnoredElement>> get ignoredOnLine {
     Map<int, List<IgnoredElement>> ignoredOnLine = {};
     for (var entry in _ignoredOnLine.entries) {
@@ -161,11 +173,11 @@ class IgnoreInfo {
   }
 
   bool ignored(AnalysisError error) {
-    var line = lineInfo.getLocation(error.offset).lineNumber;
+    var line = _lineInfo.getLocation(error.offset).lineNumber;
     return ignoredAt(error.errorCode, line);
   }
 
-  /// Return `true` if the [errorCode] is ignored at the given [line].
+  /// Returns whether the [errorCode] is ignored at the given [line].
   bool ignoredAt(ErrorCode errorCode, int line) {
     var ignoredDiagnostics = _ignoredOnLine[line];
     if (ignoredForFile.isEmpty && ignoredDiagnostics == null) {
@@ -213,6 +225,10 @@ extension CommentTokenExtension on CommentToken {
       }
     }
 
+    // We only want to add an `IgnoredDiagnosticComment` if it is preceded by
+    // one or more `IgnoredDiagnosticName`s or `IgnoredDiagnosticType`s.
+    var hasIgnoredElements = false;
+
     while (true) {
       skipPastWhitespace();
       if (offset == lexeme.length) {
@@ -224,6 +240,12 @@ extension CommentTokenExtension on CommentToken {
       readWord();
       if (wordOffset == offset) {
         // There is a non-word (other characters) at `offset`.
+        if (hasIgnoredElements) {
+          yield IgnoredDiagnosticComment(
+            lexeme.substring(offset),
+            this.offset + wordOffset,
+          );
+        }
         return;
       }
       var word = lexeme.substring(wordOffset, offset);
@@ -240,6 +262,12 @@ extension CommentTokenExtension on CommentToken {
         readWord();
         if (typeOffset == offset) {
           // There is a non-word (other characters) at `offset`.
+          if (hasIgnoredElements) {
+            yield IgnoredDiagnosticComment(
+              lexeme.substring(offset),
+              this.offset + wordOffset,
+            );
+          }
           return;
         }
         if (offset < lexeme.length) {
@@ -247,10 +275,15 @@ extension CommentTokenExtension on CommentToken {
           if (!nextChar.isSpace && !nextChar.isComma) {
             // There are non-identifier characters at the end of this word,
             // like `ignore: http://google.com`. This is not a diagnostic name.
+            if (hasIgnoredElements) {
+              yield IgnoredDiagnosticComment(
+                  lexeme.substring(wordOffset), this.offset + wordOffset);
+            }
             return;
           }
         }
         var type = lexeme.substring(typeOffset, offset);
+        hasIgnoredElements = true;
         yield IgnoredDiagnosticType(
             type, this.offset + wordOffset, offset - wordOffset);
       } else {
@@ -259,9 +292,14 @@ extension CommentTokenExtension on CommentToken {
           if (!nextChar.isSpace && !nextChar.isComma) {
             // There are non-identifier characters at the end of this word,
             // like `ignore: http://google.com`. This is not a diagnostic name.
+            if (hasIgnoredElements) {
+              yield IgnoredDiagnosticComment(
+                  lexeme.substring(wordOffset), this.offset + wordOffset);
+            }
             return;
           }
         }
+        hasIgnoredElements = true;
         yield IgnoredDiagnosticName(word, this.offset + wordOffset);
       }
 
@@ -270,9 +308,15 @@ extension CommentTokenExtension on CommentToken {
       if (offset == lexeme.length) return;
 
       var nextChar = lexeme.codeUnitAt(offset);
-      if (!nextChar.isComma) return;
-      // We've reached the end of the comma-separated codes and types. What
-      // follows is unstructured comment text.
+      if (!nextChar.isComma) {
+        // We've reached the end of the comma-separated codes and types. What
+        // follows is unstructured comment text.
+        if (hasIgnoredElements) {
+          yield IgnoredDiagnosticComment(
+              lexeme.substring(offset), this.offset + wordOffset);
+        }
+        return;
+      }
       offset++;
       if (offset == lexeme.length) return;
     }

pkg/analyzer/test/src/ignore_comments/ignore_info_test.dart

@@ -67,8 +67,9 @@ class IgnoreInfoTest extends PubPackageResolutionTest {
 
   test_trailingCommas() async {
     var ignoredElements = await _parseIgnoredElements('// ignore: foo,,');
-    expect(ignoredElements, hasLength(1));
+    expect(ignoredElements, hasLength(2));
     _expectIgnoredName(ignoredElements[0], name: 'foo', offset: 11);
+    expect(ignoredElements[1], isA<IgnoredDiagnosticComment>());
   }
 
   test_trailingCommaSpace() async {
@@ -85,8 +86,9 @@ class IgnoreInfoTest extends PubPackageResolutionTest {
 
   test_trailingText() async {
     var ignoredElements = await _parseIgnoredElements('// ignore: foo because');
-    expect(ignoredElements, hasLength(1));
+    expect(ignoredElements, hasLength(2));
     _expectIgnoredName(ignoredElements[0], name: 'foo', offset: 11);
+    expect(ignoredElements[1], isA<IgnoredDiagnosticComment>());
   }
 
   test_type() async {
@@ -107,12 +109,12 @@ class IgnoreInfoTest extends PubPackageResolutionTest {
   test_type_nameWithExtraCharacters() async {
     var ignoredElements =
         await _parseIgnoredElements('// ignore: type=http://google.com');
-    expect(ignoredElements, hasLength(0));
+    expect(ignoredElements, isEmpty);
   }
 
   test_type_nonIdentifierName() async {
     var ignoredElements = await _parseIgnoredElements('// ignore: type=!!');
-    expect(ignoredElements, hasLength(0));
+    expect(ignoredElements, isEmpty);
   }
 
   test_type_spaceAfterEqual() async {

pkg/linter/example/all.yaml

@@ -69,6 +69,7 @@ linter:
     - directives_ordering
     - discarded_futures
     - do_not_use_environment
+    - document_ignores
     - empty_catches
     - empty_constructor_bodies
     - empty_statements

pkg/linter/lib/src/rules.dart

@@ -74,6 +74,7 @@ import 'rules/diagnostic_describe_all_properties.dart';
 import 'rules/directives_ordering.dart';
 import 'rules/discarded_futures.dart';
 import 'rules/do_not_use_environment.dart';
+import 'rules/document_ignores.dart';
 import 'rules/empty_catches.dart';
 import 'rules/empty_constructor_bodies.dart';
 import 'rules/empty_statements.dart';
@@ -312,6 +313,7 @@ void registerLintRules() {
     ..register(DiagnosticDescribeAllProperties())
     ..register(DirectivesOrdering())
     ..register(DiscardedFutures())
+    ..register(DocumentIgnores())
     ..register(DoNotUseEnvironment())
     ..register(EmptyCatches())
     ..register(EmptyConstructorBodies())

pkg/linter/lib/src/rules/document_ignores.dart

@@ -0,0 +1,116 @@
+// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'package:analyzer/dart/ast/ast.dart';
+import 'package:analyzer/dart/ast/visitor.dart';
+// ignore: implementation_imports
+import 'package:analyzer/src/ignore_comments/ignore_info.dart';
+// ignore: implementation_imports
+import 'package:analyzer/src/utilities/extensions/string.dart';
+
+import '../analyzer.dart';
+
+const _desc = r'Document ignore comments.';
+
+const _details = r'''
+**DO** document all ignored diagnostic reports.
+
+**BAD:**
+```dart
+// ignore: unused_element
+int _x = 1;
+```
+
+**GOOD:**
+```dart
+// This private field will be used later.
+// ignore: unused_element
+int _x = 1;
+```
+
+''';
+
+class DocumentIgnores extends LintRule {
+  static const LintCode code = LintCode(
+    'document_ignores',
+    'Missing documentation explaining why a diagnostic is ignored.',
+    correctionMessage:
+        'Try adding a comment immediately above the ignore comment.',
+  );
+
+  DocumentIgnores()
+      : super(
+            name: 'document_ignores',
+            description: _desc,
+            details: _details,
+            group: Group.style);
+
+  @override
+  LintCode get lintCode => code;
+
+  @override
+  void registerNodeProcessors(
+      NodeLintRegistry registry, LinterContext context) {
+    var visitor = _Visitor(this, context);
+    registry.addCompilationUnit(this, visitor);
+  }
+}
+
+class _Visitor extends SimpleAstVisitor<void> {
+  final LintRule rule;
+
+  final String content;
+
+  _Visitor(this.rule, LinterContext context)
+      : content = context.currentUnit.content;
+
+  @override
+  void visitCompilationUnit(CompilationUnit node) {
+    for (var comment in node.ignoreComments) {
+      var ignoredElements = comment.ignoredElements;
+      if (ignoredElements.isEmpty) {
+        continue;
+      }
+      if (ignoredElements.last is IgnoredDiagnosticComment) {
+        // Some trailing text in `comment` documents this/these ignore(s).
+        continue;
+      }
+
+      var ignoreCommentLine =
+          node.lineInfo.getLocation(comment.offset).lineNumber;
+      if (ignoreCommentLine > 1) {
+        // Only look at the line above if the ignore comment line is not the
+        // first line.
+        var previousLineOffset =
+            node.lineInfo.getOffsetOfLine(ignoreCommentLine - 2);
+        if (_startsWithEndOfLineComment(previousLineOffset)) {
+          // A preceding comment, which may be attached to a different token,
+          // documents this/these ignore(s). For example in:
+          //
+          // ```dart
+          // // Text.
+          // int x = 0; // ignore: unused_element
+          // ```
+          continue;
+        }
+      }
+
+      rule.reportLintForToken(comment);
+    }
+  }
+
+  /// Returns whether [content] at [offset_] represents starts with optional
+  /// whitespace and then an end-of-line comment (two slashes).
+  bool _startsWithEndOfLineComment(int offset_) {
+    var offset = offset_;
+    var length = content.length;
+    while (offset < length) {
+      if (!content.codeUnitAt(offset).isSpace) break;
+      offset++;
+    }
+    if (offset + 1 >= length) return false;
+    return content.codeUnitAt(offset) == 0x2F /* '/' */ &&
+        content.codeUnitAt(offset + 1) == 0x2F /* '/' */;
+  }
+}

pkg/linter/test/rules/all.dart

@@ -89,6 +89,7 @@ import 'diagnostic_describe_all_properties_test.dart'
 import 'directives_ordering_test.dart' as directives_ordering;
 import 'discarded_futures_test.dart' as discarded_futures;
 import 'do_not_use_environment_test.dart' as do_not_use_environment;
+import 'document_ignores_test.dart' as document_ignores;
 import 'empty_catches_test.dart' as empty_catches;
 import 'empty_constructor_bodies_test.dart' as empty_constructor_bodies;
 import 'empty_statements_test.dart' as empty_statements;
@@ -330,6 +331,7 @@ void main() {
   directives_ordering.main();
   discarded_futures.main();
   do_not_use_environment.main();
+  document_ignores.main();
   empty_catches.main();
   empty_constructor_bodies.main();
   empty_statements.main();

pkg/linter/test/rules/document_ignores_test.dart

@@ -0,0 +1,100 @@
+// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'package:test_reflective_loader/test_reflective_loader.dart';
+
+import '../rule_test_support.dart';
+
+main() {
+  defineReflectiveSuite(() {
+    defineReflectiveTests(DocumentIgnoresTest);
+  });
+}
+
+@reflectiveTest
+class DocumentIgnoresTest extends LintRuleTest {
+  @override
+  String get lintRule => 'document_ignores';
+
+  test_precedingDeclaration_notDocumented() async {
+    await assertDiagnostics(
+      r'''
+int x = 0;
+
+// ignore: unused_element
+int _y = 0;
+''',
+      [
+        lint(12, 25),
+      ],
+    );
+  }
+
+  test_precedingLine_butIsTrailing_notDocumented() async {
+    await assertDiagnostics(
+      r'''
+int x = 0; // Text.
+// ignore: unused_element
+int _y = 0;
+''',
+      [
+        lint(20, 25),
+      ],
+    );
+  }
+
+  test_precedingLine_documented() async {
+    await assertNoDiagnostics(r'''
+// Text.
+// ignore: unused_element
+int _y = 0;
+''');
+  }
+
+  test_precedingLine_ignoreIsTrailing_documented() async {
+    await assertNoDiagnostics(r'''
+// Text.
+int _y = 0; // ignore: unused_element
+''');
+  }
+
+  test_precedingLine_multiple_documented() async {
+    await assertNoDiagnostics(r'''
+// Text.
+// ignore_for_file: unused_element
+// Text.
+// ignore_for_file: unnecessary_cast
+int _y = 0 as int;
+''');
+  }
+
+  test_precedingLine_multiple_notDocumented() async {
+    await assertDiagnostics(
+      r'''
+// Text.
+// ignore_for_file: unused_element
+
+// ignore_for_file: unnecessary_cast
+int _y = 0 as int;
+''',
+      [
+        lint(45, 36),
+      ],
+    );
+  }
+
+  test_sameComment_comma_documented() async {
+    await assertNoDiagnostics(r'''
+// ignore: unused_element, I need this
+int _y = 0;
+''');
+  }
+
+  test_sameComment_whitespace_documented() async {
+    await assertNoDiagnostics(r'''
+// ignore: unused_element http://linter-bug
+int _y = 0;
+''');
+  }
+}