From f013067f4b0c4af30adefd60bc00c5948b8b6dba Mon Sep 17 00:00:00 2001 From: Christopher Fujino Date: Wed, 7 Dec 2022 16:23:21 -0800 Subject: [PATCH 1/5] fix --- tools/const_finder/lib/const_finder.dart | 29 ++++++++- .../const_finder/test/const_finder_test.dart | 61 ++++++++++++++++--- 2 files changed, 80 insertions(+), 10 deletions(-) diff --git a/tools/const_finder/lib/const_finder.dart b/tools/const_finder/lib/const_finder.dart index 4af94de2aa285..566ed3f1b163e 100644 --- a/tools/const_finder/lib/const_finder.dart +++ b/tools/const_finder/lib/const_finder.dart @@ -32,6 +32,31 @@ class _ConstVisitor extends RecursiveVisitor { 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; + /// The name of the name of the class of the annotation marking classes /// whose constant references should be ignored. final String? annotationClassName; @@ -73,7 +98,7 @@ class _ConstVisitor extends RecursiveVisitor { @override void visitConstructorInvocation(ConstructorInvocation node) { final Class parentClass = node.target.parent! as Class; - if (_matches(parentClass)) { + if (_matches(parentClass) && !inTargetClass) { final Location location = node.location!; nonConstantLocations.add({ 'file': location.file.toString(), @@ -86,9 +111,11 @@ class _ConstVisitor extends RecursiveVisitor { @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; } diff --git a/tools/const_finder/test/const_finder_test.dart b/tools/const_finder/test/const_finder_test.dart index 6c6da5d2bcd8c..45d43fdabc5ec 100644 --- a/tools/const_finder/test/const_finder_test.dart +++ b/tools/const_finder/test/const_finder_test.dart @@ -31,10 +31,6 @@ void expectInstances(dynamic value, dynamic expected, Compiler compiler) { final Equality equality; if (compiler == Compiler.dart2js) { - // Ignore comparing nonConstantLocations in web dills because it will have - // extra fields. - (value as Map).remove('nonConstantLocations'); - (expected as Map).remove('nonConstantLocations'); equality = const Dart2JSDeepCollectionEquality(); } else { equality = const DeepCollectionEquality(); @@ -69,9 +65,7 @@ void _checkConsts(String dillPath, Compiler compiler) { classLibraryUri: 'package:const_finder_fixtures/target.dart', className: 'Target', ); - expectInstances( - finder.findInstances(), - { + final Map expectation = { 'constantInstances': >[ {'stringValue': '100', 'intValue': 100, 'targetValue': null}, {'stringValue': '102', 'intValue': 102, 'targetValue': null}, @@ -95,7 +89,27 @@ void _checkConsts(String dillPath, Compiler compiler) { {'stringValue': 'package', 'intValue':-1, 'targetValue': null}, ], 'nonConstantLocations': [], - }, + }; + if (compiler == Compiler.aot) { + expectation['nonConstantLocations'] = []; + } 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'] = [ + { + 'file': 'file://$fixturesUrl/pkg/package.dart', + 'line': 14, + 'column': 25, + }, + ]; + } + expectInstances( + finder.findInstances(), + expectation, compiler, ); @@ -212,6 +226,9 @@ void _checkNonConstsWeb(String dillPath, Compiler compiler) { className: 'Target', ); + final String fixturesUrl = Platform.isWindows + ? '/$fixtures'.replaceAll(Platform.pathSeparator, '/') + : fixtures; expectInstances( finder.findInstances(), { @@ -225,7 +242,33 @@ void _checkNonConstsWeb(String dillPath, Compiler compiler) { {'stringValue': '7', 'intValue': 7, 'targetValue': null}, {'stringValue': 'package', 'intValue': -1, 'targetValue': null}, ], - 'nonConstantLocations': [] + 'nonConstantLocations': [ + { + 'file': 'file://$fixturesUrl/lib/consts_and_non.dart', + 'line': 14, + 'column': 26, + }, + { + 'file': 'file://$fixturesUrl/lib/consts_and_non.dart', + 'line': 16, + 'column': 26, + }, + { + 'file': 'file://$fixturesUrl/lib/consts_and_non.dart', + 'line': 16, + 'column': 41, + }, + { + 'file': 'file://$fixturesUrl/lib/consts_and_non.dart', + 'line': 17, + 'column': 26, + }, + { + 'file': 'file://$fixturesUrl/pkg/package.dart', + 'line': 14, + 'column': 25, + } + ], }, compiler, ); From 3163564cbdec564d560d8dd79369a096e5b008e8 Mon Sep 17 00:00:00 2001 From: Christopher Fujino Date: Fri, 9 Dec 2022 11:32:25 -0800 Subject: [PATCH 2/5] ignore tear off declarations --- tools/const_finder/lib/const_finder.dart | 16 +++++++++++++++- tools/const_finder/pubspec.yaml | 2 +- tools/const_finder/test/const_finder_test.dart | 3 ++- .../test/fixtures/.dart_tool/package_config.json | 4 ++-- .../test/fixtures/lib/static_icon_provider.dart | 6 ++++++ 5 files changed, 26 insertions(+), 5 deletions(-) diff --git a/tools/const_finder/lib/const_finder.dart b/tools/const_finder/lib/const_finder.dart index 566ed3f1b163e..354c8ac9ea1ca 100644 --- a/tools/const_finder/lib/const_finder.dart +++ b/tools/const_finder/lib/const_finder.dart @@ -57,6 +57,8 @@ class _ConstVisitor extends RecursiveVisitor { /// } 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; @@ -83,6 +85,18 @@ class _ConstVisitor extends RecursiveVisitor { // Avoid visiting the same constant more than once. final Set _cache = LinkedHashSet.identity(); + @override + void visitProcedure(Procedure node) { + final bool isTearOff = node.isStatic && + node.kind == ProcedureKind.Method && + node.name.text == '_#new#tearOff'; + if (inTargetClass && isTearOff) { + inTargetTearOff = true; + } + super.visitProcedure(node); + inTargetTearOff = false; + } + @override void defaultConstant(Constant node) { if (_cache.add(node)) { @@ -98,7 +112,7 @@ class _ConstVisitor extends RecursiveVisitor { @override void visitConstructorInvocation(ConstructorInvocation node) { final Class parentClass = node.target.parent! as Class; - if (_matches(parentClass) && !inTargetClass) { + if (!inTargetTearOff && _matches(parentClass)) { final Location location = node.location!; nonConstantLocations.add({ 'file': location.file.toString(), diff --git a/tools/const_finder/pubspec.yaml b/tools/const_finder/pubspec.yaml index 4a9ce8e9b4a17..9e4804c8aa400 100644 --- a/tools/const_finder/pubspec.yaml +++ b/tools/const_finder/pubspec.yaml @@ -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. diff --git a/tools/const_finder/test/const_finder_test.dart b/tools/const_finder/test/const_finder_test.dart index 45d43fdabc5ec..dd0df7dba52c5 100644 --- a/tools/const_finder/test/const_finder_test.dart +++ b/tools/const_finder/test/const_finder_test.dart @@ -139,8 +139,9 @@ void _checkAnnotation(String dillPath, Compiler compiler) { annotationClassName: 'StaticIconProvider', annotationClassLibraryUri: 'package:const_finder_fixtures/static_icon_provider.dart', ); + final Map instances = finder.findInstances(); expectInstances( - finder.findInstances(), + instances, { 'constantInstances': >[ { diff --git a/tools/const_finder/test/fixtures/.dart_tool/package_config.json b/tools/const_finder/test/fixtures/.dart_tool/package_config.json index 52cc3dac2fe36..b6392ddc92e8c 100644 --- a/tools/const_finder/test/fixtures/.dart_tool/package_config.json +++ b/tools/const_finder/test/fixtures/.dart_tool/package_config.json @@ -4,12 +4,12 @@ { "name": "const_finder_fixtures", "rootUri": "../lib/", - "languageVersion": "2.12" + "languageVersion": "2.17" }, { "name": "const_finder_fixtures_package", "rootUri": "../pkg/", - "languageVersion": "2.12" + "languageVersion": "2.17" } ] } diff --git a/tools/const_finder/test/fixtures/lib/static_icon_provider.dart b/tools/const_finder/test/fixtures/lib/static_icon_provider.dart index ec9121c1c0fc4..67c699337fc57 100644 --- a/tools/const_finder/test/fixtures/lib/static_icon_provider.dart +++ b/tools/const_finder/test/fixtures/lib/static_icon_provider.dart @@ -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 From ab66ba7255e99e49bc37b431d770eae2ebd6cc2c Mon Sep 17 00:00:00 2001 From: Christopher Fujino Date: Fri, 9 Dec 2022 11:38:02 -0800 Subject: [PATCH 3/5] add TODO comment --- tools/const_finder/test/const_finder_test.dart | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tools/const_finder/test/const_finder_test.dart b/tools/const_finder/test/const_finder_test.dart index dd0df7dba52c5..064bd5c2a9e94 100644 --- a/tools/const_finder/test/const_finder_test.dart +++ b/tools/const_finder/test/const_finder_test.dart @@ -155,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': [], }, compiler, From c80400677591a206beabf39de7b0493537302dd3 Mon Sep 17 00:00:00 2001 From: Christopher Fujino Date: Fri, 9 Dec 2022 11:43:46 -0800 Subject: [PATCH 4/5] clean up --- tools/const_finder/test/const_finder_test.dart | 12 ++++++------ .../test/fixtures/lib/static_icon_provider.dart | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/tools/const_finder/test/const_finder_test.dart b/tools/const_finder/test/const_finder_test.dart index 064bd5c2a9e94..5df4791094465 100644 --- a/tools/const_finder/test/const_finder_test.dart +++ b/tools/const_finder/test/const_finder_test.dart @@ -93,18 +93,18 @@ void _checkConsts(String dillPath, Compiler compiler) { if (compiler == Compiler.aot) { expectation['nonConstantLocations'] = []; } 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; + // Without true tree-shaking, there is a non-const reference in a + // never-invoked function that will be present in the dill. expectation['nonConstantLocations'] = [ { - 'file': 'file://$fixturesUrl/pkg/package.dart', - 'line': 14, - 'column': 25, - }, + 'file': 'file://$fixturesUrl/pkg/package.dart', + 'line': 14, + 'column': 25, + }, ]; } expectInstances( diff --git a/tools/const_finder/test/fixtures/lib/static_icon_provider.dart b/tools/const_finder/test/fixtures/lib/static_icon_provider.dart index 67c699337fc57..8df5feec46127 100644 --- a/tools/const_finder/test/fixtures/lib/static_icon_provider.dart +++ b/tools/const_finder/test/fixtures/lib/static_icon_provider.dart @@ -7,8 +7,8 @@ import 'target.dart'; void main() { Targets.used1.hit(); Targets.used2.hit(); - final Target used3 = helper(Target.new); - used3.hit(); + final Target nonConstUsed3 = helper(Target.new); + nonConstUsed3.hit(); } Target helper(Target Function(String, int, Target?) tearOff) { From b8d98825501ec9094786500eba1b2ba32b21eb1f Mon Sep 17 00:00:00 2001 From: Christopher Fujino Date: Fri, 9 Dec 2022 14:05:43 -0800 Subject: [PATCH 5/5] fix analysis --- tools/const_finder/test/fixtures/lib/target.dart | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tools/const_finder/test/fixtures/lib/target.dart b/tools/const_finder/test/fixtures/lib/target.dart index c8d7872c4f939..821dade2971ac 100644 --- a/tools/const_finder/test/fixtures/lib/target.dart +++ b/tools/const_finder/test/fixtures/lib/target.dart @@ -15,8 +15,7 @@ class Target { } class ExtendsTarget extends Target { - const ExtendsTarget(String stringValue, int intValue, Target? targetValue) - : super(stringValue, intValue, targetValue); + const ExtendsTarget(super.stringValue, super.intValue, super.targetValue); } class ImplementsTarget implements Target {