diff --git a/CHANGELOG.md b/CHANGELOG.md index e811c1c743..acfcf1344d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,5 @@ +* Improvements to warnings, including indicating referring elements where possible. #1405 + ## 0.11.2 * Fix regression where warnings generated by the README could result in a fatal exception. #1409 diff --git a/lib/dartdoc.dart b/lib/dartdoc.dart index 43c0a6dcd4..401ef382b0 100644 --- a/lib/dartdoc.dart +++ b/lib/dartdoc.dart @@ -246,7 +246,7 @@ class DartDoc { } if (referenceElement == null && source == 'index.html') referenceElement = package; - package.warnOnElement(referenceElement, kind, p); + package.warnOnElement(referenceElement, kind, message: p); } void _doOrphanCheck(Package package, String origin, Set visited) { diff --git a/lib/src/markdown_processor.dart b/lib/src/markdown_processor.dart index 4c1f441e54..b5722852a2 100644 --- a/lib/src/markdown_processor.dart +++ b/lib/src/markdown_processor.dart @@ -279,7 +279,8 @@ MatchingLinkResult _getMatchingLinkElement( } refModelElement = new ModelElement.from(searchElement, refLibrary); if (!refModelElement.isCanonical) { - refModelElement.warn(PackageWarning.noCanonicalFound); + refModelElement.warn(PackageWarning.noCanonicalFound, + referredFrom: element); // Don't warn about doc references because that's covered by the no // canonical library found message. return new MatchingLinkResult(null, null, warn: false); @@ -407,10 +408,10 @@ Element _findRefElementInLibrary( _getResultsForClass( tryClass, codeRefChomped, results, codeRef, package); } + results.remove(null); if (results.isNotEmpty) break; } - // Sometimes documentation refers to classes that are further up the chain. - // Get those too. + if (results.isEmpty && realClass != null) { for (Class superClass in realClass.superChain.map((et) => et.element as Class)) { @@ -418,6 +419,7 @@ Element _findRefElementInLibrary( _getResultsForClass( superClass, codeRefChomped, results, codeRef, package); } + results.remove(null); if (results.isNotEmpty) break; } } @@ -553,7 +555,8 @@ Element _findRefElementInLibrary( result = results.first.element; } else { element.warn(PackageWarning.ambiguousDocReference, - "[$codeRef] => ${results.map((r) => "'${r.fullyQualifiedName}'").join(", ")}"); + message: + "[$codeRef] => ${results.map((r) => "'${r.fullyQualifiedName}'").join(", ")}"); result = results.first.element; } return result; @@ -577,8 +580,9 @@ void _getResultsForClass(Class tryClass, String codeRefChomped, } else { // TODO(jcollins-g): get rid of reimplementation of identifier resolution // or integrate into ModelElement in a simpler way. - List superChain = []; - superChain.add(tryClass); + List superChain = [tryClass]; + superChain + .addAll(tryClass.interfaces.map((t) => t.returnElement as Class)); // This seems duplicitous with our caller, but the preferredClass // hint matters with findCanonicalModelElementFor. // TODO(jcollins-g): This makes our caller ~O(n^2) vs length of superChain. @@ -627,6 +631,7 @@ void _getResultsForClass(Class tryClass, String codeRefChomped, } } } + results.remove(null); if (results.isNotEmpty) break; if (c.fullyQualifiedNameWithoutLibrary == codeRefChomped) { results.add(c); @@ -639,9 +644,6 @@ void _getResultsForClass(Class tryClass, String codeRefChomped, String _linkDocReference(String codeRef, Documentable documentable, NodeList commentRefs) { - // TODO(jcollins-g): Refactor so that doc operations work on the - // documented element. - documentable = documentable.overriddenDocumentedElement; MatchingLinkResult result; result = _getMatchingLinkElement(codeRef, documentable, commentRefs); final ModelElement linkedElement = result.element; @@ -660,15 +662,16 @@ String _linkDocReference(String codeRef, Documentable documentable, } } else { if (result.warn) { - documentable.warn(PackageWarning.unresolvedDocReference, codeRef); + documentable.warn(PackageWarning.unresolvedDocReference, + message: codeRef, referredFrom: documentable.documentationFrom); } return '${HTML_ESCAPE.convert(label)}'; } } String _renderMarkdownToHtml(Documentable element) { + NodeList commentRefs = _getCommentRefs(element); md.Node _linkResolver(String name) { - NodeList commentRefs = _getCommentRefs(element); return new md.Text(_linkDocReference(name, element, commentRefs)); } @@ -702,7 +705,7 @@ void _showWarningsForGenericsOutsideSquareBracketsBlocks( postContext.replaceAll(new RegExp(r'\n.*$', multiLine: true), ''); String errorMessage = "$priorContext$postContext"; // TODO(jcollins-g): allow for more specific error location inside comments - element.warn(PackageWarning.typeAsHtml, errorMessage); + element.warn(PackageWarning.typeAsHtml, message: errorMessage); }); } } diff --git a/lib/src/model.dart b/lib/src/model.dart index 3a7827ed17..1ace5a5bdc 100644 --- a/lib/src/model.dart +++ b/lib/src/model.dart @@ -177,11 +177,11 @@ class Accessor extends ModelElement ModelElement get enclosingCombo => _enclosingCombo; @override - void warn(PackageWarning kind, [String message]) { + void warn(PackageWarning kind, {String message, Locatable referredFrom}) { if (enclosingCombo != null) { - enclosingCombo.warn(kind, message); + enclosingCombo.warn(kind, message: message, referredFrom: referredFrom); } else { - super.warn(kind, message); + super.warn(kind, message: message, referredFrom: referredFrom); } } @@ -984,6 +984,12 @@ class EnumField extends Field { } } + @override + EnumField get documentationFrom { + if (name == 'values' && name == 'index') return this; + return super.documentationFrom; + } + @override String get documentation { if (name == 'values') { @@ -1674,8 +1680,11 @@ class Method extends ModelElement Method get overriddenElement { ClassElement parent = element.enclosingElement; for (InterfaceType t in getAllSupertypes(parent)) { - if (t.getMethod(element.name) != null) { - return new ModelElement.from(t.getMethod(element.name), library); + Element e = t.getMethod(element.name); + if (e != null) { + assert(e.enclosingElement is ClassElement); + Library l = _findOrCreateEnclosingLibraryFor(e.enclosingElement); + return new ModelElement.from(e, l); } } return null; @@ -1907,25 +1916,35 @@ abstract class ModelElement implements Comparable, Nameable, Documentable { bool get canHaveParameters => element is ExecutableElement || element is FunctionTypeAliasElement; - /// Returns the docs, stripped of their leading comments syntax. + ModelElement _documentationFrom; + + /// Returns the ModelElement from which we will get documentation. /// /// This getter will walk up the inheritance hierarchy /// to find docs, if the current class doesn't have docs /// for this element. @override - String get documentation { - if (_rawDocs != null) return _rawDocs; - - _rawDocs = _computeDocumentationComment; - - if (_rawDocs == null && canOverride()) { - var overrideElement = overriddenElement; - if (overrideElement != null) { - _rawDocs = overrideElement.documentation ?? ''; - return _rawDocs; + ModelElement get documentationFrom { + if (_documentationFrom == null) { + if (_computeDocumentationComment == null && + canOverride() && + overriddenElement != null) { + _documentationFrom = overriddenElement; + } else if (this is Inheritable && (this as Inheritable).isInherited) { + Inheritable thisInheritable = (this as Inheritable); + ModelElement fromThis = new ModelElement.from( + element, thisInheritable.definingEnclosingElement.library); + _documentationFrom = fromThis.documentationFrom; + } else { + _documentationFrom = this; } } + return _documentationFrom; + } + String get _documentationLocal { + if (_rawDocs != null) return _rawDocs; + _rawDocs = _computeDocumentationComment ?? ''; _rawDocs = stripComments(_rawDocs) ?? ''; _rawDocs = _injectExamples(_rawDocs); _rawDocs = _stripMacroTemplatesAndAddToIndex(_rawDocs); @@ -1933,6 +1952,10 @@ abstract class ModelElement implements Comparable, Nameable, Documentable { return _rawDocs; } + /// Returns the docs, stripped of their leading comments syntax. + @override + String get documentation => documentationFrom._documentationLocal; + Library get definingLibrary => package.findOrCreateLibraryFor(element); Library _canonicalLibrary; @@ -1969,7 +1992,7 @@ abstract class ModelElement implements Comparable, Nameable, Documentable { // they should go here. if (candidateLibraries.length > 1) { warn(PackageWarning.ambiguousReexport, - "${candidateLibraries.map((l) => l.name)}"); + message: "${candidateLibraries.map((l) => l.name)}"); } if (candidateLibraries.isNotEmpty) _canonicalLibrary = candidateLibraries.first; @@ -2047,7 +2070,7 @@ abstract class ModelElement implements Comparable, Nameable, Documentable { bool _isLineNumberComputed = false; @override Tuple2 get lineAndColumn { - // TODO(jcollins-g): we should always be able to get line numbers. Why can't we, sometimes? + /// TODO(jcollins-g): implement lineAndColumn for explicit fields if (!_isLineNumberComputed) { _lineAndColumn = lineNumberCache.lineAndColumn( element.source.fullName, element.nameOffset); @@ -2241,15 +2264,9 @@ abstract class ModelElement implements Comparable, Nameable, Documentable { } @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.warnOnElement(this, kind, message); - } + void warn(PackageWarning kind, {String message, Locatable referredFrom}) { + package.warnOnElement(this, kind, + message: message, referredFrom: referredFrom); } String get _computeDocumentationComment => element.documentationComment; @@ -2736,7 +2753,7 @@ Map> packageWarningText = { ], PackageWarning.noCanonicalFound: [ "no-canonical-found", - "A symbol is is part of the public interface for this package, but no library documented with this package documents it so dartdoc can not link to it" + "A symbol is part of the public interface for this package, but no library documented with this package documents it so dartdoc can not link to it" ], PackageWarning.noLibraryLevelDocs: [ "no-library-level-docs", @@ -2770,13 +2787,14 @@ Map> packageWarningText = { // Something that package warnings can be called on. abstract class Warnable implements Locatable { - void warn(PackageWarning warning, [String message]); + void warn(PackageWarning warning, {String message, Locatable referredFrom}); } // Something that can be located for warning purposes. abstract class Locatable { String get fullyQualifiedName; String get href; + Locatable get documentationFrom; Element get element; String get elementLocation; Tuple2 get lineAndColumn; @@ -2927,6 +2945,9 @@ class Package implements Nameable, Documentable { @override Documentable get overriddenDocumentedElement => this; + @override + Documentable get documentationFrom => this; + final Map _elementToLibrary = {}; String _docsAsHtml; final Map _macros = {}; @@ -2975,11 +2996,23 @@ class Package implements Nameable, Documentable { PackageWarningCounter get packageWarningCounter => _packageWarningCounter; @override - void warn(PackageWarning kind, [String message]) { - warnOnElement(this, kind, message); + void warn(PackageWarning kind, {String message, Locatable referredFrom}) { + warnOnElement(this, kind, message: message, referredFrom: referredFrom); } - void warnOnElement(Warnable warnable, PackageWarning kind, [String message]) { + /// Returns colon-stripped name and location of the given locatable. + static Tuple2 nameAndLocation(Locatable locatable) { + String locatableName = ''; + String locatableLocation = ''; + if (locatable != null) { + locatableName = locatable.fullyQualifiedName.replaceFirst(':', '-'); + locatableLocation = locatable.elementLocation; + } + return new Tuple2(locatableName, locatableLocation); + } + + void warnOnElement(Warnable warnable, PackageWarning kind, + {String message, Locatable referredFrom}) { if (warnable != null) { // This sort of warning is only applicable to top level elements. if (kind == PackageWarning.ambiguousReexport) { @@ -3010,11 +3043,11 @@ class Package implements Nameable, Documentable { // TODO(jcollins-g): What about messages that may include colons? Substituting // 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 (warnable != null) { - nameSplitFromColonPieces = - warnable.fullyQualifiedName.replaceFirst(':', '-'); - } + Tuple2 warnableStrings = nameAndLocation(warnable); + String warnablePrefix = 'from'; + Tuple2 referredFromStrings = nameAndLocation(referredFrom); + String referredFromPrefix = 'referred to by'; + String name = warnableStrings.item1; String warningMessage; switch (kind) { case PackageWarning.noCanonicalFound: @@ -3022,39 +3055,38 @@ class Package implements Nameable, Documentable { // --auto-include-dependencies. // TODO(jcollins-g): add a dartdoc flag to enable external website linking for non-canonical elements, using .packages for versioning // TODO(jcollins-g): support documenting multiple packages at once and linking between them - warningMessage = - "no canonical library found for ${nameSplitFromColonPieces}, not linking"; + warningMessage = "no canonical library found for ${name}, not linking"; break; case PackageWarning.ambiguousReexport: // Fix these warnings by adding the original library exporting the // symbol with --include, or by using --auto-include-dependencies. // TODO(jcollins-g): add a dartdoc flag to force a particular resolution order for (or drop) ambiguous reexports warningMessage = - "ambiguous reexport of ${nameSplitFromColonPieces}, canonicalization candidates: ${message}"; + "ambiguous reexport of ${name}, canonicalization candidates: ${message}"; break; case PackageWarning.noLibraryLevelDocs: warningMessage = "${warnable.fullyQualifiedName} has no library level documentation comments"; break; case PackageWarning.ambiguousDocReference: - warningMessage = - "ambiguous doc reference in ${nameSplitFromColonPieces}: ${message}"; + warningMessage = "ambiguous doc reference ${message}"; break; case PackageWarning.categoryOrderGivesMissingPackageName: warningMessage = "--category-order gives invalid package name: '${message}'"; break; case PackageWarning.unresolvedDocReference: - warningMessage = - "unresolved doc reference [${message}], from ${nameSplitFromColonPieces}"; + warningMessage = "unresolved doc reference [${message}]"; + if (referredFrom == null) { + referredFrom = warnable.documentationFrom; + } + referredFromPrefix = 'in documentation inherited from'; break; case PackageWarning.brokenLink: - warningMessage = - 'dartdoc generated a broken link to: ${message}, from ${nameSplitFromColonPieces == null ? "" : nameSplitFromColonPieces}'; + warningMessage = 'dartdoc generated a broken link to: ${message}'; break; case PackageWarning.orphanedFile: - warningMessage = - 'dartdoc generated a file orphan: ${message}, from ${nameSplitFromColonPieces == null ? "" : nameSplitFromColonPieces}'; + warningMessage = 'dartdoc generated a file orphan: ${message}'; break; case PackageWarning.unknownFile: warningMessage = @@ -3066,8 +3098,22 @@ class Package implements Nameable, Documentable { warningMessage = 'generic type handled as HTML: """${message}"""'; break; } - String fullMessage = - "${warningMessage} ${warnable != null ? warnable.elementLocation : ''}"; + + List messageParts = [warningMessage]; + if (warnable != null) + messageParts.add( + "${warnablePrefix} ${warnableStrings.item1}: ${warnableStrings.item2}"); + if (referredFrom != null && referredFrom != warnable) { + messageParts.add( + "${referredFromPrefix} ${referredFromStrings.item1}: ${referredFromStrings.item2}"); + } + String fullMessage; + if (messageParts.length <= 2) { + fullMessage = messageParts.join(', '); + } else { + fullMessage = messageParts.join('\n '); + } + packageWarningCounter.addWarning( warnable?.element, kind, message, fullMessage); } @@ -3131,7 +3177,7 @@ class Package implements Nameable, Documentable { for (String categoryName in config.categoryOrder) { if (!result.containsKey(categoryName)) warnOnElement(null, PackageWarning.categoryOrderGivesMissingPackageName, - "${categoryName}, categories: ${result.keys.join(',')}"); + message: "${categoryName}, categories: ${result.keys.join(',')}"); } List packageCategories = result.values.toList()..sort(); return packageCategories;