From 13ddc738230ef70b1e21ffa2ec4134f822eb2de8 Mon Sep 17 00:00:00 2001 From: Janice Collins Date: Tue, 15 Jun 2021 15:41:11 -0700 Subject: [PATCH] Implement package global and graph global lookups --- lib/src/comment_references/parser.dart | 21 +++++++--- lib/src/markdown_processor.dart | 27 ++++++++++--- lib/src/model/library.dart | 18 +++++++-- lib/src/model/package.dart | 1 + lib/src/model/package_graph.dart | 51 ++++++++++++++++--------- test/comment_referable/parser_test.dart | 34 ++++++++++++----- test/end2end/model_test.dart | 32 +++++++++++++--- testing/test_package/lib/fake.dart | 5 ++- 8 files changed, 139 insertions(+), 50 deletions(-) diff --git a/lib/src/comment_references/parser.dart b/lib/src/comment_references/parser.dart index e0b2a7f742..28f558edca 100644 --- a/lib/src/comment_references/parser.dart +++ b/lib/src/comment_references/parser.dart @@ -142,7 +142,6 @@ class CommentReferenceParser { } static const _constructorHintPrefix = 'new'; - static const _ignorePrefixes = ['const', 'final', 'var']; /// Implement parsing a prefix to a comment reference. @@ -153,17 +152,19 @@ class CommentReferenceParser { /// /// ::= 'new ' /// - /// ::= ('const' | 'final' | 'var' | 'operator')(' '+) + /// ::= ('const' | 'final' | 'var')(' '+) /// ``` _PrefixParseResult _parsePrefix() { if (_atEnd) { return _PrefixParseResult.endOfFile; } - if (_tryMatchLiteral(_constructorHintPrefix)) { + if (_tryMatchLiteral(_constructorHintPrefix, + requireTrailingNonidentifier: true)) { return _PrefixParseResult.ok( ConstructorHintStartNode(_constructorHintPrefix)); } - if (_ignorePrefixes.any((p) => _tryMatchLiteral(p))) { + if (_ignorePrefixes + .any((p) => _tryMatchLiteral(p, requireTrailingNonidentifier: true))) { return _PrefixParseResult.junk; } @@ -278,10 +279,12 @@ class CommentReferenceParser { /// Advances [_index] on match, preserves on non-match. bool _tryMatchLiteral(String characters, - {bool acceptTrailingWhitespace = true}) { + {bool acceptTrailingWhitespace = true, + bool requireTrailingNonidentifier = false}) { assert(acceptTrailingWhitespace != null); if (characters.length + _index > _referenceLength) return false; - for (var startIndex = _index; + int startIndex; + for (startIndex = _index; _index - startIndex < characters.length; _index++) { if (codeRef.codeUnitAt(_index) != @@ -290,6 +293,12 @@ class CommentReferenceParser { return false; } } + if (requireTrailingNonidentifier) { + if (_atEnd || !_nonIdentifierChars.contains(_thisChar)) { + _index = startIndex; + return false; + } + } if (acceptTrailingWhitespace) _walkPastWhitespace(); return true; } diff --git a/lib/src/markdown_processor.dart b/lib/src/markdown_processor.dart index b13118ffef..adde5ea7ab 100644 --- a/lib/src/markdown_processor.dart +++ b/lib/src/markdown_processor.dart @@ -191,23 +191,34 @@ class MatchingLinkResult { bool isEquivalentTo(MatchingLinkResult other) { if (this == other) return true; - if (modelElement?.canonicalModelElement == - other.modelElement?.canonicalModelElement) return true; + var compareThis = modelElement; + var compareOther = other.modelElement; + + if (compareThis is Accessor) { + compareThis = (compareThis as Accessor).enclosingCombo; + } + + if (compareOther is Accessor) { + compareOther = (compareOther as Accessor).enclosingCombo; + } + + if (compareThis?.canonicalModelElement == + compareOther?.canonicalModelElement) 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 // this as equivalent to a null type. - if (other.modelElement is Parameter && modelElement == null) { + if (compareOther is Parameter && compareThis == null) { return true; } - if (modelElement is Parameter && other.modelElement == null) { + if (compareThis is Parameter && compareOther == null) { return true; } // Same with TypeParameter. - if (other.modelElement is TypeParameter && modelElement == null) { + if (compareOther is TypeParameter && compareThis == null) { return true; } - if (modelElement is TypeParameter && other.modelElement == null) { + if (compareThis is TypeParameter && compareOther == null) { return true; } return false; @@ -290,6 +301,10 @@ bool _rejectDefaultConstructors(CommentReferable referable) { referable.name == referable.enclosingElement.name) { return false; } + // Avoid accidentally preferring arguments of the default constructor. + if (referable is ModelElement && referable.enclosingElement is Constructor) { + return false; + } return true; } diff --git a/lib/src/model/library.dart b/lib/src/model/library.dart index cc153022c0..69ad3de4c9 100644 --- a/lib/src/model/library.dart +++ b/lib/src/model/library.dart @@ -662,9 +662,21 @@ class Library extends ModelElement with Categorization, TopLevelContainer { Map _referenceChildren; @override Map get referenceChildren { - return _referenceChildren ??= Map.fromEntries( - element.exportNamespace.definedNames.entries.map((entry) => MapEntry( - entry.key, ModelElement.fromElement(entry.value, packageGraph)))); + if (_referenceChildren == null) { + _referenceChildren = Map.fromEntries( + element.exportNamespace.definedNames.entries.map((entry) => MapEntry( + entry.key, ModelElement.fromElement(entry.value, packageGraph)))); + // TODO(jcollins-g): warn and get rid of this case where it shows up. + // If a user is hiding parts of a prefix import, the user should not + // refer to hidden members via the prefix, because that can be + // ambiguous. dart-lang/dartdoc#2683. + for (var prefixEntry in prefixToLibrary.entries) { + if (!_referenceChildren.containsKey(prefixEntry.key)) { + _referenceChildren[prefixEntry.key] = prefixEntry.value.first; + } + } + } + return _referenceChildren; } @override diff --git a/lib/src/model/package.dart b/lib/src/model/package.dart index f4f8fa35ec..1c07d4e721 100644 --- a/lib/src/model/package.dart +++ b/lib/src/model/package.dart @@ -414,6 +414,7 @@ class Package extends LibraryContainer // lookups like this. for (var lib in publicLibrariesSorted) { for (var referableEntry in lib.referenceChildren.entries) { + // Avoiding tearoffs for performance reasons. if (!_referenceChildren.containsKey(referableEntry.key)) { _referenceChildren[referableEntry.key] = referableEntry.value; } diff --git a/lib/src/model/package_graph.dart b/lib/src/model/package_graph.dart index fed4c27b9f..f8cadf2804 100644 --- a/lib/src/model/package_graph.dart +++ b/lib/src/model/package_graph.dart @@ -1030,26 +1030,39 @@ class PackageGraph with CommentReferable, Nameable { Map get referenceChildren { if (_referenceChildren == null) { _referenceChildren = {}; - + // Packages are the top priority. _referenceChildren.addEntries(packages.map((p) => MapEntry(p.name, p))); - // TODO(jcollins-g): deprecate and start warning for anything needing these - // elements. - var librariesWithCanonicals = - packages.expand((p) => referenceChildren.entries.where((e) { - var v = e.value; - return v is ModelElement - ? v.canonicalModelElement != null - : true; - })); - var topLevelsWithCanonicals = librariesWithCanonicals - .expand((p) => referenceChildren.entries.where((e) { - var v = e.value; - return v is ModelElement - ? v.canonicalModelElement != null - : true; - })); - _referenceChildren.addEntries(librariesWithCanonicals); - _referenceChildren.addEntries(topLevelsWithCanonicals); + + // Libraries are next. + // TODO(jcollins-g): Warn about directly referencing libraries out of + // scope? + for (var p in documentedPackages) { + for (var lib in p.publicLibrariesSorted) { + if (!_referenceChildren.containsKey(lib.name)) { + _referenceChildren[lib.name] = lib; + } + } + } + // TODO(jcollins-g): Warn about directly referencing top level items + // out of scope? + for (var p in documentedPackages) { + for (var lib in p.publicLibrariesSorted) { + for (var me in [ + ...lib.publicConstants, + ...lib.publicFunctions, + ...lib.publicProperties, + ...lib.publicTypedefs, + ...lib.publicExtensions, + ...lib.publicClasses, + ...lib.publicEnums, + ...lib.publicMixins + ]) { + if (!_referenceChildren.containsKey(me.name)) { + _referenceChildren[me.name] = me; + } + } + } + } } return _referenceChildren; } diff --git a/test/comment_referable/parser_test.dart b/test/comment_referable/parser_test.dart index c6f11a5dcd..eb4155be1b 100644 --- a/test/comment_referable/parser_test.dart +++ b/test/comment_referable/parser_test.dart @@ -7,33 +7,36 @@ import 'package:test/test.dart'; void main() { void expectParseEquivalent(String codeRef, List parts, - {bool constructorHint = false}) { + {bool constructorHint = false, bool callableHint = false}) { var result = CommentReferenceParser(codeRef).parse(); - var hasHint = result.isNotEmpty && - (result.first is ConstructorHintStartNode || - result.last is CallableHintEndNode); + var hasConstructorHint = + result.isNotEmpty && result.first is ConstructorHintStartNode; + var hasCallableHint = + result.isNotEmpty && result.last is CallableHintEndNode; var stringParts = result.whereType().map((i) => i.text); expect(stringParts, equals(parts)); - expect(hasHint, equals(constructorHint)); + expect(hasConstructorHint, equals(constructorHint)); + expect(hasCallableHint, equals(callableHint)); } void expectParsePassthrough(String codeRef) => expectParseEquivalent(codeRef, [codeRef]); void expectParseError(String codeRef) { - expect(CommentReferenceParser(codeRef).parse(), isEmpty); + expect(CommentReferenceParser(codeRef).parse().whereType(), + isEmpty); } group('Basic comment reference parsing', () { test('Check that basic references parse', () { expectParseEquivalent('valid', ['valid']); expectParseEquivalent('new valid', ['valid'], constructorHint: true); - expectParseEquivalent('valid()', ['valid'], constructorHint: true); - expectParseEquivalent('const valid()', ['valid'], constructorHint: true); + expectParseEquivalent('valid()', ['valid'], callableHint: true); + expectParseEquivalent('const valid()', ['valid'], callableHint: true); expectParseEquivalent('final valid', ['valid']); expectParseEquivalent('this.is.valid', ['this', 'is', 'valid']); expectParseEquivalent('this.is.valid()', ['this', 'is', 'valid'], - constructorHint: true); + callableHint: true); expectParseEquivalent('new this.is.valid', ['this', 'is', 'valid'], constructorHint: true); expectParseEquivalent('const this.is.valid', ['this', 'is', 'valid']); @@ -46,6 +49,19 @@ void main() { expectParseEquivalent('this.is.valid(things)', ['this', 'is', 'valid']); }); + test('Check that cases dependent on prefix resolution not firing parse', + () { + expectParsePassthrough('constant'); + expectParsePassthrough('newThingy'); + expectParsePassthrough('operatorThingy'); + expectParseEquivalent('operator+', ['+']); + expectParseError('const()'); + // TODO(jcollins-g): might need to revisit these two with constructor + // tearoffs? + expectParsePassthrough('new'); + expectParseError('new()'); + }); + test('Check that operator references parse', () { expectParsePassthrough('[]'); expectParsePassthrough('<='); diff --git a/test/end2end/model_test.dart b/test/end2end/model_test.dart index 776f0ffa6b..57b2db59a7 100644 --- a/test/end2end/model_test.dart +++ b/test/end2end/model_test.dart @@ -2137,12 +2137,17 @@ void main() { nameWithSingleUnderscore, theOnlyThingInTheLibrary; Constructor aNonDefaultConstructor, defaultConstructor; - Class Apple, BaseClass, baseForDocComments, ExtraSpecialList, string; + Class Apple, + BaseClass, + baseForDocComments, + ExtraSpecialList, + string, + metaUseResult; Method doAwesomeStuff, anotherMethod; // ignore: unused_local_variable Operator bracketOperator, bracketOperatorOtherClass; Parameter doAwesomeStuffParam; - Field forInheriting, action, initializeMe; + Field forInheriting, action, initializeMe, somethingShadowy; setUpAll(() async { nameWithTwoUnderscores = fakeLibrary.constants @@ -2153,6 +2158,10 @@ void main() { .firstWhere((e) => e.name == 'dart:core') .allClasses .firstWhere((c) => c.name == 'String'); + metaUseResult = packageGraph.allLibraries.values + .firstWhere((e) => e.name == 'meta') + .allClasses + .firstWhere((c) => c.name == 'UseResult'); baseForDocComments = fakeLibrary.classes.firstWhere((c) => c.name == 'BaseForDocComments'); aNonDefaultConstructor = baseForDocComments.constructors.firstWhere( @@ -2161,6 +2170,8 @@ void main() { .firstWhere((c) => c.name == 'BaseForDocComments'); initializeMe = baseForDocComments.allFields .firstWhere((f) => f.name == 'initializeMe'); + somethingShadowy = baseForDocComments.allFields + .firstWhere((f) => f.name == 'somethingShadowy'); doAwesomeStuff = baseForDocComments.instanceMethods .firstWhere((m) => m.name == 'doAwesomeStuff'); anotherMethod = baseForDocComments.instanceMethods @@ -2248,12 +2259,23 @@ void main() { equals(MatchingLinkResult(aNonDefaultConstructor))); }); + test('Deprecated lookup styles still function', () { + // dart-lang/dartdoc#2683 + expect(bothLookup(baseForDocComments, 'aPrefix.UseResult'), + equals(MatchingLinkResult(metaUseResult))); + }); + test('Verify basic linking inside class', () { expect( bothLookup( baseForDocComments, 'BaseForDocComments.BaseForDocComments'), equals(MatchingLinkResult(defaultConstructor))); + // We don't want the parameter on the default constructor, here. + expect( + bothLookup(baseForDocComments, 'BaseForDocComments.somethingShadowy'), + equals(MatchingLinkResult(somethingShadowy))); + expect(bothLookup(doAwesomeStuff, 'aNonDefaultConstructor'), equals(MatchingLinkResult(aNonDefaultConstructor))); @@ -2311,8 +2333,7 @@ void main() { equals(MatchingLinkResult(theOnlyThingInTheLibrary))); // A name that exists in this package but is not imported. - // TODO(jcollins-g): package-wide lookups are not yet implemented with the new lookup code. - expect(originalLookup(doAwesomeStuff, 'doesStuff'), + expect(bothLookup(doAwesomeStuff, 'doesStuff'), equals(MatchingLinkResult(doesStuff))); // A name of a class from an import of a library that exported that name. @@ -2339,8 +2360,7 @@ void main() { equals(MatchingLinkResult(forInheriting))); // Reference to an inherited member in another library via class name. - // TODO(jcollins-g): reference to non-imported symbols isn't implemented yet in new lookup. - expect(originalLookup(doAwesomeStuff, 'ExtendedBaseReexported.action'), + expect(bothLookup(doAwesomeStuff, 'ExtendedBaseReexported.action'), equals(MatchingLinkResult(action))); }); }); diff --git a/testing/test_package/lib/fake.dart b/testing/test_package/lib/fake.dart index b7b3678d13..ed907f458d 100644 --- a/testing/test_package/lib/fake.dart +++ b/testing/test_package/lib/fake.dart @@ -55,6 +55,7 @@ import 'dart:collection'; // ignore: uri_does_not_exist //import 'dart:json' as invalidPrefix; import 'package:meta/meta.dart' show Required; +import 'package:meta/meta.dart' as aPrefix show Immutable; import 'csspub.dart' as css; import 'csspub.dart' as renamedLib2; import 'package:test_package/src//import_unusual.dart'; @@ -962,7 +963,9 @@ class BaseForDocComments { final bool initializeMe; - BaseForDocComments(this.initializeMe); + int somethingShadowy; + + BaseForDocComments(this.initializeMe, [bool somethingShadowy]); BaseForDocComments.aNonDefaultConstructor(this.initializeMe);