Skip to content

Commit 9ef58f2

Browse files
authored
Fix constructors and type parameters in the new lookup code (#2682)
* Handle different name styles in constructor * dartfmt * Fixes seem to work. * Rebuild * rebuild post-merge * fix analysis error in fake
1 parent b6bba5b commit 9ef58f2

File tree

10 files changed

+168
-33
lines changed

10 files changed

+168
-33
lines changed

lib/src/comment_references/model_comment_reference.dart

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ abstract class ModelCommentReference {
1515
bool get allowDefaultConstructor;
1616
String get codeRef;
1717
bool get hasConstructorHint;
18+
bool get hasCallableHint;
1819
List<String> get referenceBy;
1920
Element get staticElement;
2021

@@ -32,22 +33,42 @@ abstract class ModelCommentReference {
3233
/// information needed for Dartdoc. Drops link to the [CommentReference]
3334
/// and [ResourceProvider] after construction.
3435
class _ModelCommentReferenceImpl implements ModelCommentReference {
36+
bool _allowDefaultConstructor;
3537
@override
3638
bool get allowDefaultConstructor {
37-
if (parsed.length >= 2) {
38-
return parsed[parsed.length - 2] == parsed[parsed.length - 1];
39+
if (_allowDefaultConstructor == null) {
40+
_allowDefaultConstructor = false;
41+
var foundFirst = false;
42+
String checkText;
43+
// Check for two identically named identifiers at the end of the
44+
// parse list, skipping over any junk or other nodes.
45+
for (var node in parsed.reversed.whereType<IdentifierNode>()) {
46+
if (foundFirst) {
47+
if (checkText == node.text) {
48+
_allowDefaultConstructor = true;
49+
}
50+
break;
51+
} else {
52+
foundFirst = true;
53+
checkText = node.text;
54+
}
55+
}
3956
}
40-
return false;
57+
return _allowDefaultConstructor;
4158
}
4259

4360
@override
4461
final String codeRef;
4562

4663
@override
47-
bool get hasConstructorHint =>
64+
bool get hasCallableHint =>
4865
parsed.isNotEmpty &&
4966
(parsed.first is ConstructorHintStartNode ||
50-
parsed.last is ConstructorHintEndNode);
67+
parsed.last is CallableHintEndNode);
68+
69+
@override
70+
bool get hasConstructorHint =>
71+
parsed.isNotEmpty && parsed.first is ConstructorHintStartNode;
5172

5273
@override
5374
List<String> get referenceBy => parsed

lib/src/comment_references/parser.dart

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,7 @@ class CommentReferenceParser {
133133
if (suffixResult.type == _SuffixResultType.notSuffix) {
134134
// Invalid trailing junk; reject the reference.
135135
return [];
136-
} else if (suffixResult.type == _SuffixResultType.parsedConstructorHint) {
136+
} else if (suffixResult.type == _SuffixResultType.parsedCallableHint) {
137137
children.add(suffixResult.node);
138138
}
139139

@@ -234,10 +234,10 @@ class CommentReferenceParser {
234234
IdentifierNode(codeRef.substring(startIndex, _index)));
235235
}
236236

237-
static const _constructorHintSuffix = '()';
237+
static const _callableHintSuffix = '()';
238238

239239
/// ```text
240-
/// <suffix> ::= <constructorPostfixHint>
240+
/// <suffix> ::= <callableHintSuffix>
241241
/// | <trailingJunk>
242242
///
243243
/// <trailingJunk> ::= '<'<CHARACTER>*'>'
@@ -246,18 +246,18 @@ class CommentReferenceParser {
246246
/// | '?'
247247
/// | '!'
248248
///
249-
/// <constructorPostfixHint> ::= '()'
249+
/// <callableHintSuffix> ::= '()'
250250
/// ```
251251
_SuffixParseResult _parseSuffix() {
252252
var startIndex = _index;
253253
_walkPastWhitespace();
254254
if (_atEnd) {
255255
return _SuffixParseResult.missing;
256256
}
257-
if (_tryMatchLiteral(_constructorHintSuffix)) {
257+
if (_tryMatchLiteral(_callableHintSuffix)) {
258258
if (_atEnd) {
259259
return _SuffixParseResult.ok(
260-
ConstructorHintEndNode(codeRef.substring(startIndex, _index)));
260+
CallableHintEndNode(codeRef.substring(startIndex, _index)));
261261
}
262262
return _SuffixParseResult.notSuffix;
263263
}
@@ -386,10 +386,10 @@ enum _SuffixResultType {
386386
junk, // Found known types of junk it is OK to ignore.
387387
missing, // There is no suffix here. Same as EOF as this is a suffix.
388388
notSuffix, // Found something, but not a valid suffix.
389-
parsedConstructorHint, // Parsed a [ConstructorHintEndNode].
389+
parsedCallableHint, // Parsed a [CallableHintEndNode].
390390
}
391391

392-
/// The result of attempting to parse a prefix to a comment reference.
392+
/// The result of attempting to parse a suffix to a comment reference.
393393
class _SuffixParseResult {
394394
final _SuffixResultType type;
395395

@@ -398,7 +398,7 @@ class _SuffixParseResult {
398398
const _SuffixParseResult._(this.type, this.node);
399399

400400
factory _SuffixParseResult.ok(CommentReferenceNode node) =>
401-
_SuffixParseResult._(_SuffixResultType.parsedConstructorHint, node);
401+
_SuffixParseResult._(_SuffixResultType.parsedCallableHint, node);
402402

403403
static const _SuffixParseResult junk =
404404
_SuffixParseResult._(_SuffixResultType.junk, null);
@@ -426,14 +426,14 @@ class ConstructorHintStartNode extends CommentReferenceNode {
426426
String toString() => 'ConstructorHintStartNode["$text"]';
427427
}
428428

429-
class ConstructorHintEndNode extends CommentReferenceNode {
429+
class CallableHintEndNode extends CommentReferenceNode {
430430
@override
431431
final String text;
432432

433-
ConstructorHintEndNode(this.text);
433+
CallableHintEndNode(this.text);
434434

435435
@override
436-
String toString() => 'ConstructorHintEndNode["$text"]';
436+
String toString() => 'CallableHintEndNode["$text"]';
437437
}
438438

439439
/// Represents an identifier.

lib/src/generator/templates.runtime_renderers.dart

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2155,6 +2155,19 @@ class _Renderer_Class extends RendererBase<Class> {
21552155
parent: r));
21562156
},
21572157
),
2158+
'referenceChildren': Property(
2159+
getValue: (CT_ c) => c.referenceChildren,
2160+
renderVariable: (CT_ c, Property<CT_> self,
2161+
List<String> remainingNames) =>
2162+
self.renderSimpleVariable(
2163+
c, remainingNames, 'Map<String, CommentReferable>'),
2164+
isNullValue: (CT_ c) => c.referenceChildren == null,
2165+
renderValue: (CT_ c, RendererBase<CT_> r,
2166+
List<MustachioNode> ast, StringSink sink) {
2167+
renderSimple(c.referenceChildren, ast, r.template, sink,
2168+
parent: r, getters: _invisibleGetters['Map']);
2169+
},
2170+
),
21582171
'referenceParents': Property(
21592172
getValue: (CT_ c) => c.referenceParents,
21602173
renderVariable: (CT_ c, Property<CT_> self,

lib/src/markdown_processor.dart

Lines changed: 34 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -203,6 +203,13 @@ class MatchingLinkResult {
203203
if (modelElement is Parameter && other.modelElement == null) {
204204
return true;
205205
}
206+
// Same with TypeParameter.
207+
if (other.modelElement is TypeParameter && modelElement == null) {
208+
return true;
209+
}
210+
if (modelElement is TypeParameter && other.modelElement == null) {
211+
return true;
212+
}
206213
return false;
207214
}
208215

@@ -286,7 +293,12 @@ bool _rejectDefaultConstructors(CommentReferable referable) {
286293
return true;
287294
}
288295

289-
/// Return false unless the passed [referable] is a [Constructor].
296+
/// Return false unless the passed [referable] represents a callable object.
297+
/// Allows constructors but does not require them.
298+
bool _requireCallable(CommentReferable referable) =>
299+
referable is ModelElement && referable.isCallable;
300+
301+
/// Return false unless the passed [referable] represents a constructor.
290302
bool _requireConstructor(CommentReferable referable) =>
291303
referable is Constructor;
292304

@@ -295,17 +307,29 @@ MatchingLinkResult _getMatchingLinkElementCommentReferable(
295307
String codeRef, Warnable warnable) {
296308
var commentReference =
297309
warnable.commentRefs[codeRef] ?? ModelCommentReference.synthetic(codeRef);
298-
var filter = commentReference.hasConstructorHint
299-
? _requireConstructor
300-
: _rejectDefaultConstructors;
301-
302-
/// Neither reject, nor require, a default constructor in the event
303-
/// the comment reference structure implies one. (We can not require it
304-
/// in case a library name is the same as a member class name and the class
305-
/// is the intended lookup).
310+
311+
bool Function(CommentReferable) filter;
312+
306313
if (commentReference.allowDefaultConstructor) {
307-
filter = null;
314+
// Neither reject, nor require, a default constructor in the event
315+
// the comment reference structure implies one. (We can not require it
316+
// in case a library name is the same as a member class name and the class
317+
// is the intended lookup).
318+
filter = commentReference.hasCallableHint ? _requireCallable : null;
319+
} else if (commentReference.hasConstructorHint &&
320+
commentReference.hasCallableHint) {
321+
// This takes precedence over the callable hint if both are present --
322+
// pick a constructor and only constructor if we see `new`.
323+
filter = _requireConstructor;
324+
} else if (commentReference.hasCallableHint) {
325+
// Trailing parens indicate we are looking for a callable.
326+
filter = _requireCallable;
327+
} else {
328+
// Without hints, reject default constructors to force resolution to the
329+
// class.
330+
filter = _rejectDefaultConstructors;
308331
}
332+
309333
var lookupResult =
310334
warnable.referenceBy(commentReference.referenceBy, filter: filter);
311335

lib/src/model/class.dart

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -605,6 +605,22 @@ class Class extends Container
605605
@override
606606
Iterable<Field> get constantFields => allFields.where((f) => f.isConst);
607607

608+
Map<String, CommentReferable> _referenceChildren;
609+
@override
610+
Map<String, CommentReferable> get referenceChildren {
611+
if (_referenceChildren == null) {
612+
_referenceChildren = super.referenceChildren;
613+
for (var constructor in constructors) {
614+
// For default constructors this is a no-op, but other constructors
615+
// sometimes have a prefix attached to them in their "real name".
616+
// TODO(jcollins-g): consider disallowing that, and only using
617+
// the element name as here?
618+
_referenceChildren[constructor.element.name] = constructor;
619+
}
620+
}
621+
return _referenceChildren;
622+
}
623+
608624
@override
609625
Iterable<CommentReferable> get referenceParents => <CommentReferable>[
610626
...super.referenceParents,

lib/src/model/constructor.dart

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -127,8 +127,16 @@ class Constructor extends ModelElement
127127
Map<String, CommentReferable> get referenceChildren {
128128
if (_referenceChildren == null) {
129129
_referenceChildren = {};
130-
_referenceChildren
131-
.addEntries(allParameters.map((p) => MapEntry(p.name, p)));
130+
for (var param in allParameters) {
131+
var paramElement = param.element;
132+
if (paramElement is FieldFormalParameterElement) {
133+
var fieldFormal =
134+
ModelElement.fromElement(paramElement.field, packageGraph);
135+
_referenceChildren[paramElement.name] = fieldFormal;
136+
} else {
137+
_referenceChildren[param.name] = param;
138+
}
139+
}
132140
_referenceChildren
133141
.addEntries(typeParameters.map((p) => MapEntry(p.name, p)));
134142
}

lib/src/model/container.dart

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -262,14 +262,29 @@ abstract class Container extends ModelElement with TypeParameters {
262262
if (_referenceChildren == null) {
263263
_referenceChildren = {};
264264
for (var modelElement in allModelElements) {
265+
// Never directly look up accessors.
265266
if (modelElement is Accessor) continue;
267+
if (modelElement is Constructor) {
268+
// Populate default constructor names so they make sense for the
269+
// new lookup code.
270+
var constructorName = modelElement.element.name;
271+
if (constructorName == '') {
272+
constructorName = name;
273+
}
274+
_referenceChildren[constructorName] = modelElement;
275+
continue;
276+
}
266277
if (modelElement is Operator) {
267278
// TODO(jcollins-g): once todo in [Operator.name] is fixed, remove
268279
// this special case.
269280
_referenceChildren[modelElement.element.name] = modelElement;
270281
} else {
271282
_referenceChildren[modelElement.name] = modelElement;
272283
}
284+
}
285+
// Process unscoped parameters last to make sure they don't override
286+
// other options.
287+
for (var modelElement in allModelElements) {
273288
// Don't complain about references to parameter names, but prefer
274289
// referring to anything else.
275290
// TODO(jcollins-g): Figure out something good to do in the ecosystem

test/comment_referable/parser_test.dart

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ void main() {
1111
var result = CommentReferenceParser(codeRef).parse();
1212
var hasHint = result.isNotEmpty &&
1313
(result.first is ConstructorHintStartNode ||
14-
result.last is ConstructorHintEndNode);
14+
result.last is CallableHintEndNode);
1515
var stringParts = result.whereType<IdentifierNode>().map((i) => i.text);
1616
expect(stringParts, equals(parts));
1717
expect(hasHint, equals(constructorHint));

test/end2end/model_test.dart

Lines changed: 34 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2136,12 +2136,13 @@ void main() {
21362136
nameWithTwoUnderscores,
21372137
nameWithSingleUnderscore,
21382138
theOnlyThingInTheLibrary;
2139+
Constructor aNonDefaultConstructor, defaultConstructor;
21392140
Class Apple, BaseClass, baseForDocComments, ExtraSpecialList, string;
21402141
Method doAwesomeStuff, anotherMethod;
21412142
// ignore: unused_local_variable
21422143
Operator bracketOperator, bracketOperatorOtherClass;
21432144
Parameter doAwesomeStuffParam;
2144-
Field forInheriting, action;
2145+
Field forInheriting, action, initializeMe;
21452146

21462147
setUpAll(() async {
21472148
nameWithTwoUnderscores = fakeLibrary.constants
@@ -2154,6 +2155,12 @@ void main() {
21542155
.firstWhere((c) => c.name == 'String');
21552156
baseForDocComments =
21562157
fakeLibrary.classes.firstWhere((c) => c.name == 'BaseForDocComments');
2158+
aNonDefaultConstructor = baseForDocComments.constructors.firstWhere(
2159+
(c) => c.name == 'BaseForDocComments.aNonDefaultConstructor');
2160+
defaultConstructor = baseForDocComments.constructors
2161+
.firstWhere((c) => c.name == 'BaseForDocComments');
2162+
initializeMe = baseForDocComments.allFields
2163+
.firstWhere((f) => f.name == 'initializeMe');
21572164
doAwesomeStuff = baseForDocComments.instanceMethods
21582165
.firstWhere((m) => m.name == 'doAwesomeStuff');
21592166
anotherMethod = baseForDocComments.instanceMethods
@@ -2228,7 +2235,33 @@ void main() {
22282235
return newLookupResult;
22292236
}
22302237

2238+
test('Verify basic linking inside a constructor', () {
2239+
// Field formal parameters worked sometimes by accident in the old code,
2240+
// but should work reliably now.
2241+
expect(newLookup(aNonDefaultConstructor, 'initializeMe'),
2242+
equals(MatchingLinkResult(initializeMe)));
2243+
expect(newLookup(aNonDefaultConstructor, 'aNonDefaultConstructor'),
2244+
equals(MatchingLinkResult(aNonDefaultConstructor)));
2245+
expect(
2246+
bothLookup(aNonDefaultConstructor,
2247+
'BaseForDocComments.aNonDefaultConstructor'),
2248+
equals(MatchingLinkResult(aNonDefaultConstructor)));
2249+
});
2250+
22312251
test('Verify basic linking inside class', () {
2252+
expect(
2253+
bothLookup(
2254+
baseForDocComments, 'BaseForDocComments.BaseForDocComments'),
2255+
equals(MatchingLinkResult(defaultConstructor)));
2256+
2257+
expect(bothLookup(doAwesomeStuff, 'aNonDefaultConstructor'),
2258+
equals(MatchingLinkResult(aNonDefaultConstructor)));
2259+
2260+
expect(
2261+
bothLookup(
2262+
doAwesomeStuff, 'BaseForDocComments.aNonDefaultConstructor'),
2263+
equals(MatchingLinkResult(aNonDefaultConstructor)));
2264+
22322265
expect(bothLookup(doAwesomeStuff, 'this'),
22332266
equals(MatchingLinkResult(baseForDocComments)));
22342267

@@ -2287,7 +2320,6 @@ void main() {
22872320
equals(MatchingLinkResult(BaseClass)));
22882321

22892322
// A bracket operator within this class.
2290-
// TODO(jcollins-g): operator lookups not yet implemented with the new lookup code.
22912323
expect(bothLookup(doAwesomeStuff, 'operator []'),
22922324
equals(MatchingLinkResult(bracketOperator)));
22932325

0 commit comments

Comments
 (0)