Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[const_finder] Ignore constructor invocations from generated tear-off declarations #38131

Merged
merged 5 commits into from
Dec 9, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 42 additions & 1 deletion tools/const_finder/lib/const_finder.dart
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,33 @@ class _ConstVisitor extends RecursiveVisitor<void> {

bool inIgnoredClass = false;

/// Whether or not we are currently within the declaration of the target class.
///
/// We use this to determine when to skip tracking non-constant
/// [ConstructorInvocation]s. This is because, in web builds, a static
/// method is always created called _#new#tearOff() which returns the result
/// of a non-constant invocation of the unnamed constructor.
///
/// For the following Dart class "FooBar":
///
/// class FooBar {
/// const FooBar();
/// }
///
/// The following kernel structure is generated:
///
/// class FooBar extends core::Object /*hasConstConstructor*/ {
/// const constructor •() → min::FooBar
/// : super core::Object::•()
/// ;
/// static method _#new#tearOff() → min::FooBar
/// return new min::FooBar::•(); /* this is a non-const constructor invocation */
/// method noOp() → void {}
/// }
bool inTargetClass = false;

bool inTargetTearOff = false;

/// The name of the name of the class of the annotation marking classes
/// whose constant references should be ignored.
final String? annotationClassName;
Expand All @@ -58,6 +85,18 @@ class _ConstVisitor extends RecursiveVisitor<void> {
// Avoid visiting the same constant more than once.
final Set<Constant> _cache = LinkedHashSet<Constant>.identity();

@override
void visitProcedure(Procedure node) {
final bool isTearOff = node.isStatic &&
node.kind == ProcedureKind.Method &&
node.name.text == '_#new#tearOff';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will one of the tests below break if the compiler starts using a different name for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the web tests will pick up new non-const locations not in the expectations

if (inTargetClass && isTearOff) {
inTargetTearOff = true;
}
super.visitProcedure(node);
inTargetTearOff = false;
}

@override
void defaultConstant(Constant node) {
if (_cache.add(node)) {
Expand All @@ -73,7 +112,7 @@ class _ConstVisitor extends RecursiveVisitor<void> {
@override
void visitConstructorInvocation(ConstructorInvocation node) {
final Class parentClass = node.target.parent! as Class;
if (_matches(parentClass)) {
if (!inTargetTearOff && _matches(parentClass)) {
final Location location = node.location!;
nonConstantLocations.add(<String, dynamic>{
'file': location.file.toString(),
Expand All @@ -86,9 +125,11 @@ class _ConstVisitor extends RecursiveVisitor<void> {

@override
void visitClass(Class node) {
inTargetClass = _matches(node);
// check if this is a class that we should ignore
inIgnoredClass = _classShouldBeIgnored(node);
super.visitClass(node);
inTargetClass = false;
inIgnoredClass = false;
}

Expand Down
2 changes: 1 addition & 1 deletion tools/const_finder/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ name: const_finder
publish_to: none

environment:
sdk: ">=2.17.0 <3.0.0"
sdk: ">=2.17.0 <4.0.0"

# Do not add any dependencies that require more than what is provided in
# //third_party/dart/pkg or //third_party/dart/third_party/pkg.
Expand Down
66 changes: 56 additions & 10 deletions tools/const_finder/test/const_finder_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,6 @@ void expectInstances(dynamic value, dynamic expected, Compiler compiler) {

final Equality<Object?> equality;
if (compiler == Compiler.dart2js) {
// Ignore comparing nonConstantLocations in web dills because it will have
// extra fields.
(value as Map<String, Object?>).remove('nonConstantLocations');
(expected as Map<String, Object?>).remove('nonConstantLocations');
equality = const Dart2JSDeepCollectionEquality();
} else {
equality = const DeepCollectionEquality();
Expand Down Expand Up @@ -69,9 +65,7 @@ void _checkConsts(String dillPath, Compiler compiler) {
classLibraryUri: 'package:const_finder_fixtures/target.dart',
className: 'Target',
);
expectInstances(
finder.findInstances(),
<String, dynamic>{
final Map<String, Object?> expectation = <String, dynamic>{
'constantInstances': <Map<String, dynamic>>[
<String, dynamic>{'stringValue': '100', 'intValue': 100, 'targetValue': null},
<String, dynamic>{'stringValue': '102', 'intValue': 102, 'targetValue': null},
Expand All @@ -95,7 +89,27 @@ void _checkConsts(String dillPath, Compiler compiler) {
<String, dynamic>{'stringValue': 'package', 'intValue':-1, 'targetValue': null},
],
'nonConstantLocations': <dynamic>[],
},
};
if (compiler == Compiler.aot) {
expectation['nonConstantLocations'] = <Object?>[];
} else {
// Without true tree-shaking, there is a non-const reference in a
// never-invoked function that will be present in the dill.
final String fixturesUrl = Platform.isWindows
? '/$fixtures'.replaceAll(Platform.pathSeparator, '/')
: fixtures;

expectation['nonConstantLocations'] = <Object?>[
<String, dynamic>{
'file': 'file://$fixturesUrl/pkg/package.dart',
'line': 14,
'column': 25,
},
];
}
expectInstances(
finder.findInstances(),
expectation,
compiler,
);

Expand Down Expand Up @@ -125,8 +139,9 @@ void _checkAnnotation(String dillPath, Compiler compiler) {
annotationClassName: 'StaticIconProvider',
annotationClassLibraryUri: 'package:const_finder_fixtures/static_icon_provider.dart',
);
final Map<String, dynamic> instances = finder.findInstances();
expectInstances(
finder.findInstances(),
instances,
<String, dynamic>{
'constantInstances': <Map<String, Object?>>[
<String, Object?>{
Expand All @@ -140,6 +155,8 @@ void _checkAnnotation(String dillPath, Compiler compiler) {
'targetValue': null,
},
],
// TODO(fujino): This should have non-constant locations from the use of
// a tear-off, see https://github.com/flutter/flutter/issues/116797
'nonConstantLocations': <Object?>[],
},
compiler,
Expand Down Expand Up @@ -212,6 +229,9 @@ void _checkNonConstsWeb(String dillPath, Compiler compiler) {
className: 'Target',
);

final String fixturesUrl = Platform.isWindows
? '/$fixtures'.replaceAll(Platform.pathSeparator, '/')
: fixtures;
expectInstances(
finder.findInstances(),
<String, dynamic>{
Expand All @@ -225,7 +245,33 @@ void _checkNonConstsWeb(String dillPath, Compiler compiler) {
<String, dynamic>{'stringValue': '7', 'intValue': 7, 'targetValue': null},
<String, dynamic>{'stringValue': 'package', 'intValue': -1, 'targetValue': null},
],
'nonConstantLocations': <dynamic>[]
'nonConstantLocations': <dynamic>[
<String, dynamic>{
'file': 'file://$fixturesUrl/lib/consts_and_non.dart',
'line': 14,
'column': 26,
},
<String, dynamic>{
'file': 'file://$fixturesUrl/lib/consts_and_non.dart',
'line': 16,
'column': 26,
},
<String, dynamic>{
'file': 'file://$fixturesUrl/lib/consts_and_non.dart',
'line': 16,
'column': 41,
},
<String, dynamic>{
'file': 'file://$fixturesUrl/lib/consts_and_non.dart',
'line': 17,
'column': 26,
},
<String, dynamic>{
'file': 'file://$fixturesUrl/pkg/package.dart',
'line': 14,
'column': 25,
}
],
},
compiler,
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@
{
"name": "const_finder_fixtures",
"rootUri": "../lib/",
"languageVersion": "2.12"
"languageVersion": "2.17"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needed in order to use constructor tear-offs

},
{
"name": "const_finder_fixtures_package",
"rootUri": "../pkg/",
"languageVersion": "2.12"
"languageVersion": "2.17"
}
]
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,12 @@ import 'target.dart';
void main() {
Targets.used1.hit();
Targets.used2.hit();
final Target used3 = helper(Target.new);
used3.hit();
}

Target helper(Target Function(String, int, Target?) tearOff) {
return tearOff('from tear-off', 3, null);
}

@staticIconProvider
Expand Down