eliminate crash on warnings generated by README.md (#1410)

* Fix readme warnings

* Update test to generate warnings from the readme

* dartfmt

* Shift around abstract classes to make this more understandable

* dartfmt

Author: jcollins-g

CHANGELOG.md

@@ -1,3 +1,6 @@
+## 0.11.2
+* Fix regression where warnings generated by the README could result in a fatal exception.  #1409
+
 ## 0.11.1
 * Fix regression where a property or top level variable can be listed twice
   under some conditions.  #1401

bin/dartdoc.dart

@@ -177,10 +177,10 @@ main(List<String> arguments) async {
     print('\nSuccess! Docs generated into ${results.outDir.absolute.path}');
   }, onError: (e, Chain chain) {
     if (e is DartDocFailure) {
-      stderr.writeln('Generation failed: ${e}.');
+      stderr.writeln('\nGeneration failed: ${e}.');
       exit(1);
     } else {
-      stderr.writeln('Generation failed: ${e}\n${chain.terse}');
+      stderr.writeln('\nGeneration failed: ${e}\n${chain.terse}');
       exit(255);
     }
   });

lib/dartdoc.dart

@@ -44,7 +44,7 @@ export 'src/sdk.dart';
 
 const String name = 'dartdoc';
 // Update when pubspec version changes.
-const String version = '0.11.1';
+const String version = '0.11.2';
 
 final String defaultOutDir = path.join('doc', 'api');
 
@@ -241,7 +241,7 @@ class DartDoc {
     }
     if (referenceElement == null && source == 'index.html')
       referenceElement = package;
