Skip to content

Commit 9e5fd51

Browse files
jcollins-gjcollins-g
authored
Clean up and add tests for new lookup code parameter handling (#2702)
* Clean up and add tests for parameter handling * revert temporary debugging rename * comment update
1 parent f61f3ba commit 9e5fd51

File tree

6 files changed

+170
-37
lines changed

6 files changed

+170
-37
lines changed

‎lib/src/markdown_processor.dart

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -315,10 +315,6 @@ bool _rejectDefaultAndShadowingConstructors(CommentReferable referable) {
315315
return false;
316316
}
317317
}
318-
// Avoid accidentally preferring arguments of the default constructor.
319-
if (referable is ModelElement && referable.enclosingElement is Constructor) {
320-
return false;
321-
}
322318
return true;
323319
}
324320

‎lib/src/model/comment_referable.dart

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -137,11 +137,14 @@ mixin CommentReferable implements Nameable {
137137
/// A list of lookups that should be attempted on children based on
138138
/// [reference]. This allows us to deal with libraries that may have
139139
/// separators in them. [referenceBy] stops at the first one found.
140-
Iterable<ReferenceChildrenLookup> childLookups(List<String> reference) sync* {
140+
// TODO(jcollins-g): Convert to generator after dart-lang/sdk#46419
141+
Iterable<ReferenceChildrenLookup> childLookups(List<String> reference) {
142+
var retval = <ReferenceChildrenLookup>[];
141143
for (var index = 1; index <= reference.length; index++) {
142-
yield ReferenceChildrenLookup(
143-
reference.sublist(0, index).join('.'), reference.sublist(index));
144+
retval.add(ReferenceChildrenLookup(
145+
reference.sublist(0, index).join('.'), reference.sublist(index)));
144146
}
147+
return retval;
145148
}
146149

147150
/// Map of name to the elements that are a member of [this], but

‎lib/src/model/constructor.dart

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -137,15 +137,7 @@ class Constructor extends ModelElement
137137
ModelElement.fromElement(paramElement.field, packageGraph);
138138
_referenceChildren[paramElement.name] = fieldFormal;
139139
} else {
140-
var constructorName = element.name;
141-
if (constructorName == '') {
142-
constructorName = enclosingElement.name;
143-
}
144-
if (constructorName == param.name) {
145-
// Force users to specify a parameter explicitly in this case.
146-
_referenceChildren['$constructorName.${param.name}'] = param;
147-
}
148-
// Allow fallback handling in [Container] to handle other cases.
140+
_referenceChildren[param.name] = param;
149141
}
150142
}
151143
_referenceChildren

‎lib/src/model/method.dart

Lines changed: 4 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -127,20 +127,10 @@ class Method extends ModelElement
127127
Map<String, CommentReferable> _referenceChildren;
128128
@override
129129
Map<String, CommentReferable> get referenceChildren {
130-
if (_referenceChildren == null) {
131-
_referenceChildren = {};
132-
_referenceChildren
133-
.addEntries(typeParameters.map((p) => MapEntry(p.name, p)));
134-
_referenceChildren.addEntries(allParameters.expand((p) {
135-
if (p.name == name) {
136-
// Force explicit references to parameters named the same as
137-
// the method.
138-
return [MapEntry('$name.${p.name}', p)];
139-
}
140-
// Allow fallback handling in [Container] to deal with other cases.
141-
return [];
142-
}));
143-
}
130+
_referenceChildren ??= Map.fromEntries([
131+
...typeParameters.map((p) => MapEntry(p.name, p)),
132+
...allParameters.map((p) => MapEntry(p.name, p)),
133+
]);
144134
return _referenceChildren;
145135
}
146136
}

‎test/end2end/model_test.dart

