Skip to content

Commit 375e91c

Browse files
christopherfujinoloic-sharma
christopherfujino
authored and
loic-sharma
committed
[const_finder] Ignore constructor invocations from generated tear-off declarations (flutter#38131)
* fix * ignore tear off declarations * add TODO comment * clean up * fix analysis
1 parent 9b03bde commit 375e91c

File tree

6 files changed

+108
-16
lines changed

6 files changed

+108
-16
lines changed

‎tools/const_finder/lib/const_finder.dart‎

Lines changed: 42 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,33 @@ class _ConstVisitor extends RecursiveVisitor<void> {
3232

3333
bool inIgnoredClass = false;
3434

35+
/// Whether or not we are currently within the declaration of the target class.
36+
///
37+
/// We use this to determine when to skip tracking non-constant
38+
/// [ConstructorInvocation]s. This is because, in web builds, a static
39+
/// method is always created called _#new#tearOff() which returns the result
40+
/// of a non-constant invocation of the unnamed constructor.
41+
///
42+
/// For the following Dart class "FooBar":
43+
///
44+
/// class FooBar {
45+
/// const FooBar();
46+
/// }
47+
///
48+
/// The following kernel structure is generated:
49+
///
50+
/// class FooBar extends core::Object /*hasConstConstructor*/ {
51+
/// const constructor •() → min::FooBar
52+
/// : super core::Object::•()
53+
/// ;
54+
/// static method _#new#tearOff() → min::FooBar
55+
/// return new min::FooBar::•(); /* this is a non-const constructor invocation */
56+
/// method noOp() → void {}
57+
/// }
58+
bool inTargetClass = false;
59+
60+
bool inTargetTearOff = false;
61+
3562
/// The name of the name of the class of the annotation marking classes
3663
/// whose constant references should be ignored.
3764
final String? annotationClassName;
@@ -58,6 +85,18 @@ class _ConstVisitor extends RecursiveVisitor<void> {
5885
// Avoid visiting the same constant more than once.
5986
final Set<Constant> _cache = LinkedHashSet<Constant>.identity();
6087

88+
@override
89+
void visitProcedure(Procedure node) {
90+
final bool isTearOff = node.isStatic &&
91+
node.kind == ProcedureKind.Method &&
92+
node.name.text == '_#new#tearOff';
93+
if (inTargetClass && isTearOff) {
94+
inTargetTearOff = true;
95+
}
96+
super.visitProcedure(node);
97+
inTargetTearOff = false;
98+
}
99+
61100
@override
62101
void defaultConstant(Constant node) {
63102
if (_cache.add(node)) {
@@ -73,7 +112,7 @@ class _ConstVisitor extends RecursiveVisitor<void> {
73112
@override
74113
void visitConstructorInvocation(ConstructorInvocation node) {
75114
final Class parentClass = node.target.parent! as Class;
76-
if (_matches(parentClass)) {
115+
if (!inTargetTearOff && _matches(parentClass)) {
77116
final Location location = node.location!;
78117
nonConstantLocations.add(<String, dynamic>{
79118
'file': location.file.toString(),
@@ -86,9 +125,11 @@ class _ConstVisitor extends RecursiveVisitor<void> {
86125

87126
@override
88127
void visitClass(Class node) {
128+
inTargetClass = _matches(node);
89129
// check if this is a class that we should ignore
90130
inIgnoredClass = _classShouldBeIgnored(node);
91131
super.visitClass(node);
132+
inTargetClass = false;
92133
inIgnoredClass = false;
93134
}
94135

‎tools/const_finder/pubspec.yaml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ name: const_finder
66
publish_to: none
77

88
environment:
9-
sdk: ">=2.17.0 <3.0.0"
9+
sdk: ">=2.17.0 <4.0.0"
1010

1111
# Do not add any dependencies that require more than what is provided in
1212
# //third_party/dart/pkg or //third_party/dart/third_party/pkg.

‎tools/const_finder/test/const_finder_test.dart‎

Lines changed: 56 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -31,10 +31,6 @@ void expectInstances(dynamic value, dynamic expected, Compiler compiler) {
3131

3232
final Equality<Object?> equality;
3333
if (compiler == Compiler.dart2js) {
34-
// Ignore comparing nonConstantLocations in web dills because it will have
35-
// extra fields.
36-
(value as Map<String, Object?>).remove('nonConstantLocations');
37-
(expected as Map<String, Object?>).remove('nonConstantLocations');
3834
equality = const Dart2JSDeepCollectionEquality();
3935
} else {
4036
equality = const DeepCollectionEquality();
@@ -69,9 +65,7 @@ void _checkConsts(String dillPath, Compiler compiler) {
6965
classLibraryUri: 'package:const_finder_fixtures/target.dart',
7066
className: 'Target',
7167
);
72-
expectInstances(
73-
finder.findInstances(),
74-
<String, dynamic>{
68+
final Map<String, Object?> expectation = <String, dynamic>{
7569
'constantInstances': <Map<String, dynamic>>[
7670
<String, dynamic>{'stringValue': '100', 'intValue': 100, 'targetValue': null},
7771
<String, dynamic>{'stringValue': '102', 'intValue': 102, 'targetValue': null},
@@ -95,7 +89,27 @@ void _checkConsts(String dillPath, Compiler compiler) {
9589
<String, dynamic>{'stringValue': 'package', 'intValue':-1, 'targetValue': null},
9690
],
9791
'nonConstantLocations': <dynamic>[],
98-
},
92+
};
93+
if (compiler == Compiler.aot) {
94+
expectation['nonConstantLocations'] = <Object?>[];
95+
} else {
96+
final String fixturesUrl = Platform.isWindows
97+
? '/$fixtures'.replaceAll(Platform.pathSeparator, '/')
98+
: fixtures;
99+
100+
// Without true tree-shaking, there is a non-const reference in a
101+
// never-invoked function that will be present in the dill.
102+
expectation['nonConstantLocations'] = <Object?>[
103+
<String, dynamic>{
104+
'file': 'file://$fixturesUrl/pkg/package.dart',
105+
'line': 14,
106+
'column': 25,
107+
},
108+
];
109+
}
110+
expectInstances(
111+
finder.findInstances(),
112+
expectation,
99113
compiler,
100114
);
101115

@@ -125,8 +139,9 @@ void _checkAnnotation(String dillPath, Compiler compiler) {
125139
annotationClassName: 'StaticIconProvider',
126140
annotationClassLibraryUri: 'package:const_finder_fixtures/static_icon_provider.dart',
127141
);
142+
final Map<String, dynamic> instances = finder.findInstances();
128143
expectInstances(
129-
finder.findInstances(),
144+
instances,
130145
<String, dynamic>{
131146
'constantInstances': <Map<String, Object?>>[
132147
<String, Object?>{
@@ -140,6 +155,8 @@ void _checkAnnotation(String dillPath, Compiler compiler) {
140155
'targetValue': null,
141156
},
142157
],
158+
// TODO(fujino): This should have non-constant locations from the use of
159+
// a tear-off, see https://github.com/flutter/flutter/issues/116797
143160
'nonConstantLocations': <Object?>[],
144161
},
145162
compiler,
@@ -212,6 +229,9 @@ void _checkNonConstsWeb(String dillPath, Compiler compiler) {
212229
className: 'Target',
213230
);
214231

232+
final String fixturesUrl = Platform.isWindows
233+
? '/$fixtures'.replaceAll(Platform.pathSeparator, '/')
234+
: fixtures;
215235
expectInstances(
216236
finder.findInstances(),
217237
<String, dynamic>{
@@ -225,7 +245,33 @@ void _checkNonConstsWeb(String dillPath, Compiler compiler) {
225245
<String, dynamic>{'stringValue': '7', 'intValue': 7, 'targetValue': null},
226246
<String, dynamic>{'stringValue': 'package', 'intValue': -1, 'targetValue': null},
227247
],
228-
'nonConstantLocations': <dynamic>[]
248+
'nonConstantLocations': <dynamic>[
249+
<String, dynamic>{
250+
'file': 'file://$fixturesUrl/lib/consts_and_non.dart',
251+
'line': 14,
252+
'column': 26,
253+
},
254+
<String, dynamic>{
255+
'file': 'file://$fixturesUrl/lib/consts_and_non.dart',
256+
'line': 16,
257+
'column': 26,
258+
},
259+
<String, dynamic>{
260+
'file': 'file://$fixturesUrl/lib/consts_and_non.dart',
261+
'line': 16,
262+
'column': 41,
263+
},
264+
<String, dynamic>{
265+
'file': 'file://$fixturesUrl/lib/consts_and_non.dart',
266+
'line': 17,
267+
'column': 26,
268+
},
269+
<String, dynamic>{
270+
'file': 'file://$fixturesUrl/pkg/package.dart',
271+
'line': 14,
272+
'column': 25,
273+
}
274+
],
229275
},
230276
compiler,
231277
);

‎tools/const_finder/test/fixtures/.dart_tool/package_config.json‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,12 +4,12 @@
44
{
55
"name": "const_finder_fixtures",
66
"rootUri": "../lib/",
7-
"languageVersion": "2.12"
7+
"languageVersion": "2.17"
88
},
99
{
1010
"name": "const_finder_fixtures_package",
1111
"rootUri": "../pkg/",
12-
"languageVersion": "2.12"
12+
"languageVersion": "2.17"
1313
}
1414
]
1515
}

‎tools/const_finder/test/fixtures/lib/static_icon_provider.dart‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,12 @@ import 'target.dart';
77
void main() {
88
Targets.used1.hit();
99
Targets.used2.hit();
10+
final Target nonConstUsed3 = helper(Target.new);
11+
nonConstUsed3.hit();
12+
}
13+
14+
Target helper(Target Function(String, int, Target?) tearOff) {
15+
return tearOff('from tear-off', 3, null);
1016
}
1117

1218
@staticIconProvider

‎tools/const_finder/test/fixtures/lib/target.dart‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,7 @@ class Target {
1515
}
1616

1717
class ExtendsTarget extends Target {
18-
const ExtendsTarget(String stringValue, int intValue, Target? targetValue)
19-
: super(stringValue, intValue, targetValue);
18+
const ExtendsTarget(super.stringValue, super.intValue, super.targetValue);
2019
}
2120

2221
class ImplementsTarget implements Target {

0 commit comments

Comments
 (0)