Skip to content

Commit 957c0a1

Browse files
stereotype441commit-bot@chromium.org
stereotype441
authored and
[email protected]
committed
Improve "why not promoted" info for field/property reference.
When reporting that a null check is needed due to a lack of type promotion, and the thing that was not promoted was a reference to a field or property, the CFE and analyzer now report "why not promoted" using a context message that points to the definition of the field or getter. (Previously the CFE reported a context message with no location, and the analyzer didn't report "why not promoted"). Bug: #44898 Change-Id: If1841727b8b60dd87c43f239e6a06b98f363801f Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/184522 Reviewed-by: Konstantin Shcheglov <[email protected]> Commit-Queue: Paul Berry <[email protected]>
1 parent 8eda75f commit 957c0a1

File tree

9 files changed

+259
-84
lines changed

9 files changed

+259
-84
lines changed

‎pkg/_fe_analyzer_shared/lib/src/flow_analysis/flow_analysis.dart

Lines changed: 20 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -393,24 +393,6 @@ class ExpressionInfo<Variable extends Object, Type extends Object> {
393393
'ExpressionInfo(after: $after, _ifTrue: $ifTrue, ifFalse: $ifFalse)';
394394
}
395395

396-
/// Non-promotion reason describing the situation where an expression was not
397-
/// promoted due to the fact that it's a field (technically, a property get).
398-
class FieldNotPromoted extends NonPromotionReason {
399-
/// The name of the property.
400-
final String propertyName;
401-
402-
FieldNotPromoted(this.propertyName);
403-
404-
@override
405-
String get shortName => 'fieldNotPromoted($propertyName)';
406-
407-
@override
408-
R accept<R, Node extends Object, Expression extends Object,
409-
Variable extends Object>(
410-
NonPromotionReasonVisitor<R, Node, Expression, Variable> visitor) =>
411-
visitor.visitFieldNotPromoted(this);
412-
}
413-
414396
/// Implementation of flow analysis to be shared between the analyzer and the
415397
/// front end.
416398
///
@@ -2405,7 +2387,25 @@ abstract class NonPromotionReasonVisitor<R, Node extends Object,
24052387
R visitDemoteViaForEachVariableWrite(
24062388
DemoteViaForEachVariableWrite<Variable, Node> reason);
24072389

2408-
R visitFieldNotPromoted(FieldNotPromoted reason);
2390+
R visitPropertyNotPromoted(PropertyNotPromoted reason);
2391+
}
2392+
2393+
/// Non-promotion reason describing the situation where an expression was not
2394+
/// promoted due to the fact that it's a property get.
2395+
class PropertyNotPromoted extends NonPromotionReason {
2396+
/// The name of the property.
2397+
final String propertyName;
2398+
2399+
PropertyNotPromoted(this.propertyName);
2400+
2401+
@override
2402+
String get shortName => 'propertyNotPromoted';
2403+
2404+
@override
2405+
R accept<R, Node extends Object, Expression extends Object,
2406+
Variable extends Object>(
2407+
NonPromotionReasonVisitor<R, Node, Expression, Variable> visitor) =>
2408+
visitor.visitPropertyNotPromoted(this);
24092409
}
24102410

24112411
/// Immutable data structure modeling the reachability of the given point in the
@@ -5000,7 +5000,7 @@ class _PropertyGetReference<Variable extends Object, Type extends Object>
50005000
List<Type>? promotedTypes = _getInfo(variableInfo)?.promotedTypes;
50015001
if (promotedTypes != null) {
50025002
for (Type type in promotedTypes) {
5003-
result[type] = new FieldNotPromoted(propertyName);
5003+
result[type] = new PropertyNotPromoted(propertyName);
50045004
}
50055005
}
50065006
return result;

‎pkg/_fe_analyzer_shared/test/flow_analysis/flow_analysis_test.dart

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5638,7 +5638,7 @@ main() {
56385638
]);
56395639
});
56405640