Lines changed: 130 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2266,22 +2266,37 @@ void main() {
22662266
theOnlyThingInTheLibrary;
22672267
Constructor aNonDefaultConstructor,
22682268
defaultConstructor,
2269-
aConstructorShadowed;
2269+
aConstructorShadowed,
2270+
anotherName,
2271+
anotherConstructor,
2272+
factoryConstructorThingsDefault;
22702273
Class Apple,
22712274
BaseClass,
22722275
baseForDocComments,
22732276
ExtraSpecialList,
2277+
FactoryConstructorThings,
22742278
string,
22752279
metaUseResult;
2276-
Method doAwesomeStuff, anotherMethod;
2280+
Method doAwesomeStuff, anotherMethod, aMethod;
22772281
// ignore: unused_local_variable
22782282
Operator bracketOperator, bracketOperatorOtherClass;
2279-
Parameter doAwesomeStuffParam;
2283+
Parameter doAwesomeStuffParam,
2284+
aName,
2285+
anotherNameParameter,
2286+
anotherDifferentName,
2287+
differentName,
2288+
redHerring,
2289+
yetAnotherName,
2290+
somethingShadowyParameter;
22802291
Field forInheriting,
22812292
action,
22822293
initializeMe,
22832294
somethingShadowy,
2284-
aConstructorShadowedField;
2295+
aConstructorShadowedField,
2296+
aNameField,
2297+
anotherNameField,
2298+
yetAnotherNameField,
2299+
initViaFieldFormal;
22852300

22862301
setUpAll(() async {
22872302
mylibpub = packageGraph.allLibraries.values
@@ -2309,6 +2324,8 @@ void main() {
23092324
(c) => c.name == 'BaseForDocComments.aNonDefaultConstructor');
23102325
defaultConstructor = baseForDocComments.constructors
23112326
.firstWhere((c) => c.name == 'BaseForDocComments');
2327+
somethingShadowyParameter = defaultConstructor.allParameters
2328+
.firstWhere((p) => p.name == 'somethingShadowy');
23122329
initializeMe = baseForDocComments.allFields
23132330
.firstWhere((f) => f.name == 'initializeMe');
23142331
somethingShadowy = baseForDocComments.allFields
@@ -2361,6 +2378,106 @@ void main() {
23612378
(c) => c.name == 'BaseForDocComments.aConstructorShadowed');
23622379
aConstructorShadowedField = baseForDocComments.allFields
23632380
.firstWhere((f) => f.name == 'aConstructorShadowed');
2381+
2382+
FactoryConstructorThings = fakeLibrary.classes
2383+
.firstWhere((c) => c.name == 'FactoryConstructorThings');
2384+
anotherName = FactoryConstructorThings.constructors.firstWhere(
2385+
(c) => c.name == 'FactoryConstructorThings.anotherName');
2386+
anotherConstructor = FactoryConstructorThings.constructors.firstWhere(
2387+
(c) => c.name == 'FactoryConstructorThings.anotherConstructor');
2388+
factoryConstructorThingsDefault = FactoryConstructorThings.constructors
2389+
.firstWhere((c) => c.name == 'FactoryConstructorThings');
2390+
2391+
aName = anotherName.allParameters.firstWhere((p) => p.name == 'aName');
2392+
anotherNameParameter = anotherName.allParameters
2393+
.firstWhere((p) => p.name == 'anotherName');
2394+
anotherDifferentName = anotherName.allParameters
2395+
.firstWhere((p) => p.name == 'anotherDifferentName');
2396+
differentName = anotherName.allParameters
2397+
.firstWhere((p) => p.name == 'differentName');
2398+
redHerring = anotherConstructor.allParameters
2399+
.firstWhere((p) => p.name == 'redHerring');
2400+
2401+
aNameField = FactoryConstructorThings.allFields
2402+
.firstWhere((f) => f.name == 'aName');
2403+
anotherNameField = FactoryConstructorThings.allFields
2404+
.firstWhere((f) => f.name == 'anotherName');
2405+
yetAnotherNameField = FactoryConstructorThings.allFields
2406+
.firstWhere((f) => f.name == 'yetAnotherName');
2407+
initViaFieldFormal = FactoryConstructorThings.allFields
2408+
.firstWhere((f) => f.name == 'initViaFieldFormal');
2409+
2410+
aMethod = FactoryConstructorThings.instanceMethods
2411+
.firstWhere((m) => m.name == 'aMethod');
2412+
yetAnotherName =
2413+
aMethod.allParameters.firstWhere((p) => p.name == 'yetAnotherName');
2414+
});
2415+
2416+
group('Parameter references work properly', () {
2417+
test('in class scope overridden by fields', () {
2418+
expect(bothLookup(FactoryConstructorThings, 'aName'),
2419+
equals(MatchingLinkResult(aNameField)));
2420+
expect(bothLookup(FactoryConstructorThings, 'anotherName'),
2421+
equals(MatchingLinkResult(anotherNameField)));
2422+
expect(bothLookup(FactoryConstructorThings, 'yetAnotherName'),
2423+
equals(MatchingLinkResult(yetAnotherNameField)));
2424+
expect(bothLookup(FactoryConstructorThings, 'initViaFieldFormal'),
2425+
equals(MatchingLinkResult(initViaFieldFormal)));
2426+
expect(bothLookup(FactoryConstructorThings, 'redHerring'),
2427+
equals(MatchingLinkResult(redHerring)));
2428+
});
2429+
2430+
test('in class scope overridden by constructors when specified', () {
2431+
expect(
2432+
bothLookup(FactoryConstructorThings,
2433+
'new FactoryConstructorThings.anotherName'),
2434+
equals(MatchingLinkResult(anotherName)));
2435+
});
2436+
2437+
test(
2438+
'in default constructor scope referring to a field formal parameter',
2439+
() {
2440+
expect(
2441+
newLookup(factoryConstructorThingsDefault, 'initViaFieldFormal'),
2442+
equals(MatchingLinkResult(initViaFieldFormal)));
2443+
});
2444+
2445+
test('in factory constructor scope referring to parameters', () {
2446+
expect(newLookup(anotherName, 'aName'),
2447+
equals(MatchingLinkResult(aName)));
2448+
expect(bothLookup(anotherName, 'anotherName'),
2449+
equals(MatchingLinkResult(anotherNameParameter)));
2450+
expect(bothLookup(anotherName, 'anotherDifferentName'),
2451+
equals(MatchingLinkResult(anotherDifferentName)));
2452+
expect(bothLookup(anotherName, 'differentName'),
2453+
equals(MatchingLinkResult(differentName)));
2454+
expect(bothLookup(anotherName, 'redHerring'),
2455+
equals(MatchingLinkResult(redHerring)));
2456+
});
2457+
2458+
test('in factory constructor scope referring to constructors', () {
2459+
// A bare constructor reference is OK because there is no conflict.
2460+
expect(bothLookup(anotherName, 'anotherConstructor'),
2461+
equals(MatchingLinkResult(anotherConstructor)));
2462+
// A conflicting constructor has to be explicit.
2463+
expect(
2464+
bothLookup(
2465+
anotherName, 'new FactoryConstructorThings.anotherName'),
2466+
equals(MatchingLinkResult(anotherName)));
2467+
});
2468+
2469+
test('in method scope referring to parameters and variables', () {
2470+
expect(bothLookup(aMethod, 'yetAnotherName'),
2471+
equals(MatchingLinkResult(yetAnotherName)));
2472+
expect(bothLookup(aMethod, 'FactoryConstructorThings.yetAnotherName'),
2473+
equals(MatchingLinkResult(yetAnotherNameField)));
2474+
expect(
2475+
bothLookup(
2476+
aMethod, 'FactoryConstructorThings.anotherName.anotherName'),
2477+
equals(MatchingLinkResult(anotherNameParameter)));
2478+
expect(bothLookup(aMethod, 'aName'),
2479+
equals(MatchingLinkResult(aNameField)));
2480+
});
23642481
});
23652482

23662483
test('Referring to a renamed library directly works', () {
@@ -2425,11 +2542,17 @@ void main() {
24252542
equals(MatchingLinkResult(defaultConstructor)));
24262543

24272544
// We don't want the parameter on the default constructor, here.
2428-
expect(
2429-
bothLookup(
2430-
baseForDocComments, 'BaseForDocComments.somethingShadowy'),
2545+
expect(bothLookup(fakeLibrary, 'BaseForDocComments.somethingShadowy'),
2546+
equals(MatchingLinkResult(somethingShadowy)));
2547+
expect(bothLookup(baseForDocComments, 'somethingShadowy'),
24312548
equals(MatchingLinkResult(somethingShadowy)));
24322549

2550+
// Allow specific reference if necessary
2551+
expect(
2552+
newLookup(baseForDocComments,
2553+
'BaseForDocComments.BaseForDocComments.somethingShadowy'),
2554+
equals(MatchingLinkResult(somethingShadowyParameter)));
2555+
24332556
expect(bothLookup(doAwesomeStuff, 'aNonDefaultConstructor'),
24342557
equals(MatchingLinkResult(aNonDefaultConstructor)));
24352558

‎testing/test_package/lib/fake.dart

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1253,3 +1253,32 @@ abstract class IntermediateAbstract extends Object {
12531253

12541254
/// This should inherit [==] from [IntermediateAbstract].
12551255
class IntermediateAbstractSubclass extends IntermediateAbstract {}
1256+
1257+
1258+
/// Test parameter comment resolution in factory constructors and methods.
1259+
class FactoryConstructorThings {
1260+
bool aName;
1261+
int anotherName;
1262+
String yetAnotherName;
1263+
final List<String> initViaFieldFormal;
1264+
1265+
FactoryConstructorThings(this.initViaFieldFormal);
1266+
1267+
factory FactoryConstructorThings.anotherName({
1268+
bool aName,
1269+
List<int> anotherName,
1270+
int anotherDifferentName,
1271+
String differentName,
1272+
}) {
1273+
return null;
1274+
}
1275+
1276+
factory FactoryConstructorThings.anotherConstructor({
1277+
bool anotherName,
1278+
bool redHerring,
1279+
}) {
1280+
return null;
1281+
}
1282+
1283+
void aMethod(bool yetAnotherName) {}
1284+
}

0 commit comments

Comments
 (0)