From 230948e1cae87fe0d12ef2c49803c76c7f634171 Mon Sep 17 00:00:00 2001 From: Janice Collins Date: Wed, 10 May 2017 10:09:52 -0700 Subject: [PATCH 1/5] Fix readme warnings --- CHANGELOG.md | 3 ++ bin/dartdoc.dart | 4 +- lib/dartdoc.dart | 4 +- lib/src/html/html_generator_instance.dart | 2 +- lib/src/markdown_processor.dart | 54 ++++++++++++++--------- lib/src/model.dart | 38 ++++++++++++---- pubspec.yaml | 2 +- 7 files changed, 72 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ef7c44c0c..e811c1c743 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/bin/dartdoc.dart b/bin/dartdoc.dart index c368664f43..94dd366fd7 100644 --- a/bin/dartdoc.dart +++ b/bin/dartdoc.dart @@ -177,10 +177,10 @@ main(List 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); } }); diff --git a/lib/dartdoc.dart b/lib/dartdoc.dart index 69d11da6ff..b0026ce6a9 100644 --- a/lib/dartdoc.dart +++ b/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 visited) { diff --git a/lib/src/html/html_generator_instance.dart b/lib/src/html/html_generator_instance.dart index 436e6a234a..3de8f9b8a7 100644 --- a/lib/src/html/html_generator_instance.dart +++ b/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); diff --git a/lib/src/markdown_processor.dart b/lib/src/markdown_processor.dart index 093f6647d1..fed55c928d 100644 --- a/lib/src/markdown_processor.dart +++ b/lib/src/markdown_processor.dart @@ -164,7 +164,11 @@ ModelElement _getPreferredClass(ModelElement modelElement) { } // TODO: this is in the wrong place -NodeList _getCommentRefs(ModelElement modelElement) { +NodeList _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 _getCommentRefs(ModelElement modelElement) { /// Returns null if element is a parameter. MatchingLinkResult _getMatchingLinkElement( - String codeRef, ModelElement element, List commentRefs) { + String codeRef, Documentable element, List 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,6 +248,9 @@ 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. @@ -251,12 +258,6 @@ MatchingLinkResult _getMatchingLinkElement( 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> _findRefElementCache; // TODO(jcollins-g): A complex package winds up spending a lot of cycles in here. Optimize. Element _findRefElementInLibrary( String codeRef, ModelElement element, List 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 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 '${HTML_ESCAPE.convert(label)}'; } } -String _renderMarkdownToHtml(String text, [ModelElement element]) { +String _renderMarkdownToHtml(Documentable element) { md.Node _linkResolver(String name) { NodeList 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 tagPositions = findFreeHangingGenericsPositions(text); if (tagPositions.isNotEmpty) { tagPositions.forEach((int position) { @@ -732,13 +744,13 @@ class Documentation { final String asHtml; final String asOneLiner; - factory Documentation(String markdown) { - String tempHtml = _renderMarkdownToHtml(markdown); + /*factory Documentation(String markdown, Warnable warnable) { + String tempHtml = _renderMarkdownToHtml(markdown, warnable); 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); } diff --git a/lib/src/model.dart b/lib/src/model.dart index 6c710ea393..a61cfb42ff 100644 --- a/lib/src/model.dart +++ b/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 Locatable, Warnable { String get documentation; String get documentationAsHtml; bool get hasDocumentation; String get oneLineDoc; + Documentable get overriddenDocumentedElement; + Package get package; } class Dynamic extends ModelElement { @@ -1710,7 +1712,7 @@ class Method extends ModelElement /// ModelElement will reference itself as part of the "wrong" [Library] /// from the public interface perspective. abstract class ModelElement - implements Comparable, Nameable, Documentable, Locatable { + implements Comparable, Nameable, Documentable { final Element _element; final Library _library; @@ -2152,6 +2154,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 +2183,7 @@ abstract class ModelElement return _overriddenDepth; } + @override Package get package => (this is Library) ? (this as Library).package : this.library.package; @@ -2236,6 +2240,7 @@ abstract class ModelElement return _parameters; } + @override void warn(PackageWarning kind, [String message]) { if (kind == PackageWarning.unresolvedDocReference && overriddenElement != null) { @@ -2243,7 +2248,7 @@ abstract class ModelElement // 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 +2768,12 @@ Map> packageWarningText = { ], }; +// Something that package warnings can be called on. +abstract class Warnable { + void warn(PackageWarning warning, [String message]); +} + + // Something that can be located for warning purposes. abstract class Locatable { String get fullyQualifiedName; @@ -2889,7 +2900,8 @@ class PackageWarningCounter { } } -class Package implements Nameable, Documentable, Locatable { + +class Package implements Nameable, Documentable { // Library objects serving as entry points for documentation. final List _libraries = []; // All library objects related to this package; a superset of _libraries. @@ -2911,6 +2923,12 @@ class Package implements Nameable, Documentable, Locatable { final PackageMeta packageMeta; + @override + Package get package => this; + + @override + Documentable get overriddenDocumentedElement => this; + final Map _elementToLibrary = {}; String _docsAsHtml; final Map _macros = {}; @@ -2958,7 +2976,12 @@ class Package implements Nameable, Documentable, Locatable { PackageWarningCounter get packageWarningCounter => _packageWarningCounter; - void warn(Locatable modelElement, PackageWarning kind, [String message]) { + @override + void warn(PackageWarning kind, [String message]) { + warnOnElement(this, kind, message); + } + + void warnOnElement(Documentable modelElement, PackageWarning kind, [String message]) { if (modelElement != null) { // This sort of warning is only applicable to top level elements. if (kind == PackageWarning.ambiguousReexport) { @@ -3110,7 +3133,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 packageCategories = result.values.toList()..sort(); @@ -3167,8 +3190,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; } diff --git a/pubspec.yaml b/pubspec.yaml index 1192a831aa..ca637efd3c 100644 --- a/pubspec.yaml +++ b/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 description: A documentation generator for Dart. homepage: https://github.com/dart-lang/dartdoc From 40b99c3d6854b538a445180cde9ac3c8a95db6b7 Mon Sep 17 00:00:00 2001 From: Janice Collins Date: Wed, 10 May 2017 10:14:18 -0700 Subject: [PATCH 2/5] Update test to generate warnings from the readme --- testing/test_package/README.md | 2 ++ testing/test_package_docs/index.html | 3 ++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/testing/test_package/README.md b/testing/test_package/README.md index e0b03ab81c..f507880160 100644 --- a/testing/test_package/README.md +++ b/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 diff --git a/testing/test_package_docs/index.html b/testing/test_package_docs/index.html index c18a6073fc..51787a7668 100644 --- a/testing/test_package_docs/index.html +++ b/testing/test_package_docs/index.html @@ -4,7 +4,7 @@ - + test_package - Dart API docs @@ -92,6 +92,7 @@