5641-
group('because field', () {
5641+
group('because property', () {
56425642
test('via explicit this', () {
56435643
var h = Harness();
56445644
h.run([
@@ -5648,7 +5648,7 @@ main() {
56485648
this_('C').propertyGet('field').whyNotPromoted((reasons) {
56495649
expect(reasons.keys, unorderedEquals([Type('Object')]));
56505650
var nonPromotionReason = reasons.values.single;
5651-
expect(nonPromotionReason, TypeMatcher<FieldNotPromoted>());
5651+
expect(nonPromotionReason, TypeMatcher<PropertyNotPromoted>());
56525652
}).stmt,
56535653
]);
56545654
});
@@ -5662,7 +5662,7 @@ main() {
56625662
thisOrSuperPropertyGet('field').whyNotPromoted((reasons) {
56635663
expect(reasons.keys, unorderedEquals([Type('Object')]));
56645664
var nonPromotionReason = reasons.values.single;
5665-
expect(nonPromotionReason, TypeMatcher<FieldNotPromoted>());
5665+
expect(nonPromotionReason, TypeMatcher<PropertyNotPromoted>());
56665666
}).stmt,
56675667
]);
56685668
});
@@ -5678,7 +5678,7 @@ main() {
56785678
x.read.propertyGet('field').whyNotPromoted((reasons) {
56795679
expect(reasons.keys, unorderedEquals([Type('Object')]));
56805680
var nonPromotionReason = reasons.values.single;
5681-
expect(nonPromotionReason, TypeMatcher<FieldNotPromoted>());
5681+
expect(nonPromotionReason, TypeMatcher<PropertyNotPromoted>());
56825682
}).stmt,
56835683
]);
56845684
});

‎pkg/_fe_analyzer_shared/test/flow_analysis/why_not_promoted/data/extension_property.dart

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
// Copyright (c) 2020, the Dart project authors. Please see the AUTHORS file
2+
// for details. All rights reserved. Use of this source code is governed by a
3+
// BSD-style license that can be found in the LICENSE file.
4+
5+
class C {
6+
get_property_via_explicit_this() {
7+
if (this.i == null) return;
8+
this.i. /*notPromoted(propertyNotPromoted(member:E|get#i))*/ isEven;
9+
}
10+
11+
get_property_via_explicit_this_parenthesized() {
12+
if ((this).i == null) return;
13+
(this).i. /*notPromoted(propertyNotPromoted(member:E|get#i))*/ isEven;
14+
}
15+
16+
get_property_by_implicit_this() {
17+
if (i == null) return;
18+
i. /*notPromoted(propertyNotPromoted(member:E|get#i))*/ isEven;
19+
}
20+
}
21+
22+
extension E on C {
23+
int? get i => null;
24+
int? get j => null;
25+
}
26+
27+
class D extends C {
28+
get_property_by_implicit_super() {
29+
if (i == null) return;
30+
i. /*notPromoted(propertyNotPromoted(member:E|get#i))*/ isEven;
31+
}
32+
}
33+
34+
get_property_via_prefixed_identifier(C c) {
35+
if (c.i == null) return;
36+
c.i. /*notPromoted(propertyNotPromoted(member:E|get#i))*/ isEven;
37+
}
38+
39+
get_property_via_prefixed_identifier_mismatched_target(C c1, C c2) {
40+
if (c1.i == null) return;
41+
c2.i.isEven;
42+
}
43+
44+
get_property_via_prefixed_identifier_mismatched_property(C c) {
45+
if (c.i == null) return;
46+
c.j.isEven;
47+
}

‎pkg/_fe_analyzer_shared/test/flow_analysis/why_not_promoted/data/field.dart

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -8,35 +8,35 @@ class C {
88

99
get_field_via_explicit_this() {
1010
if (this.i == null) return;
11-
this.i. /*notPromoted(fieldNotPromoted(i))*/ isEven;
11+
this.i. /*notPromoted(propertyNotPromoted(member:C.i))*/ isEven;
1212
}
1313

1414
get_field_via_explicit_this_parenthesized() {
1515
if ((this).i == null) return;
16-
(this).i. /*notPromoted(fieldNotPromoted(i))*/ isEven;
16+
(this).i. /*notPromoted(propertyNotPromoted(member:C.i))*/ isEven;
1717
}
1818

1919
get_field_by_implicit_this() {
2020
if (i == null) return;
21-
i. /*notPromoted(fieldNotPromoted(i))*/ isEven;
21+
i. /*notPromoted(propertyNotPromoted(member:C.i))*/ isEven;
2222
}
2323
}
2424

