Add unresolved export warning to dartdoc (#1748)

* Add unresolved export warning

* Simplification

* Split test package docs into dev and stable versions

* Make stable and dev trees separate for Dart compare_output_test

Author: jcollins-g

analysis_options.yaml

@@ -8,6 +8,7 @@ analyzer:
     - 'pub.dartlang.org/**'
     - 'testing/**'
     - 'testing/test_package_flutter_plugin/**'
+    - 'testing/test_package_export_error/**'
 linter:
   rules:
     - annotate_overrides

lib/dartdoc.dart

@@ -133,7 +133,7 @@ class Dartdoc extends PackageBuilder {
   /// [DartdocResults] is returned if dartdoc succeeds. [DartdocFailure] is
   /// thrown if dartdoc fails in an expected way, for example if there is an
   /// analysis error in the code.
-  Future<DartdocResults> generateDocs() async {
+  Future<DartdocResults> generateDocsBase() async {
     Stopwatch _stopwatch = new Stopwatch()..start();
     double seconds;
     packageGraph = await buildPackageGraph();
@@ -167,18 +167,22 @@ class Dartdoc extends PackageBuilder {
     logInfo(
         "Documented ${packageGraph.publicLibraries.length} public librar${packageGraph.publicLibraries.length == 1 ? 'y' : 'ies'} "
         "in ${seconds.toStringAsFixed(1)} seconds");
+    return new DartdocResults(
+        config.topLevelPackageMeta, packageGraph, outputDir);
+  }
 
-    if (packageGraph.publicLibraries.isEmpty) {
+  Future<DartdocResults> generateDocs() async {
+    DartdocResults dartdocResults = await generateDocsBase();
+    if (dartdocResults.packageGraph.publicLibraries.isEmpty) {
       throw new DartdocFailure(
           "dartdoc could not find any libraries to document. Run `pub get` and try again.");
     }
 
-    if (packageGraph.packageWarningCounter.errorCount > 0) {
+    if (dartdocResults.packageGraph.packageWarningCounter.errorCount > 0) {
       throw new DartdocFailure("dartdoc encountered errors while processing");
     }
 
-    return new DartdocResults(
-        config.topLevelPackageMeta, packageGraph, outputDir);
+    return dartdocResults;
   }
 
   /// Warn on file paths.

lib/src/model.dart

@@ -4357,6 +4357,9 @@ class PackageGraph {
       case PackageWarning.deprecated:
         warningMessage = 'deprecated dartdoc usage: ${message}';
         break;
+      case PackageWarning.unresolvedExport:
+        warningMessage = 'unresolved export uri: ${message}';
+        break;
     }
 
     List<String> messageParts = [warningMessage];
@@ -4425,11 +4428,26 @@ class PackageGraph {
 
   Map<LibraryElement, Set<Library>> _libraryElementReexportedBy = new Map();
   void _tagReexportsFor(
-      final Library tll, final LibraryElement libraryElement) {
+      final Library topLevelLibrary,
+      final LibraryElement libraryElement,
+      [ExportElement lastExportedElement]) {
+    if (libraryElement == null) {
+      // The first call to _tagReexportFor should not have a null libraryElement.
+      assert(lastExportedElement != null);
+      warnOnElement(
+          findOrCreateLibraryFor(lastExportedElement.enclosingElement),
+          PackageWarning.unresolvedExport,
+          message: '"${lastExportedElement.uri}"',
+          referredFrom: <Locatable>[topLevelLibrary]);
+      return;
+    }
     _libraryElementReexportedBy.putIfAbsent(libraryElement, () => new Set());
-    _libraryElementReexportedBy[libraryElement].add(tll);
+    _libraryElementReexportedBy[libraryElement].add(topLevelLibrary);
     for (ExportElement exportedElement in libraryElement.exports) {
-      _tagReexportsFor(tll, exportedElement.exportedLibrary);
+      _tagReexportsFor(
+          topLevelLibrary,
+          exportedElement.exportedLibrary,
+          exportedElement);
     }
   }
 
@@ -5674,6 +5692,7 @@ class PackageBuilder {
       for (PackageWarning kind in PackageWarning.values) {
         switch (kind) {
           case PackageWarning.invalidParameter:
+          case PackageWarning.unresolvedExport:
             warningOptions.error(kind);
             break;
           default:

lib/src/warnings.dart

@@ -94,6 +94,10 @@ final Map<PackageWarning, PackageWarningHelpText> packageWarningText = const {
       PackageWarning.deprecated,
       "deprecated",
       "A dartdoc directive has a deprecated format."),
+  PackageWarning.unresolvedExport: const PackageWarningHelpText(
+      PackageWarning.unresolvedExport,
+      "unresolvedExport",
+      "An export refers to a URI that can not be resolved."),
 };
 
 /// Something that package warnings can be called on.  Optionally associated
@@ -135,6 +139,7 @@ enum PackageWarning {
   typeAsHtml,
   invalidParameter,
   deprecated,
+  unresolvedExport,
 }
 
 /// Warnings it is OK to skip if we can determine the warnable isn't documented.
@@ -158,6 +163,7 @@ class PackageWarningOptions {
   PackageWarningOptions(this.verboseWarnings) {
     asWarnings.addAll(PackageWarning.values);
     ignore(PackageWarning.typeAsHtml);
+    error(PackageWarning.unresolvedExport);
   }
 
   void _assertInvariantsOk() {
@@ -194,7 +200,7 @@ class PackageWarningOptions {
 }
 
 class PackageWarningCounter {
-  final _countedWarnings =
+  final countedWarnings =
       new Map<Element, Set<Tuple2<PackageWarning, String>>>();
   final _warningCounts = new Map<PackageWarning, int>();
   final PackageWarningOptions options;
@@ -249,8 +255,8 @@ class PackageWarningCounter {
   /// Returns true if we've already warned for this.
   bool hasWarning(Warnable element, PackageWarning kind, String message) {
     Tuple2<PackageWarning, String> warningData = new Tuple2(kind, message);
-    if (_countedWarnings.containsKey(element?.element)) {
-      return _countedWarnings[element?.element].contains(warningData);
+    if (countedWarnings.containsKey(element?.element)) {
+      return countedWarnings[element?.element].contains(warningData);
     }
     return false;
   }
@@ -263,8 +269,8 @@ class PackageWarningCounter {
     Tuple2<PackageWarning, String> warningData = new Tuple2(kind, message);
     _warningCounts.putIfAbsent(kind, () => 0);
     _warningCounts[kind] += 1;
-    _countedWarnings.putIfAbsent(element?.element, () => new Set());
-    _countedWarnings[element?.element].add(warningData);
+    countedWarnings.putIfAbsent(element?.element, () => new Set());
+    countedWarnings[element?.element].add(warningData);
     _writeWarning(kind, element?.fullyQualifiedName, fullMessage);
   }
 

test/dartdoc_test.dart

@@ -9,6 +9,8 @@ import 'dart:io';
 
 import 'package:dartdoc/dartdoc.dart';
 import 'package:dartdoc/src/model.dart';
+import 'package:dartdoc/src/tuple.dart';
+import 'package:dartdoc/src/warnings.dart';
 import 'package:path/path.dart' as pathLib;
 import 'package:test/test.dart';
 
@@ -33,9 +35,25 @@ void main() {
           argv..addAll(['--input', packageRoot.path])..addAll(outputParam)));
     }
 
+    test('with broken reexport chain', () async {
+      Dartdoc dartdoc = await buildDartdoc([], testPackageImportExportError);
+      DartdocResults results = await dartdoc.generateDocsBase();
+      PackageGraph p = results.packageGraph;
+      Iterable<String> 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));
+
+      expect(unresolvedExportWarnings.length, equals(1));
+      expect(unresolvedExportWarnings.first,
+          equals('"package:not_referenced_in_pubspec/library3.dart"'));
+    });
+
     group('include/exclude parameters', () {
       test('with config file', () async {
-        Dartdoc dartdoc = await buildDartdoc([], testPackageImportExport);
+        Dartdoc dartdoc = await buildDartdoc([], testPackageIncludeExclude);
         DartdocResults results = await dartdoc.generateDocs();
         PackageGraph p = results.packageGraph;
         expect(p.localPublicLibraries.map((l) => l.name),
@@ -44,7 +62,7 @@ void main() {
 
       test('with include command line argument', () async {
         Dartdoc dartdoc = await buildDartdoc(
-            ['--include', 'another_included'], testPackageImportExport);
+            ['--include', 'another_included'], testPackageIncludeExclude);
         DartdocResults results = await dartdoc.generateDocs();
         PackageGraph p = results.packageGraph;
         expect(p.localPublicLibraries.length, equals(1));
@@ -53,7 +71,7 @@ void main() {
 
       test('with exclude command line argument', () async {
         Dartdoc dartdoc = await buildDartdoc(
-            ['--exclude', 'more_included'], testPackageImportExport);
+            ['--exclude', 'more_included'], testPackageIncludeExclude);
         DartdocResults results = await dartdoc.generateDocs();
         PackageGraph p = results.packageGraph;
         expect(p.localPublicLibraries.length, equals(1));

test/src/utils.dart

@@ -27,8 +27,10 @@ final Directory testPackageWithEmbedderYaml =
     new Directory('testing/test_package_embedder_yaml');
 final Directory testPackageWithNoReadme =
     new Directory('testing/test_package_small');
-final Directory testPackageImportExport =
+final Directory testPackageIncludeExclude =
     new Directory('testing/test_package_include_exclude');
+final Directory testPackageImportExportError =
+    new Directory('testing/test_package_import_export_error');
 
 /// Convenience factory to build a [DartdocGeneratorOptionContext] and associate
 /// it with a [DartdocOptionSet] based on the current working directory and/or

testing/test_package_export_error/lib/library1.dart

@@ -0,0 +1,5 @@
+library library1;
+
+/// An invalid reexport - a non-existent library3.dart from 'not_referenced_in_pubspec'
+/// package.
+export 'package:not_referenced_in_pubspec/library3.dart' show Lib3Class;

testing/test_package_export_error/lib/library2.dart

@@ -0,0 +1,5 @@
+library library2;
+
+export 'package:test_package_export_error/library1.dart';
+
+class Lib2Class {}

testing/test_package_export_error/pubspec.yaml

@@ -0,0 +1,5 @@
+name: test_package_export_error
+version: 0.0.1
+description: A simple console application.
+environment:
+  sdk: <=3.0.0

testing/test_package_import_export_error/lib/main.dart

@@ -0,0 +1,6 @@
+library main;
+
+export 'package:test_package_export_error/library2.dart';
+
+/// This is an important class.
+class BugFreeClass {}