diff --git a/lib/src/comment_references/parser.dart b/lib/src/comment_references/parser.dart index 28f558edca..6b09ccd308 100644 --- a/lib/src/comment_references/parser.dart +++ b/lib/src/comment_references/parser.dart @@ -158,6 +158,7 @@ class CommentReferenceParser { if (_atEnd) { return _PrefixParseResult.endOfFile; } + _walkPastWhitespace(); if (_tryMatchLiteral(_constructorHintPrefix, requireTrailingNonidentifier: true)) { return _PrefixParseResult.ok( diff --git a/lib/src/generator/templates.runtime_renderers.dart b/lib/src/generator/templates.runtime_renderers.dart index a530c78ab7..91ad99805b 100644 --- a/lib/src/generator/templates.runtime_renderers.dart +++ b/lib/src/generator/templates.runtime_renderers.dart @@ -2581,6 +2581,25 @@ class _Renderer_CommentReferable extends RendererBase { parent: r, getters: _invisibleGetters['Element']); }, ), + 'href': Property( + getValue: (CT_ c) => c.href, + renderVariable: + (CT_ c, Property self, List remainingNames) { + if (remainingNames.isEmpty) { + return self.getValue(c).toString(); + } + var name = remainingNames.first; + var nextProperty = + _Renderer_String.propertyMap().getValue(name); + return nextProperty.renderVariable(self.getValue(c), + nextProperty, [...remainingNames.skip(1)]); + }, + isNullValue: (CT_ c) => c.href == null, + renderValue: (CT_ c, RendererBase r, + List ast, StringSink sink) { + _render_String(c.href, ast, r.template, sink, parent: r); + }, + ), 'library': Property( getValue: (CT_ c) => c.library, renderVariable: @@ -14592,6 +14611,7 @@ const _invisibleGetters = { }, 'CommentReferable': { 'scope', + 'href', 'referenceChildren', 'referenceParents', 'referenceGrandparentOverrides', diff --git a/lib/src/markdown_processor.dart b/lib/src/markdown_processor.dart index 02c056a281..2ad19f1753 100644 --- a/lib/src/markdown_processor.dart +++ b/lib/src/markdown_processor.dart @@ -174,25 +174,24 @@ final List _markdownBlockSyntaxes = [ final RegExp _hideSchemes = RegExp('^(http|https)://'); class MatchingLinkResult { - final ModelElement modelElement; + final CommentReferable commentReferable; final bool warn; - MatchingLinkResult(this.modelElement, {this.warn = true}); + MatchingLinkResult(this.commentReferable, {this.warn = true}); @override bool operator ==(Object other) { return other is MatchingLinkResult && - modelElement == other.modelElement && + commentReferable == other.commentReferable && warn == other.warn; } @override - int get hashCode => hash2(modelElement, warn); + int get hashCode => hash2(commentReferable, warn); bool isEquivalentTo(MatchingLinkResult other) { - if (this == other) return true; - var compareThis = modelElement; - var compareOther = other.modelElement; + var compareThis = commentReferable; + var compareOther = other.commentReferable; if (compareThis is Accessor) { compareThis = (compareThis as Accessor).enclosingCombo; @@ -202,8 +201,16 @@ class MatchingLinkResult { compareOther = (compareOther as Accessor).enclosingCombo; } - if (compareThis?.canonicalModelElement == - compareOther?.canonicalModelElement) return true; + if (compareThis is ModelElement && + compareThis.canonicalModelElement != null) { + compareThis = (compareThis as ModelElement).canonicalModelElement; + } + if (compareOther is ModelElement && + compareOther.canonicalModelElement != null) { + compareOther = (compareOther as ModelElement).canonicalModelElement; + } + if (compareThis == compareOther) return true; + // The old implementation just throws away Parameter matches to avoid // problems with warning unnecessarily at higher levels of the code. // I'd like to fix this at a different layer with the new lookup, so treat @@ -221,12 +228,13 @@ class MatchingLinkResult { if (compareThis is TypeParameter && compareOther == null) { return true; } + return false; } @override String toString() { - return 'element: [${modelElement is Constructor ? 'new ' : ''}${modelElement?.fullyQualifiedName}] warn: $warn'; + return 'element: [${commentReferable is Constructor ? 'new ' : ''}${commentReferable?.fullyQualifiedName}] warn: $warn'; } } @@ -354,12 +362,6 @@ MatchingLinkResult _getMatchingLinkElementCommentReferable( var lookupResult = warnable.referenceBy(commentReference.referenceBy, filter: filter); - // TODO(jcollins-g): Referring to packages or other non-[ModelElement]s - // might be needed here. Determine if that's the case. - if (!(lookupResult is ModelElement)) { - lookupResult = null; - } - // TODO(jcollins-g): Consider prioritizing analyzer resolution before custom. return MatchingLinkResult(lookupResult); } @@ -1001,11 +1003,11 @@ const _referenceLookupWarnings = { md.Node _makeLinkNode(String codeRef, Warnable warnable) { var result = getMatchingLinkElement(warnable, codeRef); var textContent = htmlEscape.convert(codeRef); - var linkedElement = result.modelElement; + var linkedElement = result.commentReferable; if (linkedElement != null) { if (linkedElement.href != null) { var anchor = md.Element.text('a', textContent); - if (linkedElement.isDeprecated) { + if (linkedElement is ModelElement && linkedElement.isDeprecated) { anchor.attributes['class'] = 'deprecated'; } anchor.attributes['href'] = linkedElement.href; @@ -1041,11 +1043,11 @@ MatchingLinkResult getMatchingLinkElement(Warnable warnable, String codeRef, if (doComparison) { resultNew = _getMatchingLinkElementCommentReferable(codeRef, warnable); resultOld = _getMatchingLinkElementLegacy(codeRef, warnable); - if (resultNew.modelElement != null) { + if (resultNew.commentReferable != null) { markdownStats.resolvedNewLookupReferences++; } result = experimentalReferenceLookup ? resultNew : resultOld; - if (resultOld.modelElement != null) { + if (resultOld.commentReferable != null) { markdownStats.resolvedOldLookupReferences++; } } else { @@ -1059,12 +1061,13 @@ MatchingLinkResult getMatchingLinkElement(Warnable warnable, String codeRef, if (resultOld.isEquivalentTo(resultNew)) { markdownStats.resolvedEquivalentlyReferences++; } else { - if (resultNew.modelElement == null && resultOld.modelElement != null) { + if (resultNew.commentReferable == null && + resultOld.commentReferable != null) { warnable.warn(PackageWarning.referenceLookupMissingWithNew, message: '[$codeRef] => ' + resultOld.toString(), referredFrom: warnable.documentationFrom); - } else if (resultNew.modelElement != null && - resultOld.modelElement == null) { + } else if (resultNew.commentReferable != null && + resultOld.commentReferable == null) { warnable.warn(PackageWarning.referenceLookupFoundWithNew, message: '[$codeRef] => ' + resultNew.toString(), referredFrom: warnable.documentationFrom); @@ -1077,7 +1080,7 @@ MatchingLinkResult getMatchingLinkElement(Warnable warnable, String codeRef, } } markdownStats.totalReferences++; - if (result.modelElement != null) markdownStats.resolvedReferences++; + if (result.commentReferable != null) markdownStats.resolvedReferences++; return result; } @@ -1095,16 +1098,21 @@ final RegExp allAfterLastNewline = RegExp(r'\n.*$', multiLine: true); // https://github.com/dart-lang/dartdoc/issues/1250#issuecomment-269257942 void showWarningsForGenericsOutsideSquareBracketsBlocks( String text, Warnable element) { - for (var position in findFreeHangingGenericsPositions(text)) { - var priorContext = - '${text.substring(max(position - maxPriorContext, 0), position)}'; - var postContext = - '${text.substring(position, min(position + maxPostContext, text.length))}'; - priorContext = priorContext.replaceAll(allBeforeFirstNewline, ''); - postContext = postContext.replaceAll(allAfterLastNewline, ''); - var errorMessage = '$priorContext$postContext'; - // TODO(jcollins-g): allow for more specific error location inside comments - element.warn(PackageWarning.typeAsHtml, message: errorMessage); + // Skip this if not warned for performance and for dart-lang/sdk#46419. + if (element.config.packageWarningOptions + .warningModes[PackageWarning.typeAsHtml] != + PackageWarningMode.ignore) { + for (var position in findFreeHangingGenericsPositions(text)) { + var priorContext = + '${text.substring(max(position - maxPriorContext, 0), position)}'; + var postContext = + '${text.substring(position, min(position + maxPostContext, text.length))}'; + priorContext = priorContext.replaceAll(allBeforeFirstNewline, ''); + postContext = postContext.replaceAll(allAfterLastNewline, ''); + var errorMessage = '$priorContext$postContext'; + // TODO(jcollins-g): allow for more specific error location inside comments + element.warn(PackageWarning.typeAsHtml, message: errorMessage); + } } } diff --git a/lib/src/model/comment_referable.dart b/lib/src/model/comment_referable.dart index 53523f3925..e11e0bced9 100644 --- a/lib/src/model/comment_referable.dart +++ b/lib/src/model/comment_referable.dart @@ -42,6 +42,8 @@ mixin CommentReferable implements Nameable { /// lookups via [referenceChildren]. Can be cached. Scope get scope => null; + String get href => null; + /// Look up a comment reference by its component parts. If [tryParents] is /// true, try looking up the same reference in any parents of [this]. /// Will skip over results that do not pass a given [filter] and keep diff --git a/lib/src/model/package_graph.dart b/lib/src/model/package_graph.dart index f8cadf2804..a3331d36d6 100644 --- a/lib/src/model/package_graph.dart +++ b/lib/src/model/package_graph.dart @@ -1031,7 +1031,8 @@ class PackageGraph with CommentReferable, Nameable { if (_referenceChildren == null) { _referenceChildren = {}; // Packages are the top priority. - _referenceChildren.addEntries(packages.map((p) => MapEntry(p.name, p))); + _referenceChildren + .addEntries(packages.map((p) => MapEntry('package:${p.name}', p))); // Libraries are next. // TODO(jcollins-g): Warn about directly referencing libraries out of diff --git a/test/comment_referable/parser_test.dart b/test/comment_referable/parser_test.dart index eb4155be1b..b716408be7 100644 --- a/test/comment_referable/parser_test.dart +++ b/test/comment_referable/parser_test.dart @@ -47,6 +47,7 @@ void main() { expectParseEquivalent('this.is.valid<>', ['this', 'is', 'valid']); expectParseEquivalent('this.is.valid', ['this', 'is', 'valid']); expectParseEquivalent('this.is.valid(things)', ['this', 'is', 'valid']); + expectParseEquivalent('\nthis.is.valid', ['this', 'is', 'valid']); }); test('Check that cases dependent on prefix resolution not firing parse', diff --git a/test/end2end/model_test.dart b/test/end2end/model_test.dart index bf7d14fad8..e8fe859dcd 100644 --- a/test/end2end/model_test.dart +++ b/test/end2end/model_test.dart @@ -2129,10 +2129,10 @@ void main() { /// as the original lookup code returns canonicalized results and the /// new lookup code is only guaranteed to return equivalent results. MatchingLinkResult definingLinkResult(MatchingLinkResult originalResult) { - if (originalResult.modelElement != null) { + if (originalResult.commentReferable?.element != null) { return MatchingLinkResult( - ModelElement.fromElement(originalResult.modelElement.element, - originalResult.modelElement.packageGraph), + ModelElement.fromElement(originalResult.commentReferable.element, + originalResult.commentReferable.packageGraph), warn: originalResult.warn); } return originalResult; @@ -2228,7 +2228,10 @@ void main() { }); test('Linking for inherited field from reexport context', () { - expect(bothLookup(aField, 'anotherNotReexportedVariable'), + // While this can work in the old code at times, it is using the + // analyzer and the new test harness can't leverage the analyzer + // when testing the old lookup code. + expect(newLookup(aField, 'anotherNotReexportedVariable'), equals(MatchingLinkResult(anotherNotReexportedVariable))); }); @@ -2250,6 +2253,8 @@ void main() { }); group('Ordinary namespace cases', () { + Package DartPackage; + Library Dart; ModelFunction doesStuff, function1, topLevelFunction; TopLevelVariable incorrectDocReference, incorrectDocReferenceFromEx, @@ -2276,6 +2281,9 @@ void main() { aConstructorShadowedField; setUpAll(() async { + Dart = packageGraph.allLibraries.values + .firstWhere((l) => l.name == 'Dart'); + DartPackage = packageGraph.packages.firstWhere((p) => p.name == 'Dart'); nameWithTwoUnderscores = fakeLibrary.constants .firstWhere((v) => v.name == 'NAME_WITH_TWO_UNDERSCORES'); nameWithSingleUnderscore = fakeLibrary.constants @@ -2348,6 +2356,14 @@ void main() { .firstWhere((f) => f.name == 'aConstructorShadowed'); }); + test('Referring to libraries and packages with the same name is fine', + () { + expect(bothLookup(Apple, 'Dart'), equals(MatchingLinkResult(Dart))); + // New feature: allow disambiguation if you really want to specify a package. + expect(newLookup(Apple, 'package:Dart'), + equals(MatchingLinkResult(DartPackage))); + }); + test('Verify basic linking inside a constructor', () { // Field formal parameters worked sometimes by accident in the old code, // but should work reliably now. @@ -2378,7 +2394,10 @@ void main() { test('Deprecated lookup styles still function', () { // dart-lang/dartdoc#2683 - expect(bothLookup(baseForDocComments, 'aPrefix.UseResult'), + // We can't check the old code this way as this is one of the few + // cases in the old code where it relies on the analyzer's resolution of + // the doc comment -- the test harness can't do that for the old code. + expect(newLookup(baseForDocComments, 'aPrefix.UseResult'), equals(MatchingLinkResult(metaUseResult))); }); diff --git a/test/src/utils.dart b/test/src/utils.dart index e3e87d75ea..779fc77839 100644 --- a/test/src/utils.dart +++ b/test/src/utils.dart @@ -19,7 +19,7 @@ import 'package:pub_semver/pub_semver.dart'; /// The number of public libraries in testing/test_package, minus 2 for /// the excluded libraries listed in the initializers for _testPackageGraphMemo /// and minus 1 for the tag in the 'excluded' library. -const int kTestPackagePublicLibraries = 23; +const int kTestPackagePublicLibraries = 24; final _resourceProvider = pubPackageMetaProvider.resourceProvider; final _pathContext = _resourceProvider.pathContext; diff --git a/testing/test_package/lib/dart.dart b/testing/test_package/lib/dart.dart new file mode 100644 index 0000000000..76402ba6a8 --- /dev/null +++ b/testing/test_package/lib/dart.dart @@ -0,0 +1,2 @@ +/// A bold name for a library to test comment reference resolution. +library Dart; \ No newline at end of file