diff --git a/lib/src/comment_references/model_comment_reference.dart b/lib/src/comment_references/model_comment_reference.dart index 4d044da89d..aa7eb0779c 100644 --- a/lib/src/comment_references/model_comment_reference.dart +++ b/lib/src/comment_references/model_comment_reference.dart @@ -15,6 +15,7 @@ abstract class ModelCommentReference { bool get allowDefaultConstructor; String get codeRef; bool get hasConstructorHint; + bool get hasCallableHint; List get referenceBy; Element get staticElement; @@ -32,22 +33,42 @@ abstract class ModelCommentReference { /// information needed for Dartdoc. Drops link to the [CommentReference] /// and [ResourceProvider] after construction. class _ModelCommentReferenceImpl implements ModelCommentReference { + bool _allowDefaultConstructor; @override bool get allowDefaultConstructor { - if (parsed.length >= 2) { - return parsed[parsed.length - 2] == parsed[parsed.length - 1]; + if (_allowDefaultConstructor == null) { + _allowDefaultConstructor = false; + var foundFirst = false; + String checkText; + // Check for two identically named identifiers at the end of the + // parse list, skipping over any junk or other nodes. + for (var node in parsed.reversed.whereType()) { + if (foundFirst) { + if (checkText == node.text) { + _allowDefaultConstructor = true; + } + break; + } else { + foundFirst = true; + checkText = node.text; + } + } } - return false; + return _allowDefaultConstructor; } @override final String codeRef; @override - bool get hasConstructorHint => + bool get hasCallableHint => parsed.isNotEmpty && (parsed.first is ConstructorHintStartNode || - parsed.last is ConstructorHintEndNode); + parsed.last is CallableHintEndNode); + + @override + bool get hasConstructorHint => + parsed.isNotEmpty && parsed.first is ConstructorHintStartNode; @override List get referenceBy => parsed diff --git a/lib/src/comment_references/parser.dart b/lib/src/comment_references/parser.dart index 626a61a5d7..e0b2a7f742 100644 --- a/lib/src/comment_references/parser.dart +++ b/lib/src/comment_references/parser.dart @@ -133,7 +133,7 @@ class CommentReferenceParser { if (suffixResult.type == _SuffixResultType.notSuffix) { // Invalid trailing junk; reject the reference. return []; - } else if (suffixResult.type == _SuffixResultType.parsedConstructorHint) { + } else if (suffixResult.type == _SuffixResultType.parsedCallableHint) { children.add(suffixResult.node); } @@ -234,10 +234,10 @@ class CommentReferenceParser { IdentifierNode(codeRef.substring(startIndex, _index))); } - static const _constructorHintSuffix = '()'; + static const _callableHintSuffix = '()'; /// ```text - /// ::= + /// ::= /// | /// /// ::= '<'*'>' @@ -246,7 +246,7 @@ class CommentReferenceParser { /// | '?' /// | '!' /// - /// ::= '()' + /// ::= '()' /// ``` _SuffixParseResult _parseSuffix() { var startIndex = _index; @@ -254,10 +254,10 @@ class CommentReferenceParser { if (_atEnd) { return _SuffixParseResult.missing; } - if (_tryMatchLiteral(_constructorHintSuffix)) { + if (_tryMatchLiteral(_callableHintSuffix)) { if (_atEnd) { return _SuffixParseResult.ok( - ConstructorHintEndNode(codeRef.substring(startIndex, _index))); + CallableHintEndNode(codeRef.substring(startIndex, _index))); } return _SuffixParseResult.notSuffix; } @@ -386,10 +386,10 @@ enum _SuffixResultType { junk, // Found known types of junk it is OK to ignore. missing, // There is no suffix here. Same as EOF as this is a suffix. notSuffix, // Found something, but not a valid suffix. - parsedConstructorHint, // Parsed a [ConstructorHintEndNode]. + parsedCallableHint, // Parsed a [CallableHintEndNode]. } -/// The result of attempting to parse a prefix to a comment reference. +/// The result of attempting to parse a suffix to a comment reference. class _SuffixParseResult { final _SuffixResultType type; @@ -398,7 +398,7 @@ class _SuffixParseResult { const _SuffixParseResult._(this.type, this.node); factory _SuffixParseResult.ok(CommentReferenceNode node) => - _SuffixParseResult._(_SuffixResultType.parsedConstructorHint, node); + _SuffixParseResult._(_SuffixResultType.parsedCallableHint, node); static const _SuffixParseResult junk = _SuffixParseResult._(_SuffixResultType.junk, null); @@ -426,14 +426,14 @@ class ConstructorHintStartNode extends CommentReferenceNode { String toString() => 'ConstructorHintStartNode["$text"]'; } -class ConstructorHintEndNode extends CommentReferenceNode { +class CallableHintEndNode extends CommentReferenceNode { @override final String text; - ConstructorHintEndNode(this.text); + CallableHintEndNode(this.text); @override - String toString() => 'ConstructorHintEndNode["$text"]'; + String toString() => 'CallableHintEndNode["$text"]'; } /// Represents an identifier. diff --git a/lib/src/generator/templates.runtime_renderers.dart b/lib/src/generator/templates.runtime_renderers.dart index 8bff603be9..e024f7adb9 100644 --- a/lib/src/generator/templates.runtime_renderers.dart +++ b/lib/src/generator/templates.runtime_renderers.dart @@ -2155,6 +2155,19 @@ class _Renderer_Class extends RendererBase { parent: r)); }, ), + 'referenceChildren': Property( + getValue: (CT_ c) => c.referenceChildren, + renderVariable: (CT_ c, Property self, + List remainingNames) => + self.renderSimpleVariable( + c, remainingNames, 'Map'), + isNullValue: (CT_ c) => c.referenceChildren == null, + renderValue: (CT_ c, RendererBase r, + List ast, StringSink sink) { + renderSimple(c.referenceChildren, ast, r.template, sink, + parent: r, getters: _invisibleGetters['Map']); + }, + ), 'referenceParents': Property( getValue: (CT_ c) => c.referenceParents, renderVariable: (CT_ c, Property self, diff --git a/lib/src/markdown_processor.dart b/lib/src/markdown_processor.dart index 4ce5c3f030..b13118ffef 100644 --- a/lib/src/markdown_processor.dart +++ b/lib/src/markdown_processor.dart @@ -203,6 +203,13 @@ class MatchingLinkResult { if (modelElement is Parameter && other.modelElement == null) { return true; } + // Same with TypeParameter. + if (other.modelElement is TypeParameter && modelElement == null) { + return true; + } + if (modelElement is TypeParameter && other.modelElement == null) { + return true; + } return false; } @@ -286,7 +293,12 @@ bool _rejectDefaultConstructors(CommentReferable referable) { return true; } -/// Return false unless the passed [referable] is a [Constructor]. +/// Return false unless the passed [referable] represents a callable object. +/// Allows constructors but does not require them. +bool _requireCallable(CommentReferable referable) => + referable is ModelElement && referable.isCallable; + +/// Return false unless the passed [referable] represents a constructor. bool _requireConstructor(CommentReferable referable) => referable is Constructor; @@ -295,17 +307,29 @@ MatchingLinkResult _getMatchingLinkElementCommentReferable( String codeRef, Warnable warnable) { var commentReference = warnable.commentRefs[codeRef] ?? ModelCommentReference.synthetic(codeRef); - var filter = commentReference.hasConstructorHint - ? _requireConstructor - : _rejectDefaultConstructors; - - /// Neither reject, nor require, a default constructor in the event - /// the comment reference structure implies one. (We can not require it - /// in case a library name is the same as a member class name and the class - /// is the intended lookup). + + bool Function(CommentReferable) filter; + if (commentReference.allowDefaultConstructor) { - filter = null; + // Neither reject, nor require, a default constructor in the event + // the comment reference structure implies one. (We can not require it + // in case a library name is the same as a member class name and the class + // is the intended lookup). + filter = commentReference.hasCallableHint ? _requireCallable : null; + } else if (commentReference.hasConstructorHint && + commentReference.hasCallableHint) { + // This takes precedence over the callable hint if both are present -- + // pick a constructor and only constructor if we see `new`. + filter = _requireConstructor; + } else if (commentReference.hasCallableHint) { + // Trailing parens indicate we are looking for a callable. + filter = _requireCallable; + } else { + // Without hints, reject default constructors to force resolution to the + // class. + filter = _rejectDefaultConstructors; } + var lookupResult = warnable.referenceBy(commentReference.referenceBy, filter: filter); diff --git a/lib/src/model/class.dart b/lib/src/model/class.dart index 01c6bdc2a5..620851c065 100644 --- a/lib/src/model/class.dart +++ b/lib/src/model/class.dart @@ -605,6 +605,22 @@ class Class extends Container @override Iterable get constantFields => allFields.where((f) => f.isConst); + Map _referenceChildren; + @override + Map get referenceChildren { + if (_referenceChildren == null) { + _referenceChildren = super.referenceChildren; + for (var constructor in constructors) { + // For default constructors this is a no-op, but other constructors + // sometimes have a prefix attached to them in their "real name". + // TODO(jcollins-g): consider disallowing that, and only using + // the element name as here? + _referenceChildren[constructor.element.name] = constructor; + } + } + return _referenceChildren; + } + @override Iterable get referenceParents => [ ...super.referenceParents, diff --git a/lib/src/model/constructor.dart b/lib/src/model/constructor.dart index a572b3d699..fb35dcf2b9 100644 --- a/lib/src/model/constructor.dart +++ b/lib/src/model/constructor.dart @@ -127,8 +127,16 @@ class Constructor extends ModelElement Map get referenceChildren { if (_referenceChildren == null) { _referenceChildren = {}; - _referenceChildren - .addEntries(allParameters.map((p) => MapEntry(p.name, p))); + for (var param in allParameters) { + var paramElement = param.element; + if (paramElement is FieldFormalParameterElement) { + var fieldFormal = + ModelElement.fromElement(paramElement.field, packageGraph); + _referenceChildren[paramElement.name] = fieldFormal; + } else { + _referenceChildren[param.name] = param; + } + } _referenceChildren .addEntries(typeParameters.map((p) => MapEntry(p.name, p))); } diff --git a/lib/src/model/container.dart b/lib/src/model/container.dart index 9f2ccc1d22..c798c60e23 100644 --- a/lib/src/model/container.dart +++ b/lib/src/model/container.dart @@ -262,7 +262,18 @@ abstract class Container extends ModelElement with TypeParameters { if (_referenceChildren == null) { _referenceChildren = {}; for (var modelElement in allModelElements) { + // Never directly look up accessors. if (modelElement is Accessor) continue; + if (modelElement is Constructor) { + // Populate default constructor names so they make sense for the + // new lookup code. + var constructorName = modelElement.element.name; + if (constructorName == '') { + constructorName = name; + } + _referenceChildren[constructorName] = modelElement; + continue; + } if (modelElement is Operator) { // TODO(jcollins-g): once todo in [Operator.name] is fixed, remove // this special case. @@ -270,6 +281,10 @@ abstract class Container extends ModelElement with TypeParameters { } else { _referenceChildren[modelElement.name] = modelElement; } + } + // Process unscoped parameters last to make sure they don't override + // other options. + for (var modelElement in allModelElements) { // Don't complain about references to parameter names, but prefer // referring to anything else. // TODO(jcollins-g): Figure out something good to do in the ecosystem diff --git a/test/comment_referable/parser_test.dart b/test/comment_referable/parser_test.dart index d29ade35cb..c6f11a5dcd 100644 --- a/test/comment_referable/parser_test.dart +++ b/test/comment_referable/parser_test.dart @@ -11,7 +11,7 @@ void main() { var result = CommentReferenceParser(codeRef).parse(); var hasHint = result.isNotEmpty && (result.first is ConstructorHintStartNode || - result.last is ConstructorHintEndNode); + result.last is CallableHintEndNode); var stringParts = result.whereType().map((i) => i.text); expect(stringParts, equals(parts)); expect(hasHint, equals(constructorHint)); diff --git a/test/end2end/model_test.dart b/test/end2end/model_test.dart index d23abdf6ef..776f0ffa6b 100644 --- a/test/end2end/model_test.dart +++ b/test/end2end/model_test.dart @@ -2136,12 +2136,13 @@ void main() { nameWithTwoUnderscores, nameWithSingleUnderscore, theOnlyThingInTheLibrary; + Constructor aNonDefaultConstructor, defaultConstructor; Class Apple, BaseClass, baseForDocComments, ExtraSpecialList, string; Method doAwesomeStuff, anotherMethod; // ignore: unused_local_variable Operator bracketOperator, bracketOperatorOtherClass; Parameter doAwesomeStuffParam; - Field forInheriting, action; + Field forInheriting, action, initializeMe; setUpAll(() async { nameWithTwoUnderscores = fakeLibrary.constants @@ -2154,6 +2155,12 @@ void main() { .firstWhere((c) => c.name == 'String'); baseForDocComments = fakeLibrary.classes.firstWhere((c) => c.name == 'BaseForDocComments'); + aNonDefaultConstructor = baseForDocComments.constructors.firstWhere( + (c) => c.name == 'BaseForDocComments.aNonDefaultConstructor'); + defaultConstructor = baseForDocComments.constructors + .firstWhere((c) => c.name == 'BaseForDocComments'); + initializeMe = baseForDocComments.allFields + .firstWhere((f) => f.name == 'initializeMe'); doAwesomeStuff = baseForDocComments.instanceMethods .firstWhere((m) => m.name == 'doAwesomeStuff'); anotherMethod = baseForDocComments.instanceMethods @@ -2228,7 +2235,33 @@ void main() { return newLookupResult; } + test('Verify basic linking inside a constructor', () { + // Field formal parameters worked sometimes by accident in the old code, + // but should work reliably now. + expect(newLookup(aNonDefaultConstructor, 'initializeMe'), + equals(MatchingLinkResult(initializeMe))); + expect(newLookup(aNonDefaultConstructor, 'aNonDefaultConstructor'), + equals(MatchingLinkResult(aNonDefaultConstructor))); + expect( + bothLookup(aNonDefaultConstructor, + 'BaseForDocComments.aNonDefaultConstructor'), + equals(MatchingLinkResult(aNonDefaultConstructor))); + }); + test('Verify basic linking inside class', () { + expect( + bothLookup( + baseForDocComments, 'BaseForDocComments.BaseForDocComments'), + equals(MatchingLinkResult(defaultConstructor))); + + expect(bothLookup(doAwesomeStuff, 'aNonDefaultConstructor'), + equals(MatchingLinkResult(aNonDefaultConstructor))); + + expect( + bothLookup( + doAwesomeStuff, 'BaseForDocComments.aNonDefaultConstructor'), + equals(MatchingLinkResult(aNonDefaultConstructor))); + expect(bothLookup(doAwesomeStuff, 'this'), equals(MatchingLinkResult(baseForDocComments))); @@ -2287,7 +2320,6 @@ void main() { equals(MatchingLinkResult(BaseClass))); // A bracket operator within this class. - // TODO(jcollins-g): operator lookups not yet implemented with the new lookup code. expect(bothLookup(doAwesomeStuff, 'operator []'), equals(MatchingLinkResult(bracketOperator))); diff --git a/testing/test_package/lib/fake.dart b/testing/test_package/lib/fake.dart index 5b94160ec3..b7b3678d13 100644 --- a/testing/test_package/lib/fake.dart +++ b/testing/test_package/lib/fake.dart @@ -960,8 +960,12 @@ class BaseForDocComments { String operator [](String key) => "${key}'s value"; + final bool initializeMe; + + BaseForDocComments(this.initializeMe); + + BaseForDocComments.aNonDefaultConstructor(this.initializeMe); - BaseForDocComments(); factory BaseForDocComments.aFactoryFunction() => null; } @@ -981,6 +985,8 @@ enum MacrosFromAccessors { /// Testing if docs for inherited method are correct. /// {@category NotSoExcellent} class SubForDocComments extends BaseForDocComments { + SubForDocComments(bool thing) : super(thing); + /// Reference to [foo] and [bar] void localMethod(String foo, bar) {}