2525
class D extends C {
2626
get_field_via_explicit_super() {
2727
if (super.i == null) return;
28-
super.i. /*notPromoted(fieldNotPromoted(i))*/ isEven;
28+
super.i. /*notPromoted(propertyNotPromoted(member:C.i))*/ isEven;
2929
}
3030

3131
get_field_by_implicit_super() {
3232
if (i == null) return;
33-
i. /*notPromoted(fieldNotPromoted(i))*/ isEven;
33+
i. /*notPromoted(propertyNotPromoted(member:C.i))*/ isEven;
3434
}
3535
}
3636

3737
get_field_via_prefixed_identifier(C c) {
3838
if (c.i == null) return;
39-
c.i. /*notPromoted(fieldNotPromoted(i))*/ isEven;
39+
c.i. /*notPromoted(propertyNotPromoted(member:C.i))*/ isEven;
4040
}
4141

4242
get_field_via_prefixed_identifier_mismatched_target(C c1, C c2) {

‎pkg/_fe_analyzer_shared/test/flow_analysis/why_not_promoted/data/property.dart

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
// Copyright (c) 2020, the Dart project authors. Please see the AUTHORS file
2+
// for details. All rights reserved. Use of this source code is governed by a
3+
// BSD-style license that can be found in the LICENSE file.
4+
5+
class C {
6+
int? get i => null;
7+
int? get j => null;
8+
9+
get_property_via_explicit_this() {
10+
if (this.i == null) return;
11+
this.i. /*notPromoted(propertyNotPromoted(member:C.i))*/ isEven;
12+
}
13+
14+
get_property_via_explicit_this_parenthesized() {
15+
if ((this).i == null) return;
16+
(this).i. /*notPromoted(propertyNotPromoted(member:C.i))*/ isEven;
17+
}
18+
19+
get_property_by_implicit_this() {
20+
if (i == null) return;
21+
i. /*notPromoted(propertyNotPromoted(member:C.i))*/ isEven;
22+
}
23+
}
24+
25+
class D extends C {
26+
get_property_via_explicit_super() {
27+
if (super.i == null) return;
28+
super.i. /*notPromoted(propertyNotPromoted(member:C.i))*/ isEven;
29+
}
30+
31+
get_property_by_implicit_super() {
32+
if (i == null) return;
33+
i. /*notPromoted(propertyNotPromoted(member:C.i))*/ isEven;
34+
}
35+
}
36+
37+
get_property_via_prefixed_identifier(C c) {
38+
if (c.i == null) return;
39+
c.i. /*notPromoted(propertyNotPromoted(member:C.i))*/ isEven;
40+
}
41+
42+
get_property_via_prefixed_identifier_mismatched_target(C c1, C c2) {
43+
if (c1.i == null) return;
44+
c2.i.isEven;
45+
}
46+
47+
get_property_via_prefixed_identifier_mismatched_property(C c) {
48+
if (c.i == null) return;
49+
c.j.isEven;
50+
}

‎pkg/analyzer/lib/src/dart/resolver/type_property_resolver.dart

Lines changed: 60 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import 'package:analyzer/src/diagnostic/diagnostic.dart';
1919
import 'package:analyzer/src/error/codes.dart';
2020
import 'package:analyzer/src/generated/resolver.dart';
2121
import 'package:analyzer/src/generated/source.dart';
22+
import 'package:analyzer/src/util/ast_data_extractor.dart';
2223

2324
/// Helper for resolving properties (getters, setters, or methods).
2425
class TypePropertyResolver {
@@ -119,24 +120,32 @@ class TypePropertyResolver {
119120
}
120121
}
121122

122-
var whyNotPromoted = receiver == null
123-
? null
124-
: _resolver.flowAnalysis?.flow?.whyNotPromoted(receiver);
125123
List<DiagnosticMessage> messages = [];
126-
if (whyNotPromoted != null) {
127-
for (var entry in whyNotPromoted.entries) {
128-
var whyNotPromotedVisitor = _WhyNotPromotedVisitor(
129-
_resolver.source, _resolver.flowAnalysis!.dataForTesting);
130-
if (_typeSystem.isPotentiallyNullable(entry.key)) continue;
131-
if (_resolver.flowAnalysis!.dataForTesting != null) {
132-
_resolver.flowAnalysis!.dataForTesting!
133-
.nonPromotionReasons[nameErrorEntity] = entry.value.shortName;
124+
if (receiver != null) {
125+
var whyNotPromoted =
126+
_resolver.flowAnalysis?.flow?.whyNotPromoted(receiver);
127+
if (whyNotPromoted != null) {
128+
for (var entry in whyNotPromoted.entries) {
129+
var whyNotPromotedVisitor = _WhyNotPromotedVisitor(_resolver.source,
130+
receiver, _resolver.flowAnalysis!.dataForTesting);
131+
if (_typeSystem.isPotentiallyNullable(entry.key)) continue;
132+
var message = entry.value.accept(whyNotPromotedVisitor);
133+
if (message != null) {
134+
if (_resolver.flowAnalysis!.dataForTesting != null) {
135+
var nonPromotionReasonText = entry.value.shortName;
136+
if (whyNotPromotedVisitor.propertyReference != null) {
137+
var id =
138+
computeMemberId(whyNotPromotedVisitor.propertyReference!);
139+
nonPromotionReasonText += '($id)';
140+
}
141+
_resolver.flowAnalysis!.dataForTesting!
142+
.nonPromotionReasons[nameErrorEntity] =
143+
nonPromotionReasonText;
144+
}
145+
messages = [message];
146+
}
147+
break;
134148
}
135-
var message = entry.value.accept(whyNotPromotedVisitor);
136-
if (message != null) {
137-
messages = [message];
138-
}
139-
break;
140149
}
141150
}
142151

@@ -297,9 +306,13 @@ class _WhyNotPromotedVisitor
297306
PromotableElement> {
298307
final Source source;
299308

309+
final Expression _receiver;
310+
300311
final FlowAnalysisDataForTesting? _dataForTesting;
301312

302-
_WhyNotPromotedVisitor(this.source, this._dataForTesting);
313+
PropertyAccessorElement? propertyReference;
314+
315+
_WhyNotPromotedVisitor(this.source, this._receiver, this._dataForTesting);
303316

304317
@override
305318
DiagnosticMessage? visitDemoteViaExplicitWrite(
@@ -343,9 +356,36 @@ class _WhyNotPromotedVisitor
343356
}
344357

345358
@override
346-
DiagnosticMessage? visitFieldNotPromoted(FieldNotPromoted reason) {
347-
// TODO(paulberry): how to report this?
348-
return null;
359+
DiagnosticMessage? visitPropertyNotPromoted(PropertyNotPromoted reason) {
360+
var receiver = _receiver;
361+
Element? receiverElement;
362+
if (receiver is SimpleIdentifier) {
363+
receiverElement = receiver.staticElement;
364+
} else if (receiver is PropertyAccess) {
365+
receiverElement = receiver.propertyName.staticElement;
366+
} else if (receiver is PrefixedIdentifier) {
367+
receiverElement = receiver.identifier.staticElement;
368+
} else {
369+
assert(false, 'Unrecognized receiver: ${receiver.runtimeType}');
370+
}
371+
if (receiverElement is PropertyAccessorElement) {
372+
propertyReference = receiverElement;
373+
return _contextMessageForProperty(receiverElement, reason.propertyName);
374+
} else {
375+
assert(receiverElement == null,
376+
'Unrecognized receiver element: ${receiverElement.runtimeType}');
377+
return null;
378+
}
379+
}
380+
381+
DiagnosticMessageImpl _contextMessageForProperty(
382+
PropertyAccessorElement property, String propertyName) {
383+
return DiagnosticMessageImpl(
384+
filePath: property.source.fullName,
385+
message:
386+
"'$propertyName' refers to a property so it could not be promoted.",
387+
offset: property.nameOffset,
388+
length: property.nameLength);
349389
}
350390

351391
DiagnosticMessageImpl _contextMessageForWrite(

0 commit comments

Comments
 (0)