-    package.warn(referenceElement, kind, p);
+    package.warnOnElement(referenceElement, kind, p);
   }
 
   void _doOrphanCheck(Package package, String origin, Set<String> visited) {

lib/src/html/html_generator_instance.dart

@@ -189,7 +189,7 @@ class HtmlGeneratorInstance implements HtmlOptions {
     stdout
         .write('\ngenerating docs for library ${lib.name} from ${lib.path}...');
     if (!lib.isAnonymous && !lib.hasDocumentation) {
-      package.warn(lib, PackageWarning.noLibraryLevelDocs);
+      package.warnOnElement(lib, PackageWarning.noLibraryLevelDocs);
     }
     TemplateData data =
         new LibraryTemplateData(this, package, lib, useCategories);

lib/src/markdown_processor.dart

@@ -164,7 +164,11 @@ ModelElement _getPreferredClass(ModelElement modelElement) {
 }
 
 // TODO: this is in the wrong place
-NodeList<CommentReference> _getCommentRefs(ModelElement modelElement) {
+NodeList<CommentReference> _getCommentRefs(Documentable documentable) {
+  // Documentable items that aren't related to analyzer elements have no
+  // CommentReference list.
+  if (documentable is! ModelElement) return null;
+  ModelElement modelElement = documentable;
   Class preferredClass = _getPreferredClass(modelElement);
   ModelElement cModelElement = modelElement.package
       .findCanonicalModelElementFor(modelElement.element,
@@ -219,7 +223,7 @@ NodeList<CommentReference> _getCommentRefs(ModelElement modelElement) {
 
 /// Returns null if element is a parameter.
 MatchingLinkResult _getMatchingLinkElement(
-    String codeRef, ModelElement element, List<CommentReference> commentRefs) {
+    String codeRef, Documentable element, List<CommentReference> commentRefs) {
   // By debugging inspection, it seems correct to not warn when we don't have
   // CommentReferences; there's actually nothing that needs resolving in
   // that case.
@@ -244,19 +248,16 @@ MatchingLinkResult _getMatchingLinkElement(
   // dartdoc-canonicalization-aware?
   if (refElement == null) {
     refElement = _getRefElementFromCommentRefs(commentRefs, codeRef);
+  } else {
+    assert(refElement is! PropertyAccessorElement);
+    assert(refElement is! PrefixElement);
   }
 
   // Did not find it anywhere.
   if (refElement == null) {
     return new MatchingLinkResult(null, null);
   }
 
-  if (refElement is PropertyAccessorElement) {
-    // yay we found an accessor that wraps a const, but we really
-    // want the top-level field itself
-    refElement = (refElement as PropertyAccessorElement).variable;
-  }
-
   // Ignore all parameters.
   if (refElement is ParameterElement || refElement is TypeParameterElement)
     return new MatchingLinkResult(null, null, warn: false);
@@ -300,7 +301,17 @@ Element _getRefElementFromCommentRefs(
       bool isConstrElement = ref.identifier.staticElement is ConstructorElement;
       // Constructors are now handled by library search.
       if (!isConstrElement) {
-        return ref.identifier.staticElement;
+        Element refElement = ref.identifier.staticElement;
+        if (refElement is PropertyAccessorElement) {
+          // yay we found an accessor that wraps a const, but we really
+          // want the top-level field itself
+          refElement = (refElement as PropertyAccessorElement).variable;
+        }
+        if (refElement is PrefixElement) {
+          // We found a prefix element, but what we really want is the library element.
+          refElement = (refElement as PrefixElement).enclosingElement;
+        }
+        return refElement;
       }
     }
   }
@@ -340,6 +351,7 @@ Map<String, Set<ModelElement>> _findRefElementCache;
 // TODO(jcollins-g): A complex package winds up spending a lot of cycles in here.  Optimize.
 Element _findRefElementInLibrary(
     String codeRef, ModelElement element, List<CommentReference> commentRefs) {
+  assert(element != null);
   assert(element.package.allLibrariesAdded);
 
   String codeRefChomped = codeRef.replaceFirst(isConstructor, '');
@@ -625,14 +637,13 @@ void _getResultsForClass(Class tryClass, String codeRefChomped,
   }
 }
 
-String _linkDocReference(String codeRef, ModelElement element,
+String _linkDocReference(String codeRef, Documentable documentable,
     NodeList<CommentReference> commentRefs) {
   // TODO(jcollins-g): Refactor so that doc operations work on the
   //                   documented element.
-  element = element.overriddenDocumentedElement;
-
+  documentable = documentable.overriddenDocumentedElement;
   MatchingLinkResult result;
-  result = _getMatchingLinkElement(codeRef, element, commentRefs);
+  result = _getMatchingLinkElement(codeRef, documentable, commentRefs);
   final ModelElement linkedElement = result.element;
   final String label = result.label ?? codeRef;
   if (linkedElement != null) {
@@ -649,18 +660,19 @@ String _linkDocReference(String codeRef, ModelElement element,
     }
   } else {
     if (result.warn) {
-      element.warn(PackageWarning.unresolvedDocReference, codeRef);
+      documentable.warn(PackageWarning.unresolvedDocReference, codeRef);
     }
     return '<code>${HTML_ESCAPE.convert(label)}</code>';
   }
 }
 
-String _renderMarkdownToHtml(String text, [ModelElement element]) {
+String _renderMarkdownToHtml(Documentable element) {
   md.Node _linkResolver(String name) {
     NodeList<CommentReference> commentRefs = _getCommentRefs(element);
     return new md.Text(_linkDocReference(name, element, commentRefs));
   }
 
+  String text = element.documentation;
   _showWarningsForGenericsOutsideSquareBracketsBlocks(text, element);
   return md.markdownToHtml(text,
       inlineSyntaxes: _markdown_syntaxes, linkResolver: _linkResolver);
@@ -676,7 +688,7 @@ const maxPostContext = 30;
 // like a non HTML tag (a generic?) outside of a `[]` block.
 // https://github.com/dart-lang/dartdoc/issues/1250#issuecomment-269257942
 void _showWarningsForGenericsOutsideSquareBracketsBlocks(
-    String text, ModelElement element) {
+    String text, Warnable element) {
   List<int> tagPositions = findFreeHangingGenericsPositions(text);
   if (tagPositions.isNotEmpty) {
     tagPositions.forEach((int position) {
@@ -732,13 +744,8 @@ class Documentation {
   final String asHtml;
   final String asOneLiner;
 
-  factory Documentation(String markdown) {
-    String tempHtml = _renderMarkdownToHtml(markdown);
-    return new Documentation._internal(markdown, tempHtml);
-  }
-
-  factory Documentation.forElement(ModelElement element) {
-    String tempHtml = _renderMarkdownToHtml(element.documentation, element);
+  factory Documentation.forElement(Documentable element) {
+    String tempHtml = _renderMarkdownToHtml(element);
     return new Documentation._internal(element.documentation, tempHtml);
   }
 

lib/src/model.dart

@@ -895,11 +895,13 @@ class Constructor extends ModelElement
 
 /// Bridges the gap between model elements and packages,
 /// both of which have documentation.
-abstract class Documentable {
+abstract class Documentable implements Warnable {
   String get documentation;
   String get documentationAsHtml;
   bool get hasDocumentation;
   String get oneLineDoc;
+  Documentable get overriddenDocumentedElement;
+  Package get package;
 }
 
 class Dynamic extends ModelElement {
@@ -1709,8 +1711,7 @@ class Method extends ModelElement
 /// helps prevent subtle bugs as generated output for a non-canonical
 /// ModelElement will reference itself as part of the "wrong" [Library]
 /// from the public interface perspective.
-abstract class ModelElement
-    implements Comparable, Nameable, Documentable, Locatable {
+abstract class ModelElement implements Comparable, Nameable, Documentable {
   final Element _element;
   final Library _library;
 
@@ -2152,6 +2153,7 @@ abstract class ModelElement
   bool _overriddenDocumentedElementIsSet = false;
   // TODO(jcollins-g): This method prefers canonical elements, but it isn't
   // guaranteed and is probably the source of bugs or confusing warnings.
+  @override
   ModelElement get overriddenDocumentedElement {
     if (!_overriddenDocumentedElementIsSet) {
       ModelElement found = this;
@@ -2180,6 +2182,7 @@ abstract class ModelElement
     return _overriddenDepth;
   }
 
+  @override
   Package get package =>
       (this is Library) ? (this as Library).package : this.library.package;
 
@@ -2236,14 +2239,15 @@ abstract class ModelElement
     return _parameters;
   }
 
+  @override
   void warn(PackageWarning kind, [String message]) {
     if (kind == PackageWarning.unresolvedDocReference &&
         overriddenElement != null) {
       // The documentation we're using for this element came from somewhere else.
       // Attach the warning to that element to deduplicate.
       overriddenElement.warn(kind, message);
     } else {
-      library.package.warn(this, kind, message);
+      library.package.warnOnElement(this, kind, message);
     }
   }
 
@@ -2763,6 +2767,11 @@ Map<PackageWarning, List<String>> packageWarningText = {
   ],
 };
 
+// Something that package warnings can be called on.
+abstract class Warnable implements Locatable {
+  void warn(PackageWarning warning, [String message]);
+}
+
 // Something that can be located for warning purposes.
 abstract class Locatable {
   String get fullyQualifiedName;
@@ -2889,7 +2898,7 @@ class PackageWarningCounter {
   }
 }
 
-class Package implements Nameable, Documentable, Locatable {
+class Package implements Nameable, Documentable {
   // Library objects serving as entry points for documentation.
   final List<Library> _libraries = [];
   // All library objects related to this package; a superset of _libraries.
@@ -2911,6 +2920,12 @@ class Package implements Nameable, Documentable, Locatable {
 
   final PackageMeta packageMeta;
 
+  @override
+  Package get package => this;
+
+  @override
+  Documentable get overriddenDocumentedElement => this;
+
   final Map<Element, Library> _elementToLibrary = {};
   String _docsAsHtml;
   final Map<String, String> _macros = {};
@@ -2958,29 +2973,33 @@ class Package implements Nameable, Documentable, Locatable {
 
   PackageWarningCounter get packageWarningCounter => _packageWarningCounter;
 
-  void warn(Locatable modelElement, PackageWarning kind, [String message]) {
-    if (modelElement != null) {
+  @override
+  void warn(PackageWarning kind, [String message]) {
+    warnOnElement(this, kind, message);
+  }
+
+  void warnOnElement(Warnable warnable, PackageWarning kind, [String message]) {
+    if (warnable != null) {
       // This sort of warning is only applicable to top level elements.
       if (kind == PackageWarning.ambiguousReexport) {
-        Element topLevelElement = modelElement.element;
+        Element topLevelElement = warnable.element;
         while (topLevelElement.enclosingElement is! CompilationUnitElement) {
           topLevelElement = topLevelElement.enclosingElement;
         }
-        modelElement = new ModelElement.from(
+        warnable = new ModelElement.from(
             topLevelElement, findOrCreateLibraryFor(topLevelElement));
       }
-      if (modelElement is Accessor) {
+      if (warnable is Accessor) {
         // This might be part of a Field, if so, assign this warning to the field
         // rather than the Accessor.
-        if ((modelElement as Accessor).enclosingCombo != null)
-          modelElement = (modelElement as Accessor).enclosingCombo;
+        if ((warnable as Accessor).enclosingCombo != null)
+          warnable = (warnable as Accessor).enclosingCombo;
       }
     } else {
       // If we don't have an element, we need a message to disambiguate.
       assert(message != null);
     }
-    if (_packageWarningCounter.hasWarning(
-        modelElement?.element, kind, message)) {
+    if (_packageWarningCounter.hasWarning(warnable?.element, kind, message)) {
       return;
     }
     // Elements that are part of the Dart SDK can have colons in their FQNs.
@@ -2991,9 +3010,9 @@ class Package implements Nameable, Documentable, Locatable {
     //                   them out doesn't work as well there since it might confuse
     //                   the user, yet we still want IntelliJ to link properly.
     String nameSplitFromColonPieces;
-    if (modelElement != null) {
+    if (warnable != null) {
       nameSplitFromColonPieces =
-          modelElement.fullyQualifiedName.replaceFirst(':', '-');
+          warnable.fullyQualifiedName.replaceFirst(':', '-');
     }
     String warningMessage;
     switch (kind) {
@@ -3014,7 +3033,7 @@ class Package implements Nameable, Documentable, Locatable {
         break;
       case PackageWarning.noLibraryLevelDocs:
         warningMessage =
-            "${modelElement.fullyQualifiedName} has no library level documentation comments";
+            "${warnable.fullyQualifiedName} has no library level documentation comments";
         break;
       case PackageWarning.ambiguousDocReference:
         warningMessage =
@@ -3047,9 +3066,9 @@ class Package implements Nameable, Documentable, Locatable {
         break;
     }
     String fullMessage =
-        "${warningMessage} ${modelElement != null ? modelElement.elementLocation : ''}";
+        "${warningMessage} ${warnable != null ? warnable.elementLocation : ''}";
     packageWarningCounter.addWarning(
-        modelElement?.element, kind, message, fullMessage);
+        warnable?.element, kind, message, fullMessage);
   }
 
   static Package _withAutoIncludedDependencies(
@@ -3110,7 +3129,7 @@ class Package implements Nameable, Documentable, Locatable {
     // Help the user if they pass us a category that doesn't exist.
     for (String categoryName in config.categoryOrder) {
       if (!result.containsKey(categoryName))
-        warn(null, PackageWarning.categoryOrderGivesMissingPackageName,
+        warnOnElement(null, PackageWarning.categoryOrderGivesMissingPackageName,
             "${categoryName}, categories: ${result.keys.join(',')}");
     }
     List<PackageCategory> packageCategories = result.values.toList()..sort();
@@ -3167,8 +3186,7 @@ class Package implements Nameable, Documentable, Locatable {
   @override
   String get documentationAsHtml {
     if (_docsAsHtml != null) return _docsAsHtml;
-
-    _docsAsHtml = new Documentation(documentation).asHtml;
+    _docsAsHtml = new Documentation.forElement(this).asHtml;
 
     return _docsAsHtml;
   }

pubspec.yaml

@@ -1,6 +1,6 @@
 name: dartdoc
 # Also update the `version` field in lib/dartdoc.dart.
-version: 0.11.1
+version: 0.11.2
 author: Dart Team <[email protected]>
 description: A documentation generator for Dart.
 homepage: https://github.com/dart-lang/dartdoc

testing/test_package/README.md

@@ -23,6 +23,8 @@ and_yaml:
   - 3.14
 ```
 
+It sometimes generates warnings in commentRefs like this: [unknownThingy.FromSomewhere]
+
 Be sure to check out other awesome packages on [pub][].
 
 [pub]: https://pub.dartlang.org