Canonicalization overhaul (#1368)

Fix some structural problems with elements in dartdoc leading to canonicalization bugs, see CHANGELOG.md

Author: jcollins-g

CHANGELOG.md

@@ -1,3 +1,37 @@
+## 0.10.0
+
+* fix canonicalization problems and related issues introduced or not addressed
+  in 0.9.11, including:
+  * (#1361), (#1232), (#1239 (partial))- Broken links in enums
+  * (#1345) and (#1090)- Reexports have wrong links in many places
+  * (#1341), (#1197 (partial)) - Duplicate docs still in some cases
+  * (#1334) - Some classes don't list their subclasses
+  * Inheritable class members had incorrect canonicalization in many cases
+  * ... and many other unfiled bugs relating to inheritance and duplicate files.
+* Dartdoc no longer creates documentation for a given identifier more than once.
+  This means dartdoc is 20-30% faster on complex packages.
+* --auto-include-dependencies is now recursive past one layer (#589) It now drills
+  all the way down and will dive into the SDK and other packages.
+* Change display of warnings to be more consistent; warnings now always
+  go to stderr and are printed on their own line.
+* Dartdoc now warns when it is unable to find a canonical object to link to
+* Dartdoc now warns if a package exports an identifier so that it is
+  ambiguous which one should be treated as canonical
+* Dartdoc now has a number of asserts in checked mode for issues solved
+  and as-yet-unsolved, including (#1367) or canonicalization problems; try
+  running in checked mode if you see structural problems in generated docs and
+  see if an assert fires.
+* Dartdoc internals have changed significantly:
+  * Package now owns the calculation of recursive dependencies with a factory
+    constructor, Package.withAutoincludedDependencies.
+  * ModelElements and Libraries now have Package-scoped caches.
+  * ModelElements and their subclasses now must be constructed from a single
+    factory, ModelElements.from
+  * Package has new methods to assist canonicalization, including
+    findCanonicalLibraryFor and findCanonicalModelElementFor.
+  * New mixin "Inheritable" helps class members calculate canonicalization
+    for inheritable members
+
 ## 0.9.13
 
 * fix grind check-links and check-sdk-links (#1360)

bin/dartdoc.dart

@@ -41,7 +41,7 @@ main(List<String> arguments) async {
 
   Directory sdkDir = getSdkDir();
   if (sdkDir == null) {
-    print("Error: unable to locate the Dart SDK.");
+    stderr.write(" Error: unable to locate the Dart SDK.");
     exit(1);
   }
 
@@ -56,13 +56,15 @@ main(List<String> arguments) async {
 
   var readme = args['sdk-readme'];
   if (readme != null && !(new File(readme).existsSync())) {
-    print("Error: unable to locate the SDK description file at $readme.");
+    stderr
+        .write(" Error: unable to locate the SDK description file at $readme.");
     exit(1);
   }
 
   Directory inputDir = new Directory(args['input']);
   if (!inputDir.existsSync()) {
-    print("Error: unable to locate the input directory at ${inputDir.path}.");
+    stderr.write(
+        " Error: unable to locate the input directory at ${inputDir.path}.");
     exit(1);
   }
 
@@ -75,15 +77,15 @@ main(List<String> arguments) async {
       args['footer'].map(_resolveTildePath).toList() as List<String>;
   for (String footerFilePath in footerFilePaths) {
     if (!new File(footerFilePath).existsSync()) {
-      print("Error: unable to locate footer file: ${footerFilePath}.");
+      stderr.write(" Error: unable to locate footer file: ${footerFilePath}.");
       exit(1);
     }
   }
   List<String> headerFilePaths =
       args['header'].map(_resolveTildePath).toList() as List<String>;
   for (String headerFilePath in footerFilePaths) {
     if (!new File(headerFilePath).existsSync()) {
-      print("Error: unable to locate header file: ${headerFilePath}.");
+      stderr.write(" Error: unable to locate header file: ${headerFilePath}.");
       exit(1);
     }
   }
@@ -96,7 +98,8 @@ main(List<String> arguments) async {
 
   if (args.rest.isNotEmpty) {
     var unknownArgs = args.rest.join(' ');
-    print('Error: detected unknown command-line argument(s): $unknownArgs');
+    stderr.write(
+        'Error: detected unknown command-line argument(s): $unknownArgs');
     _printUsageAndExit(parser, exitCode: 1);
   }
 

lib/dartdoc.dart

@@ -40,7 +40,7 @@ export 'src/package_meta.dart';
 
 const String name = 'dartdoc';
 // Update when pubspec version changes.
-const String version = '0.9.13';
+const String version = '0.10.0';
 
 final String defaultOutDir = path.join('doc', 'api');
 
@@ -134,6 +134,7 @@ class DartDoc {
         ? const []
         : findFilesToDocumentInPackage(rootDir.path).toList();
 
+    // TODO(jcollins-g): seems like most of this belongs in the Package constructor
     List<LibraryElement> libraries = _parseLibraries(files, includeExternals);
 
     if (includes != null && includes.isNotEmpty) {
@@ -154,21 +155,27 @@ class DartDoc {
       });
     }
 
-    if (includes.isNotEmpty || excludes.isNotEmpty) {
-      print('generating docs for libraries ${libraries.join(', ')}\n');
-    }
-
-    Package package = new Package(libraries, packageMeta);
-
+    Package package;
     if (config != null && config.autoIncludeDependencies) {
-      final newLibraryElements =
-          _buildLibrariesWithAutoincludedDependencies(package);
-      Library.clearLibraryMap();
-      package = new Package(newLibraryElements, packageMeta);
+      package = Package.withAutoIncludedDependencies(libraries, packageMeta);
+      libraries = package.libraries.map((l) => l.element).toList();
+      // remove excluded libraries again, in case they are picked up through
+      // dependencies.
+      excludes.forEach((pattern) {
+        libraries.removeWhere((lib) {
+          return lib.name.startsWith(pattern) || lib.name == pattern;
+        });
+      });
     }
+    package = new Package(libraries, packageMeta);
+
+    print(
+        'generating docs for libraries ${package.libraries.map((Library l) => l.name).join(', ')}\n');
 
     // Go through docs of every model element in package to prebuild the macros index
-    package.allModelElements.forEach((m) => m.documentation);
+    // TODO(jcollins-g): move index building into a cached-on-demand generation
+    // like most other bits in [Package].
+    package.allCanonicalModelElements.forEach((m) => m.documentation);
 
     // Create the out directory.
     if (!outputDir.existsSync()) outputDir.createSync(recursive: true);
@@ -179,11 +186,11 @@ class DartDoc {
 
     double seconds = _stopwatch.elapsedMilliseconds / 1000.0;
     print(
-        "\nDocumented ${libraries.length} librar${libraries.length == 1 ? 'y' : 'ies'} "
+        "\nDocumented ${package.libraries.length} librar${package.libraries.length == 1 ? 'y' : 'ies'} "
         "in ${seconds.toStringAsFixed(1)} seconds.");
 
-    if (libraries.isEmpty) {
-      print(
+    if (package.libraries.isEmpty) {
+      stderr.write(
           "\ndartdoc could not find any libraries to document. Run `pub get` and try again.");
     }
 
@@ -192,7 +199,7 @@ class DartDoc {
 
   List<LibraryElement> _parseLibraries(
       List<String> files, List<String> includeExternals) {
-    List<LibraryElement> libraries = [];
+    Set<LibraryElement> libraries = new Set();
     DartSdk sdk = new FolderBasedDartSdk(PhysicalResourceProvider.INSTANCE,
         PhysicalResourceProvider.INSTANCE.getFolder(sdkDir.path));
     List<UriResolver> resolvers = [];
@@ -257,7 +264,7 @@ class DartDoc {
       sources.add(source);
       if (context.computeKindOf(source) == SourceKind.LIBRARY) {
         LibraryElement library = context.computeLibraryElement(source);
-        libraries.add(library);
+        if (!isPrivate(library)) libraries.add(library);
       }
     }
 
@@ -281,11 +288,35 @@ class DartDoc {
       LibraryElement library = context.computeLibraryElement(source);
       String libraryName = Library.getLibraryName(library);
       var fullPath = source.fullName;
+
       if (includeExternals.any((string) => fullPath.endsWith(string))) {
         if (libraries.map(Library.getLibraryName).contains(libraryName)) {
           continue;
         }
         libraries.add(library);
+      } else if (config != null &&
+          config.autoIncludeDependencies &&
+          libraryName != '') {
+        File searchFile = new File(fullPath);
+        searchFile =
+            new File(path.join(searchFile.parent.path, 'pubspec.yaml'));
+        bool foundLibSrc = false;
+        while (!foundLibSrc && searchFile.parent != null) {
+          if (searchFile.existsSync()) break;
+          List<String> pathParts = path.split(searchFile.parent.path);
+          // This is a pretty intensely hardcoded convention, but there seems to
+          // to be no other way to identify what might be a "top level" library
+          // here.  If lib/src is in the path between the file and the pubspec,
+          // assume that this is supposed to be private.
+          if (pathParts.length < 2) break;
+          pathParts = pathParts.sublist(pathParts.length - 2, pathParts.length);
+          foundLibSrc =
+              path.join(pathParts[0], pathParts[1]) == path.join('lib', 'src');
+          searchFile = new File(
+              path.join(searchFile.parent.parent.path, 'pubspec.yaml'));
+        }
+        if (foundLibSrc) continue;
+        libraries.add(library);
       }
     }
 
@@ -376,26 +407,3 @@ class _Error implements Comparable<_Error> {
   @override
   String toString() => '[${severityName}] ${description}';
 }
-
-Iterable<LibraryElement> _buildLibrariesWithAutoincludedDependencies(
-    Package package) {
-  final List<LibraryElement> newLibraryElements = []
-    ..addAll(package.libraries.map((l) => l.element as LibraryElement));
-
-  package.allModelElements.forEach((modelElement) {
-    modelElement.usedElements.forEach((used) {
-      if (used != null && used.modelType != null) {
-        final ModelElement modelTypeElement = used.modelType.element;
-        final library = package.findLibraryFor(modelTypeElement.element);
-        if (library == null && modelTypeElement.library != null) {
-          if (!newLibraryElements.contains(modelTypeElement.library.element) &&
-              !modelTypeElement.library.name.startsWith("dart:")) {
-            newLibraryElements.add(modelTypeElement.library.element);
-          }
-        }
-      }
-    });
-  });
-
-  return newLibraryElements;
-}

lib/src/element_type.dart

@@ -63,7 +63,11 @@ class ElementType {
   String get name => _type.name;
 
   ModelElement get returnElement {
-    Element e = _returnTypeCore.element;
+    Element e;
+    if (_type is FunctionType)
+      e = _returnTypeCore.element;
+    else
+      e = _type.element;
     if (e == null || e.library == null) {
       return null;
     }