Clean up warnings handling (#2536)

* Clean up warnings handling

* Address feedback around comments

Author: parlough

lib/src/warnings.dart

@@ -2,13 +2,14 @@
 // 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 'dart:collection';
+
 import 'package:analyzer/dart/element/element.dart';
 import 'package:analyzer/file_system/file_system.dart';
 import 'package:dartdoc/src/dartdoc_options.dart';
 import 'package:dartdoc/src/logging.dart';
 import 'package:dartdoc/src/model/model.dart';
 import 'package:dartdoc/src/package_meta.dart';
-import 'package:dartdoc/src/tuple.dart';
 
 abstract class PackageWarningOptionContext implements DartdocOptionContextBase {
   bool get allowNonLocalWarnings =>
@@ -113,14 +114,15 @@ class PackageWarningDefinition implements Comparable<PackageWarningDefinition> {
 }
 
 /// Same as [packageWarningDefinitions], except keyed by the warning name.
-final Map<String, PackageWarningDefinition> packageWarningsByName =
-    Map.fromEntries(packageWarningDefinitions.values
-        .map((definition) => MapEntry(definition.warningName, definition)));
+final Map<String, PackageWarningDefinition> packageWarningsByName = {
+  for (final definition in packageWarningDefinitions.values)
+    definition.warningName: definition
+};
 
 /// Provides description text and command line flags for warnings.
 /// TODO(jcollins-g): Actually use this for command line flags.
-final Map<PackageWarning, PackageWarningDefinition> packageWarningDefinitions =
-    const {
+const Map<PackageWarning, PackageWarningDefinition> packageWarningDefinitions =
+    {
   PackageWarning.ambiguousDocReference: PackageWarningDefinition(
       PackageWarning.ambiguousDocReference,
       'ambiguous-doc-reference',
@@ -317,7 +319,7 @@ enum PackageWarningMode {
 /// In particular, this set should not include warnings around public/private
 /// or canonicalization problems, because those can break the isDocumented()
 /// check.
-final Set<PackageWarning> skipWarningIfNotDocumentedFor = {
+const Set<PackageWarning> skipWarningIfNotDocumentedFor = {
   PackageWarning.unresolvedDocReference,
   PackageWarning.typeAsHtml
 };
@@ -419,11 +421,26 @@ class PackageWarningOptions {
 }
 
 class PackageWarningCounter {
-  final Map<Element, Set<Tuple2<PackageWarning, String>>> countedWarnings = {};
+  final Map<Element, Map<PackageWarning, Set<String>>> _countedWarnings = {};
   final _items = <Jsonable>[];
   final _displayedWarningCounts = <PackageWarning, int>{};
   final PackageGraph packageGraph;
 
+  int _errorCount = 0;
+
+  /// The total amount of errors this package has experienced.
+  int get errorCount => _errorCount;
+
+  int _warningCount = 0;
+
+  /// The total amount of warnings this package has experienced.
+  int get warningCount => _warningCount;
+
+  /// An unmodifiable map view of all counted warnings related by their element,
+  /// warning type, and message.
+  UnmodifiableMapView<Element, Map<PackageWarning, Set<String>>>
+      get countedWarnings => UnmodifiableMapView(_countedWarnings);
+
   PackageWarningCounter(this.packageGraph);
 
   /// Actually write out the warning.  Assumes it is already counted with add.
@@ -462,11 +479,16 @@ class PackageWarningCounter {
     _items.clear();
   }
 
-  /// Returns true if we've already warned for this.
+  /// If this package has had any warnings counted.
+  bool get hasWarnings => _countedWarnings.isNotEmpty;
+
+  /// Returns `true` if we've already warned for this
+  /// combination of [element], [kind], and [message].
   bool hasWarning(Warnable element, PackageWarning kind, String message) {
-    var warningData = Tuple2<PackageWarning, String>(kind, message);
-    if (countedWarnings.containsKey(element?.element)) {
-      return countedWarnings[element?.element].contains(warningData);
+    final warning = _countedWarnings[element?.element];
+    if (warning != null) {
+      final messages = warning[kind];
+      return messages != null && messages.contains(message);
     }
     return false;
   }
@@ -484,19 +506,18 @@ class PackageWarningCounter {
       warningMode = PackageWarningMode.ignore;
     }
     if (warningMode == PackageWarningMode.warn) {
-      warningCount += 1;
+      _warningCount += 1;
     } else if (warningMode == PackageWarningMode.error) {
-      errorCount += 1;
+      _errorCount += 1;
     }
-    var warningData = Tuple2<PackageWarning, String>(kind, message);
-    countedWarnings.putIfAbsent(element?.element, () => {}).add(warningData);
+    _countedWarnings
+        .putIfAbsent(element?.element, () => {})
+        .putIfAbsent(kind, () => {})
+        .add(message);
     _writeWarning(kind, warningMode, config.verboseWarnings,
         element?.fullyQualifiedName, fullMessage);
   }
 
-  int errorCount = 0;
-  int warningCount = 0;
-
   @override
   String toString() {
     var errors = '$errorCount ${errorCount == 1 ? "error" : "errors"}';

test/documentation_comment_test.dart

@@ -32,8 +32,10 @@ void main() {
   Matcher hasMissingExampleWarning(String message) =>
       _HasWarning(PackageWarning.missingExampleFile, message);
 
-  void expectNoWarnings() =>
-      expect(packageGraph.packageWarningCounter.countedWarnings, isEmpty);
+  void expectNoWarnings() {
+    expect(packageGraph.packageWarningCounter.hasWarnings, isFalse);
+    expect(packageGraph.packageWarningCounter.countedWarnings, isEmpty);
+  }
 
   group('documentation_comment tests', () {
     setUp(() async {

test/end2end/dartdoc_test.dart

@@ -12,7 +12,6 @@ import 'package:dartdoc/dartdoc.dart';
 import 'package:dartdoc/src/io_utils.dart';
 import 'package:dartdoc/src/logging.dart';
 import 'package:dartdoc/src/model/model.dart';
-import 'package:dartdoc/src/tuple.dart';
 import 'package:dartdoc/src/warnings.dart';
 import 'package:path/path.dart' as path;
 import 'package:test/test.dart';
@@ -147,10 +146,8 @@ void main() {
       var results = await dartdoc.generateDocsBase();
       var p = results.packageGraph;
       var unresolvedToolErrors = p.packageWarningCounter.countedWarnings.values
-          .expand<String>((Set<Tuple2<PackageWarning, String>> s) => s
-              .where((Tuple2<PackageWarning, String> t) =>
-                  t.item1 == PackageWarning.toolError)
-              .map<String>((Tuple2<PackageWarning, String> t) => t.item2));
+          .map((e) => e[PackageWarning.toolError] ?? {})
+          .expand((element) => element);
 
       expect(p.packageWarningCounter.errorCount, equals(1));
       expect(unresolvedToolErrors.length, equals(1));
@@ -165,10 +162,8 @@ void main() {
       var p = results.packageGraph;
       var unresolvedExportWarnings = p
           .packageWarningCounter.countedWarnings.values
-          .expand<String>((Set<Tuple2<PackageWarning, String>> s) => s
-              .where((Tuple2<PackageWarning, String> t) =>
-                  t.item1 == PackageWarning.unresolvedExport)
-              .map<String>((Tuple2<PackageWarning, String> t) => t.item2));
+          .map((e) => e[PackageWarning.unresolvedExport] ?? {})
+          .expand((element) => element);
 
       expect(unresolvedExportWarnings.length, equals(1));
       expect(unresolvedExportWarnings.first,

test/end2end/model_test.dart

@@ -15,7 +15,6 @@ import 'package:dartdoc/src/render/model_element_renderer.dart';
 import 'package:dartdoc/src/render/parameter_renderer.dart';
 import 'package:dartdoc/src/render/typedef_renderer.dart';
 import 'package:dartdoc/src/special_elements.dart';
-import 'package:dartdoc/src/tuple.dart';
 import 'package:dartdoc/src/warnings.dart';
 import 'package:test/test.dart';
 
@@ -905,11 +904,11 @@ void main() {
                 '<a href="%%__HTMLBASE_dartdoc_internal__%%reexport_two/BaseReexported/action.html">ExtendedBaseReexported.action</a></p>'));
         var doAwesomeStuffWarnings = packageGraph.packageWarningCounter
                 .countedWarnings[doAwesomeStuff.element] ??
-            [];
+            {};
         expect(
             doAwesomeStuffWarnings,
-            isNot(anyElement((Tuple2<PackageWarning, String> e) =>
-                e.item1 == PackageWarning.ambiguousDocReference)));
+            isNot(doAwesomeStuffWarnings
+                .containsKey(PackageWarning.ambiguousDocReference)));
       });
 
       test('can handle renamed imports', () {