Best Package

- "value" - 3.14 +

It sometimes generates warnings in commentRefs like this: unknownThingy.FromSomewhere

Be sure to check out other awesome packages on pub.

From 726c6fc989d0fcb1e03d82592c28a160a05ecbd0 Mon Sep 17 00:00:00 2001 From: Janice Collins Date: Wed, 10 May 2017 10:15:03 -0700 Subject: [PATCH 3/5] dartfmt --- lib/src/model.dart | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/lib/src/model.dart b/lib/src/model.dart index a61cfb42ff..b8e5fba1da 100644 --- a/lib/src/model.dart +++ b/lib/src/model.dart @@ -1711,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 { +abstract class ModelElement implements Comparable, Nameable, Documentable { final Element _element; final Library _library; @@ -2773,7 +2772,6 @@ abstract class Warnable { void warn(PackageWarning warning, [String message]); } - // Something that can be located for warning purposes. abstract class Locatable { String get fullyQualifiedName; @@ -2900,7 +2898,6 @@ class PackageWarningCounter { } } - class Package implements Nameable, Documentable { // Library objects serving as entry points for documentation. final List _libraries = []; @@ -2981,7 +2978,8 @@ class Package implements Nameable, Documentable { warnOnElement(this, kind, message); } - void warnOnElement(Documentable modelElement, PackageWarning kind, [String message]) { + void warnOnElement(Documentable modelElement, PackageWarning kind, + [String message]) { if (modelElement != null) { // This sort of warning is only applicable to top level elements. if (kind == PackageWarning.ambiguousReexport) { From 2d0d1a9499643fe21c00e0ad3246da1d011bed55 Mon Sep 17 00:00:00 2001 From: Janice Collins Date: Wed, 10 May 2017 10:27:12 -0700 Subject: [PATCH 4/5] Shift around abstract classes to make this more understandable --- lib/src/markdown_processor.dart | 5 ----- lib/src/model.dart | 30 +++++++++++++++--------------- 2 files changed, 15 insertions(+), 20 deletions(-) diff --git a/lib/src/markdown_processor.dart b/lib/src/markdown_processor.dart index fed55c928d..4c1f441e54 100644 --- a/lib/src/markdown_processor.dart +++ b/lib/src/markdown_processor.dart @@ -744,11 +744,6 @@ class Documentation { final String asHtml; final String asOneLiner; - /*factory Documentation(String markdown, Warnable warnable) { - String tempHtml = _renderMarkdownToHtml(markdown, warnable); - return new Documentation._internal(markdown, tempHtml); - }*/ - factory Documentation.forElement(Documentable element) { String tempHtml = _renderMarkdownToHtml(element); return new Documentation._internal(element.documentation, tempHtml); diff --git a/lib/src/model.dart b/lib/src/model.dart index b8e5fba1da..b9c31ba19e 100644 --- a/lib/src/model.dart +++ b/lib/src/model.dart @@ -895,7 +895,7 @@ class Constructor extends ModelElement /// Bridges the gap between model elements and packages, /// both of which have documentation. -abstract class Documentable implements Locatable, Warnable { +abstract class Documentable implements Warnable { String get documentation; String get documentationAsHtml; bool get hasDocumentation; @@ -2768,7 +2768,7 @@ Map> packageWarningText = { }; // Something that package warnings can be called on. -abstract class Warnable { +abstract class Warnable implements Locatable { void warn(PackageWarning warning, [String message]); } @@ -2978,30 +2978,30 @@ class Package implements Nameable, Documentable { warnOnElement(this, kind, message); } - void warnOnElement(Documentable modelElement, PackageWarning kind, + void warnOnElement(Warnable warnable, PackageWarning kind, [String message]) { - if (modelElement != null) { + 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)) { + warnable?.element, kind, message)) { return; } // Elements that are part of the Dart SDK can have colons in their FQNs. @@ -3012,9 +3012,9 @@ class Package implements Nameable, Documentable { // 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) { @@ -3035,7 +3035,7 @@ class Package implements Nameable, Documentable { 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 = @@ -3068,9 +3068,9 @@ class Package implements Nameable, Documentable { 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( From 6538f166dc90256b099362b1b3941e8f2bee46bf Mon Sep 17 00:00:00 2001 From: Janice Collins Date: Wed, 10 May 2017 10:27:28 -0700 Subject: [PATCH 5/5] dartfmt --- lib/src/model.dart | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/src/model.dart b/lib/src/model.dart index b9c31ba19e..8153456575 100644 --- a/lib/src/model.dart +++ b/lib/src/model.dart @@ -2978,8 +2978,7 @@ class Package implements Nameable, Documentable { warnOnElement(this, kind, message); } - void warnOnElement(Warnable warnable, PackageWarning kind, - [String 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) { @@ -3000,8 +2999,7 @@ class Package implements Nameable, Documentable { // If we don't have an element, we need a message to disambiguate. assert(message != null); } - if (_packageWarningCounter.hasWarning( - warnable?.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.