From f35217cf398ccd41c3fad52ea9a398888509c933 Mon Sep 17 00:00:00 2001 From: Christopher Fujino Date: Wed, 2 Nov 2022 14:23:59 -0700 Subject: [PATCH 01/13] wip --- tools/const_finder/lib/const_finder.dart | 34 +++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/tools/const_finder/lib/const_finder.dart b/tools/const_finder/lib/const_finder.dart index ebf81962baff4..12d52226c6615 100644 --- a/tools/const_finder/lib/const_finder.dart +++ b/tools/const_finder/lib/const_finder.dart @@ -3,6 +3,7 @@ // found in the LICENSE file. import 'dart:collection'; +import 'dart:io' as io; import 'package:kernel/kernel.dart'; @@ -13,6 +14,7 @@ class _ConstVisitor extends RecursiveVisitor { this.className, ) : _visitedInstances = {}, constantInstances = >[], + skippedConstantInstances = >[], nonConstantLocations = >[]; /// The path to the file to open. @@ -26,8 +28,11 @@ class _ConstVisitor extends RecursiveVisitor { final Set _visitedInstances; final List> constantInstances; + final List> skippedConstantInstances; final List> nonConstantLocations; + bool inIconClass = false; + // A cache of previously evaluated classes. static final Map _classHeirarchyCache = {}; bool _matches(Class node) { @@ -72,12 +77,24 @@ class _ConstVisitor extends RecursiveVisitor { super.visitConstructorInvocation(node); } + @override + void visitClass(Class node) { + if (node.name == 'Icons' && node.enclosingLibrary.importUri.toString() == 'package:flutter/src/material/icons.dart') { + inIconClass = true; + super.visitClass(node); + inIconClass = false; + } else { + super.visitClass(node); + } + } + @override void visitInstanceConstantReference(InstanceConstant node) { super.visitInstanceConstantReference(node); if (!_matches(node.classNode)) { return; } + final Map instance = {}; for (final MapEntry kvp in node.fieldValues.entries) { if (kvp.value is! PrimitiveConstant) { @@ -87,11 +104,22 @@ class _ConstVisitor extends RecursiveVisitor { instance[kvp.key.asField.name.text] = value.value; } if (_visitedInstances.add(instance.toString())) { - constantInstances.add(instance); + if (inIconClass) { + skippedConstantInstances.add(instance); + } else { + constantInstances.add(instance); + } } } } +void _printStackTrace(Node node) { + io.stderr.writeln(StackTrace.current); +} + +/// For debugging. +Library? lastLibrary; + /// A kernel AST visitor that finds const references. class ConstFinder { /// Creates a new ConstFinder class. All arguments are required and must not @@ -114,11 +142,15 @@ class ConstFinder { Map findInstances() { _visitor._visitedInstances.clear(); for (final Library library in loadComponentFromBinary(_visitor.kernelFilePath).libraries) { + //io.stderr.writeln('loadComponentFromBinary($library)'); + lastLibrary = library; library.visitChildren(_visitor); } + //io.stderr.flush(); return { 'constantInstances': _visitor.constantInstances, 'nonConstantLocations': _visitor.nonConstantLocations, + 'skippedConstantInstances': _visitor.skippedConstantInstances, }; } } From f06d70a9d122ddbca1a45820514207cb2239fadd Mon Sep 17 00:00:00 2001 From: Christopher Fujino Date: Thu, 3 Nov 2022 15:51:49 -0700 Subject: [PATCH 02/13] extend const_finder_test to compile web dills --- tools/const_finder/lib/const_finder.dart | 7 +- .../const_finder/test/const_finder_test.dart | 207 +++++++++++++----- 2 files changed, 149 insertions(+), 65 deletions(-) diff --git a/tools/const_finder/lib/const_finder.dart b/tools/const_finder/lib/const_finder.dart index 12d52226c6615..fc70185659aab 100644 --- a/tools/const_finder/lib/const_finder.dart +++ b/tools/const_finder/lib/const_finder.dart @@ -3,7 +3,6 @@ // found in the LICENSE file. import 'dart:collection'; -import 'dart:io' as io; import 'package:kernel/kernel.dart'; @@ -113,10 +112,6 @@ class _ConstVisitor extends RecursiveVisitor { } } -void _printStackTrace(Node node) { - io.stderr.writeln(StackTrace.current); -} - /// For debugging. Library? lastLibrary; @@ -150,7 +145,7 @@ class ConstFinder { return { 'constantInstances': _visitor.constantInstances, 'nonConstantLocations': _visitor.nonConstantLocations, - 'skippedConstantInstances': _visitor.skippedConstantInstances, + //'skippedConstantInstances': _visitor.skippedConstantInstances, }; } } diff --git a/tools/const_finder/test/const_finder_test.dart b/tools/const_finder/test/const_finder_test.dart index 9eac1900b7e97..d6ff86d93d3b3 100644 --- a/tools/const_finder/test/const_finder_test.dart +++ b/tools/const_finder/test/const_finder_test.dart @@ -35,26 +35,15 @@ void expectInstances(dynamic value, dynamic expected) { } } -final String basePath = - path.canonicalize(path.join(path.dirname(Platform.script.toFilePath()), '..')); -final String fixtures = path.join(basePath, 'test', 'fixtures'); -final String box = path.join(fixtures, 'lib', 'box.dart'); -final String consts = path.join(fixtures, 'lib', 'consts.dart'); -final String packageConfig = path.join(fixtures, '.dart_tool', 'package_config.json'); -final String constsAndNon = path.join(fixtures, 'lib', 'consts_and_non.dart'); -final String boxDill = path.join(fixtures, 'box.dill'); -final String constsDill = path.join(fixtures, 'consts.dill'); -final String constsAndNonDill = path.join(fixtures, 'consts_and_non.dill'); - // This test is assuming the `dart` used to invoke the tests is compatible // with the version of package:kernel in //third-party/dart/pkg/kernel final String dart = Platform.resolvedExecutable; final String bat = Platform.isWindows ? '.bat' : ''; -void _checkRecursion() { +void _checkRecursion(String dillPath) { stdout.writeln('Checking recursive calls.'); final ConstFinder finder = ConstFinder( - kernelFilePath: boxDill, + kernelFilePath: dillPath, classLibraryUri: 'package:const_finder_fixtures/box.dart', className: 'Box', ); @@ -62,10 +51,10 @@ void _checkRecursion() { jsonEncode(finder.findInstances()); } -void _checkConsts() { +void _checkConsts(String dillPath) { stdout.writeln('Checking for expected constants.'); final ConstFinder finder = ConstFinder( - kernelFilePath: constsDill, + kernelFilePath: dillPath, classLibraryUri: 'package:const_finder_fixtures/target.dart', className: 'Target', ); @@ -99,7 +88,7 @@ void _checkConsts() { ); final ConstFinder finder2 = ConstFinder( - kernelFilePath: constsDill, + kernelFilePath: dillPath, classLibraryUri: 'package:const_finder_fixtures/target.dart', className: 'MixedInTarget', ); @@ -114,10 +103,10 @@ void _checkConsts() { ); } -void _checkNonConsts() { +void _checkNonConsts(String dillPath) { stdout.writeln('Checking for non-constant instances.'); final ConstFinder finder = ConstFinder( - kernelFilePath: constsAndNonDill, + kernelFilePath: dillPath, classLibraryUri: 'package:const_finder_fixtures/target.dart', className: 'Target', ); @@ -168,37 +157,136 @@ void _checkNonConsts() { ); } +void checkProcessResult(ProcessResult result) { + if (result.exitCode != 0) { + stdout.writeln(result.stdout); + stderr.writeln(result.stderr); + } + expect(result.exitCode, 0); +} + Future main(List args) async { - if (args.length != 2) { - stderr.writeln('The first argument must be the path to the forntend server dill.'); + if (args.length != 3) { + stderr.writeln('The first argument must be the path to the frontend server dill.'); stderr.writeln('The second argument must be the path to the flutter_patched_sdk'); + stderr.writeln('The third argument must be the path to libraries.json'); exit(-1); } - final String frontendServer = args[0]; - final String sdkRoot = args[1]; - try { - void checkProcessResult(ProcessResult result) { - if (result.exitCode != 0) { - stdout.writeln(result.stdout); - stderr.writeln(result.stderr); + + TestRunner( + frontendServer: args[0], + sdkRoot: args[1], + librariesSpec: args[2], + ).test(); +} + +final String basePath = + path.canonicalize(path.join(path.dirname(Platform.script.toFilePath()), '..')); +final String fixtures = path.join(basePath, 'test', 'fixtures'); +final String packageConfig = path.join(fixtures, '.dart_tool', 'package_config.json'); + +class TestRunner { + TestRunner({ + required this.frontendServer, + required this.sdkRoot, + required this.librariesSpec, + }); + + //static final String box = path.join(fixtures, 'lib', 'box.dart'); + //static final String consts = path.join(fixtures, 'lib', 'consts.dart'); + //static final String constsAndNon = path.join(fixtures, 'lib', 'consts_and_non.dart'); + + final String frontendServer; + final String sdkRoot; + final String librariesSpec; + + void test() { + final List<_Test> tests = <_Test>[ + _Test( + name: 'box', + dartSource: path.join(fixtures, 'lib', 'box.dart'), + frontendServer: frontendServer, + sdkRoot: sdkRoot, + librariesSpec: librariesSpec, + verify: _checkRecursion, + ), + _Test( + name: 'consts', + dartSource: path.join(fixtures, 'lib', 'consts.dart'), + frontendServer: frontendServer, + sdkRoot: sdkRoot, + librariesSpec: librariesSpec, + verify: _checkConsts, + ), + _Test( + name: 'consts_and_non', + dartSource: path.join(fixtures, 'lib', 'consts_and_non.dart'), + frontendServer: frontendServer, + sdkRoot: sdkRoot, + librariesSpec: librariesSpec, + verify: _checkNonConsts, + ), + ]; + try { + stdout.writeln('Generating kernel fixtures...'); + + for (final _Test test in tests) { + test.run(); + } + } finally { + try { + for (final _Test test in tests) { + test.dispose(); + } + } finally { + stdout.writeln('Tests ${exitCode == 0 ? 'succeeded' : 'failed'} - exit code: $exitCode'); } - expect(result.exitCode, 0); } + } - stdout.writeln('Generating kernel fixtures...'); - stdout.writeln(consts); +} - checkProcessResult(Process.runSync(dart, [ - frontendServer, - '--sdk-root=$sdkRoot', - '--target=flutter', - '--aot', - '--tfa', - '--packages=$packageConfig', - '--output-dill=$boxDill', - box, - ])); +class _Test { + _Test({ + required this.name, + required this.dartSource, + required this.sdkRoot, + required this.verify, + required this.frontendServer, + required this.librariesSpec, + }); + final String name; + final String dartSource; + final String sdkRoot; + final String frontendServer; + final String librariesSpec; + void Function(String dillPath) verify; + + final List resourcesToDispose = []; + + void run() { + final String tfaDill = path.join(fixtures, '$name-tfa.dill'); + stdout.writeln('Compiling $dartSource to $tfaDill'); + _compileTFADill(tfaDill); + stdout.writeln('Testing $tfaDill'); + verify(tfaDill); + + final String webDill = path.join(fixtures, '$name-web.dill'); + stdout.writeln('Compiling $dartSource to $webDill'); + _compileWebDill(webDill, dartSource); + verify(webDill); + stdout.writeln('Testing $webDill'); + } + + void dispose() { + for (final String resource in resourcesToDispose) { + stdout.writeln('Deleting $resource'); + File(resource).deleteSync(); + } + } + + void _compileTFADill(String dillPath) { checkProcessResult(Process.runSync(dart, [ frontendServer, '--sdk-root=$sdkRoot', @@ -206,30 +294,31 @@ Future main(List args) async { '--aot', '--tfa', '--packages=$packageConfig', - '--output-dill=$constsDill', - consts, + '--output-dill=$dillPath', + dartSource, ])); + resourcesToDispose.add(dillPath); + } + + void _compileWebDill(String dillPath, String dartSource) { checkProcessResult(Process.runSync(dart, [ - frontendServer, - '--sdk-root=$sdkRoot', - '--target=flutter', - '--aot', - '--tfa', + 'compile', + 'js', + '--libraries-spec=$librariesSpec', + '-Ddart.vm.product=true', + '-o', + dillPath, '--packages=$packageConfig', - '--output-dill=$constsAndNonDill', - constsAndNon, + '--cfe-only', + dartSource, ])); - _checkRecursion(); - _checkConsts(); - _checkNonConsts(); - } finally { - try { - File(constsDill).deleteSync(); - File(constsAndNonDill).deleteSync(); - } finally { - stdout.writeln('Tests ${exitCode == 0 ? 'succeeded' : 'failed'} - exit code: $exitCode'); - } + resourcesToDispose.add(dillPath); } } + +enum Compiler { + frontendServer, + dart2js, +} From 779e03a9e59d5bba5153581f472db4acbd6288aa Mon Sep 17 00:00:00 2001 From: Christopher Fujino Date: Fri, 4 Nov 2022 15:49:38 -0700 Subject: [PATCH 03/13] add test --- tools/const_finder/lib/const_finder.dart | 40 ++-- tools/const_finder/pubspec.yaml | 2 +- .../const_finder/test/const_finder_test.dart | 206 ++++++++++++++---- .../test/fixtures/lib/denylist.dart | 16 ++ 4 files changed, 200 insertions(+), 64 deletions(-) create mode 100644 tools/const_finder/test/fixtures/lib/denylist.dart diff --git a/tools/const_finder/lib/const_finder.dart b/tools/const_finder/lib/const_finder.dart index fc70185659aab..15c2d61973673 100644 --- a/tools/const_finder/lib/const_finder.dart +++ b/tools/const_finder/lib/const_finder.dart @@ -11,9 +11,9 @@ class _ConstVisitor extends RecursiveVisitor { this.kernelFilePath, this.classLibraryUri, this.className, + this.ignoredClasses, ) : _visitedInstances = {}, constantInstances = >[], - skippedConstantInstances = >[], nonConstantLocations = >[]; /// The path to the file to open. @@ -25,12 +25,15 @@ class _ConstVisitor extends RecursiveVisitor { /// The name of the class to find. final String className; + /// A list of two-element tuples corresponding to library URI and class name + /// that should be ignored. + final List> ignoredClasses; + final Set _visitedInstances; final List> constantInstances; - final List> skippedConstantInstances; final List> nonConstantLocations; - bool inIconClass = false; + bool inIgnoredClass = false; // A cache of previously evaluated classes. static final Map _classHeirarchyCache = {}; @@ -78,13 +81,19 @@ class _ConstVisitor extends RecursiveVisitor { @override void visitClass(Class node) { - if (node.name == 'Icons' && node.enclosingLibrary.importUri.toString() == 'package:flutter/src/material/icons.dart') { - inIconClass = true; - super.visitClass(node); - inIconClass = false; - } else { - super.visitClass(node); + // check if this is a class that we should ignore + for (final List tuple in ignoredClasses) { + final String libraryUri = tuple[0]; + final String className = tuple[1]; + if (node.name == className && node.enclosingLibrary.importUri.toString() == libraryUri) { + inIgnoredClass = true; + super.visitClass(node); + inIgnoredClass = false; + return; + } } + // not an ignored class + super.visitClass(node); } @override @@ -102,10 +111,11 @@ class _ConstVisitor extends RecursiveVisitor { final PrimitiveConstant value = kvp.value as PrimitiveConstant; instance[kvp.key.asField.name.text] = value.value; } - if (_visitedInstances.add(instance.toString())) { - if (inIconClass) { - skippedConstantInstances.add(instance); - } else { + if (!inIgnoredClass) { + if (_visitedInstances.add(instance.toString())) { + if (instance['stringValue'] == 'unused1') { + throw 'whoops'; + } constantInstances.add(instance); } } @@ -125,10 +135,12 @@ class ConstFinder { required String kernelFilePath, required String classLibraryUri, required String className, + List> ignoredClasses = const >[], }) : _visitor = _ConstVisitor( kernelFilePath, classLibraryUri, className, + ignoredClasses, ); final _ConstVisitor _visitor; @@ -137,11 +149,9 @@ class ConstFinder { Map findInstances() { _visitor._visitedInstances.clear(); for (final Library library in loadComponentFromBinary(_visitor.kernelFilePath).libraries) { - //io.stderr.writeln('loadComponentFromBinary($library)'); lastLibrary = library; library.visitChildren(_visitor); } - //io.stderr.flush(); return { 'constantInstances': _visitor.constantInstances, 'nonConstantLocations': _visitor.nonConstantLocations, diff --git a/tools/const_finder/pubspec.yaml b/tools/const_finder/pubspec.yaml index d2193a5bb1e22..4a9ce8e9b4a17 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.12.0 <3.0.0" + sdk: ">=2.17.0 <3.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 d6ff86d93d3b3..49e2a4fa3b7bc 100644 --- a/tools/const_finder/test/const_finder_test.dart +++ b/tools/const_finder/test/const_finder_test.dart @@ -4,7 +4,7 @@ // ignore_for_file: avoid_dynamic_calls -import 'dart:convert' show jsonEncode; +import 'dart:convert'; import 'dart:io'; import 'package:collection/collection.dart'; @@ -19,7 +19,7 @@ void expect(T value, T expected) { } } -void expectInstances(dynamic value, dynamic expected) { +void expectInstances(dynamic value, dynamic expected, Compiler compiler) { // To ensure we ignore insertion order into maps as well as lists we use // DeepCollectionEquality as well as sort the lists. @@ -28,9 +28,18 @@ void expectInstances(dynamic value, dynamic expected) { } value['constantInstances'].sort(compareByStringValue); expected['constantInstances'].sort(compareByStringValue); - if (!const DeepCollectionEquality().equals(value, expected)) { - stderr.writeln('Expected: ${jsonEncode(expected)}'); - stderr.writeln('Actual: ${jsonEncode(value)}'); + + final Equality equality; + if (compiler == Compiler.dart2js) { + (value as Map).remove('nonConstantLocations'); + (expected as Map).remove('nonConstantLocations'); + equality = const Dart2JSDeepCollectionEquality(); + } else { + equality = const DeepCollectionEquality(); + } + if (!equality.equals(value, expected)) { + stderr.writeln('Expected: ${const JsonEncoder.withIndent(' ').convert(expected)}'); + stderr.writeln('Actual: ${const JsonEncoder.withIndent(' ').convert(value)}'); exitCode = -1; } } @@ -40,7 +49,7 @@ void expectInstances(dynamic value, dynamic expected) { final String dart = Platform.resolvedExecutable; final String bat = Platform.isWindows ? '.bat' : ''; -void _checkRecursion(String dillPath) { +void _checkRecursion(String dillPath, Compiler compiler) { stdout.writeln('Checking recursive calls.'); final ConstFinder finder = ConstFinder( kernelFilePath: dillPath, @@ -51,7 +60,7 @@ void _checkRecursion(String dillPath) { jsonEncode(finder.findInstances()); } -void _checkConsts(String dillPath) { +void _checkConsts(String dillPath, Compiler compiler) { stdout.writeln('Checking for expected constants.'); final ConstFinder finder = ConstFinder( kernelFilePath: dillPath, @@ -85,6 +94,7 @@ void _checkConsts(String dillPath) { ], 'nonConstantLocations': [], }, + compiler, ); final ConstFinder finder2 = ConstFinder( @@ -100,11 +110,43 @@ void _checkConsts(String dillPath) { ], 'nonConstantLocations': [], }, + compiler, ); } -void _checkNonConsts(String dillPath) { - stdout.writeln('Checking for non-constant instances.'); +void _checkDenyList(String dillPath, Compiler compiler) { + stdout.writeln('Checking constant instances in a denylist are ignored with $compiler'); + final ConstFinder finder = ConstFinder( + kernelFilePath: dillPath, + classLibraryUri: 'package:const_finder_fixtures/target.dart', + className: 'Target', + ignoredClasses: >[[ + 'package:const_finder_fixtures/denylist.dart', + 'Targets', + ]], + ); + expectInstances( + finder.findInstances(), + { + 'constantInstances': >[ + { + 'stringValue': 'used1', + 'intValue': 1, + 'targetValue': null, + }, + { + 'stringValue': 'used2', + 'intValue': 2, + 'targetValue': null, + }, + ], + 'nonConstantLocations': [], + }, + compiler, + ); +} +void _checkNonConsts(String dillPath, Compiler compiler) { + stdout.writeln('Checking for non-constant instances with $compiler'); final ConstFinder finder = ConstFinder( kernelFilePath: dillPath, classLibraryUri: 'package:const_finder_fixtures/target.dart', @@ -154,6 +196,7 @@ void _checkNonConsts(String dillPath) { } ] }, + Compiler.frontendServer, // TODO foo bar ); } @@ -202,29 +245,77 @@ class TestRunner { void test() { final List<_Test> tests = <_Test>[ + //_Test( + // name: 'box_frontend', + // dartSource: path.join(fixtures, 'lib', 'box.dart'), + // frontendServer: frontendServer, + // sdkRoot: sdkRoot, + // librariesSpec: librariesSpec, + // verify: _checkRecursion, + // compiler: Compiler.frontendServer, + //), + //_Test( + // name: 'box_web', + // dartSource: path.join(fixtures, 'lib', 'box.dart'), + // frontendServer: frontendServer, + // sdkRoot: sdkRoot, + // librariesSpec: librariesSpec, + // verify: _checkRecursion, + // compiler: Compiler.dart2js, + //), + //_Test( + // name: 'consts_frontend', + // dartSource: path.join(fixtures, 'lib', 'consts.dart'), + // frontendServer: frontendServer, + // sdkRoot: sdkRoot, + // librariesSpec: librariesSpec, + // verify: _checkConsts, + // compiler: Compiler.frontendServer, + //), + //_Test( + // name: 'consts_web', + // dartSource: path.join(fixtures, 'lib', 'consts.dart'), + // frontendServer: frontendServer, + // sdkRoot: sdkRoot, + // librariesSpec: librariesSpec, + // verify: _checkConsts, + // compiler: Compiler.dart2js, + //), + //_Test( + // name: 'consts_and_non_frontend', + // dartSource: path.join(fixtures, 'lib', 'consts_and_non.dart'), + // frontendServer: frontendServer, + // sdkRoot: sdkRoot, + // librariesSpec: librariesSpec, + // verify: _checkNonConsts, + // compiler: Compiler.frontendServer, + //), + //_Test( + // name: 'consts_and_non_web', + // dartSource: path.join(fixtures, 'lib', 'consts_and_non.dart'), + // frontendServer: frontendServer, + // sdkRoot: sdkRoot, + // librariesSpec: librariesSpec, + // verify: _checkNonConsts, + // compiler: Compiler.dart2js, + //), + //_Test( + // name: 'denylist_frontend', + // dartSource: path.join(fixtures, 'lib', 'denylist.dart'), + // frontendServer: frontendServer, + // sdkRoot: sdkRoot, + // librariesSpec: librariesSpec, + // verify: _checkDenyList, + // compiler: Compiler.frontendServer, + //), _Test( - name: 'box', - dartSource: path.join(fixtures, 'lib', 'box.dart'), - frontendServer: frontendServer, - sdkRoot: sdkRoot, - librariesSpec: librariesSpec, - verify: _checkRecursion, - ), - _Test( - name: 'consts', - dartSource: path.join(fixtures, 'lib', 'consts.dart'), - frontendServer: frontendServer, - sdkRoot: sdkRoot, - librariesSpec: librariesSpec, - verify: _checkConsts, - ), - _Test( - name: 'consts_and_non', - dartSource: path.join(fixtures, 'lib', 'consts_and_non.dart'), + name: 'denylist_web', + dartSource: path.join(fixtures, 'lib', 'denylist.dart'), frontendServer: frontendServer, sdkRoot: sdkRoot, librariesSpec: librariesSpec, - verify: _checkNonConsts, + verify: _checkDenyList, + compiler: Compiler.dart2js, ), ]; try { @@ -243,7 +334,11 @@ class TestRunner { } } } +} +enum Compiler { + frontendServer, + dart2js, } class _Test { @@ -254,29 +349,32 @@ class _Test { required this.verify, required this.frontendServer, required this.librariesSpec, - }); + required this.compiler, + }) : dillPath = path.join(fixtures, '$name.dill'); final String name; final String dartSource; final String sdkRoot; final String frontendServer; final String librariesSpec; - void Function(String dillPath) verify; + final String dillPath; + void Function(String, Compiler) verify; + final Compiler compiler; final List resourcesToDispose = []; void run() { - final String tfaDill = path.join(fixtures, '$name-tfa.dill'); - stdout.writeln('Compiling $dartSource to $tfaDill'); - _compileTFADill(tfaDill); - stdout.writeln('Testing $tfaDill'); - verify(tfaDill); - - final String webDill = path.join(fixtures, '$name-web.dill'); - stdout.writeln('Compiling $dartSource to $webDill'); - _compileWebDill(webDill, dartSource); - verify(webDill); - stdout.writeln('Testing $webDill'); + stdout.writeln('Compiling $dartSource to $dillPath'); + + if (compiler == Compiler.frontendServer) { + _compileTFADill(); + } else { + _compileWebDill(); + } + + stdout.writeln('Testing $dillPath'); + + verify(dillPath, compiler); } void dispose() { @@ -286,7 +384,7 @@ class _Test { } } - void _compileTFADill(String dillPath) { + void _compileTFADill() { checkProcessResult(Process.runSync(dart, [ frontendServer, '--sdk-root=$sdkRoot', @@ -301,8 +399,8 @@ class _Test { resourcesToDispose.add(dillPath); } - void _compileWebDill(String dillPath, String dartSource) { - checkProcessResult(Process.runSync(dart, [ + void _compileWebDill() { + final ProcessResult result = Process.runSync(dart, [ 'compile', 'js', '--libraries-spec=$librariesSpec', @@ -312,13 +410,25 @@ class _Test { '--packages=$packageConfig', '--cfe-only', dartSource, - ])); + ]); + if ((result.stderr as String).trim().isNotEmpty) { + print(result.stderr); + } + checkProcessResult(result); resourcesToDispose.add(dillPath); } } -enum Compiler { - frontendServer, - dart2js, +/// Equality that casts all [num]'s to [double] before comparing. +class Dart2JSDeepCollectionEquality extends DeepCollectionEquality { + const Dart2JSDeepCollectionEquality(); + + @override + bool equals(Object? e1, Object? e2) { + if (e1 is num && e2 is num) { + return e1.toDouble() == e2.toDouble(); + } + return super.equals(e1, e2); + } } diff --git a/tools/const_finder/test/fixtures/lib/denylist.dart b/tools/const_finder/test/fixtures/lib/denylist.dart new file mode 100644 index 0000000000000..84435f9c6bfb8 --- /dev/null +++ b/tools/const_finder/test/fixtures/lib/denylist.dart @@ -0,0 +1,16 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'target.dart'; + +void main() { + Targets.used1.hit(); + Targets.used2.hit(); +} + +class Targets { + static const Target used1 = Target('used1', 1, null); + static const Target used2 = Target('used2', 2, null); + static const Target unused1 = Target('unused1', 1, null); +} From 6785ce650b96b9331a8e58aa6e8c86bffb3aa279 Mon Sep 17 00:00:00 2001 From: Christopher Fujino Date: Fri, 4 Nov 2022 16:01:02 -0700 Subject: [PATCH 04/13] add back tests, including non-const for web --- .../const_finder/test/const_finder_test.dart | 160 +++++++++++------- 1 file changed, 95 insertions(+), 65 deletions(-) diff --git a/tools/const_finder/test/const_finder_test.dart b/tools/const_finder/test/const_finder_test.dart index 49e2a4fa3b7bc..0eda72c097614 100644 --- a/tools/const_finder/test/const_finder_test.dart +++ b/tools/const_finder/test/const_finder_test.dart @@ -145,7 +145,8 @@ void _checkDenyList(String dillPath, Compiler compiler) { compiler, ); } -void _checkNonConsts(String dillPath, Compiler compiler) { + +void _checkNonConstsFrontend(String dillPath, Compiler compiler) { stdout.writeln('Checking for non-constant instances with $compiler'); final ConstFinder finder = ConstFinder( kernelFilePath: dillPath, @@ -196,7 +197,36 @@ void _checkNonConsts(String dillPath, Compiler compiler) { } ] }, - Compiler.frontendServer, // TODO foo bar + compiler, + ); +} + +// Note, since web dills don't have tree shaking, we aren't able to eliminate +void _checkNonConstsWeb(String dillPath, Compiler compiler) { + assert(compiler == Compiler.dart2js); + stdout.writeln('Checking for non-constant instances with $compiler'); + final ConstFinder finder = ConstFinder( + kernelFilePath: dillPath, + classLibraryUri: 'package:const_finder_fixtures/target.dart', + className: 'Target', + ); + + expectInstances( + finder.findInstances(), + { + 'constantInstances': [ + {'stringValue': '1', 'intValue': 1, 'targetValue': null}, + {'stringValue': '4', 'intValue': 4, 'targetValue': null}, + {'stringValue': '6', 'intValue': 6, 'targetValue': null}, + {'stringValue': '8', 'intValue': 8, 'targetValue': null}, + {'stringValue': '10', 'intValue': 10, 'targetValue': null}, + {'stringValue': '9', 'intValue': 9}, + {'stringValue': '7', 'intValue': 7, 'targetValue': null}, + {'stringValue': 'package', 'intValue': -1, 'targetValue': null}, + ], + 'nonConstantLocations': [] + }, + compiler, ); } @@ -245,69 +275,69 @@ class TestRunner { void test() { final List<_Test> tests = <_Test>[ - //_Test( - // name: 'box_frontend', - // dartSource: path.join(fixtures, 'lib', 'box.dart'), - // frontendServer: frontendServer, - // sdkRoot: sdkRoot, - // librariesSpec: librariesSpec, - // verify: _checkRecursion, - // compiler: Compiler.frontendServer, - //), - //_Test( - // name: 'box_web', - // dartSource: path.join(fixtures, 'lib', 'box.dart'), - // frontendServer: frontendServer, - // sdkRoot: sdkRoot, - // librariesSpec: librariesSpec, - // verify: _checkRecursion, - // compiler: Compiler.dart2js, - //), - //_Test( - // name: 'consts_frontend', - // dartSource: path.join(fixtures, 'lib', 'consts.dart'), - // frontendServer: frontendServer, - // sdkRoot: sdkRoot, - // librariesSpec: librariesSpec, - // verify: _checkConsts, - // compiler: Compiler.frontendServer, - //), - //_Test( - // name: 'consts_web', - // dartSource: path.join(fixtures, 'lib', 'consts.dart'), - // frontendServer: frontendServer, - // sdkRoot: sdkRoot, - // librariesSpec: librariesSpec, - // verify: _checkConsts, - // compiler: Compiler.dart2js, - //), - //_Test( - // name: 'consts_and_non_frontend', - // dartSource: path.join(fixtures, 'lib', 'consts_and_non.dart'), - // frontendServer: frontendServer, - // sdkRoot: sdkRoot, - // librariesSpec: librariesSpec, - // verify: _checkNonConsts, - // compiler: Compiler.frontendServer, - //), - //_Test( - // name: 'consts_and_non_web', - // dartSource: path.join(fixtures, 'lib', 'consts_and_non.dart'), - // frontendServer: frontendServer, - // sdkRoot: sdkRoot, - // librariesSpec: librariesSpec, - // verify: _checkNonConsts, - // compiler: Compiler.dart2js, - //), - //_Test( - // name: 'denylist_frontend', - // dartSource: path.join(fixtures, 'lib', 'denylist.dart'), - // frontendServer: frontendServer, - // sdkRoot: sdkRoot, - // librariesSpec: librariesSpec, - // verify: _checkDenyList, - // compiler: Compiler.frontendServer, - //), + _Test( + name: 'box_frontend', + dartSource: path.join(fixtures, 'lib', 'box.dart'), + frontendServer: frontendServer, + sdkRoot: sdkRoot, + librariesSpec: librariesSpec, + verify: _checkRecursion, + compiler: Compiler.frontendServer, + ), + _Test( + name: 'box_web', + dartSource: path.join(fixtures, 'lib', 'box.dart'), + frontendServer: frontendServer, + sdkRoot: sdkRoot, + librariesSpec: librariesSpec, + verify: _checkRecursion, + compiler: Compiler.dart2js, + ), + _Test( + name: 'consts_frontend', + dartSource: path.join(fixtures, 'lib', 'consts.dart'), + frontendServer: frontendServer, + sdkRoot: sdkRoot, + librariesSpec: librariesSpec, + verify: _checkConsts, + compiler: Compiler.frontendServer, + ), + _Test( + name: 'consts_web', + dartSource: path.join(fixtures, 'lib', 'consts.dart'), + frontendServer: frontendServer, + sdkRoot: sdkRoot, + librariesSpec: librariesSpec, + verify: _checkConsts, + compiler: Compiler.dart2js, + ), + _Test( + name: 'consts_and_non_frontend', + dartSource: path.join(fixtures, 'lib', 'consts_and_non.dart'), + frontendServer: frontendServer, + sdkRoot: sdkRoot, + librariesSpec: librariesSpec, + verify: _checkNonConstsFrontend, + compiler: Compiler.frontendServer, + ), + _Test( + name: 'consts_and_non_web', + dartSource: path.join(fixtures, 'lib', 'consts_and_non.dart'), + frontendServer: frontendServer, + sdkRoot: sdkRoot, + librariesSpec: librariesSpec, + verify: _checkNonConstsWeb, + compiler: Compiler.dart2js, + ), + _Test( + name: 'denylist_frontend', + dartSource: path.join(fixtures, 'lib', 'denylist.dart'), + frontendServer: frontendServer, + sdkRoot: sdkRoot, + librariesSpec: librariesSpec, + verify: _checkDenyList, + compiler: Compiler.frontendServer, + ), _Test( name: 'denylist_web', dartSource: path.join(fixtures, 'lib', 'denylist.dart'), From 7abff6dd436c0fb203f2daf1f0baa1711a89b38f Mon Sep 17 00:00:00 2001 From: Christopher Fujino Date: Fri, 4 Nov 2022 16:08:52 -0700 Subject: [PATCH 05/13] update run_tests.py --- testing/run_tests.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/testing/run_tests.py b/testing/run_tests.py index 23553c156ed03..acbfa9c0cac61 100755 --- a/testing/run_tests.py +++ b/testing/run_tests.py @@ -837,7 +837,8 @@ def GatherConstFinderTests(build_dir): '--disable-dart-dev', os.path.join(test_dir, 'const_finder_test.dart'), os.path.join(build_dir, 'gen', 'frontend_server.dart.snapshot'), - os.path.join(build_dir, 'flutter_patched_sdk') + os.path.join(build_dir, 'flutter_patched_sdk'), + os.path.join(build_dir, 'dart-sdk', 'lib', 'libraries.json') ] yield EngineExecutableTask( build_dir, From 66c52366ab199f2d9cb14c0943174f0bc9efc00f Mon Sep 17 00:00:00 2001 From: Christopher Fujino Date: Fri, 4 Nov 2022 16:11:14 -0700 Subject: [PATCH 06/13] fix whitespace --- tools/const_finder/test/const_finder_test.dart | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tools/const_finder/test/const_finder_test.dart b/tools/const_finder/test/const_finder_test.dart index 0eda72c097614..5ce292dfd2a6e 100644 --- a/tools/const_finder/test/const_finder_test.dart +++ b/tools/const_finder/test/const_finder_test.dart @@ -201,7 +201,8 @@ void _checkNonConstsFrontend(String dillPath, Compiler compiler) { ); } -// Note, since web dills don't have tree shaking, we aren't able to eliminate +// Note, since web dills don't have tree shaking, we aren't able to eliminate +// an additional const versus _checkNonConstFrontend. void _checkNonConstsWeb(String dillPath, Compiler compiler) { assert(compiler == Compiler.dart2js); stdout.writeln('Checking for non-constant instances with $compiler'); From e7d11856d7f3f5bdd00911e19ecd9292016c5d4f Mon Sep 17 00:00:00 2001 From: Christopher Fujino Date: Fri, 4 Nov 2022 16:42:35 -0700 Subject: [PATCH 07/13] clean up test file --- .../const_finder/test/const_finder_test.dart | 213 ++++++++---------- 1 file changed, 98 insertions(+), 115 deletions(-) diff --git a/tools/const_finder/test/const_finder_test.dart b/tools/const_finder/test/const_finder_test.dart index 5ce292dfd2a6e..ca3f17a33abe8 100644 --- a/tools/const_finder/test/const_finder_test.dart +++ b/tools/const_finder/test/const_finder_test.dart @@ -38,8 +38,8 @@ void expectInstances(dynamic value, dynamic expected, Compiler compiler) { equality = const DeepCollectionEquality(); } if (!equality.equals(value, expected)) { - stderr.writeln('Expected: ${const JsonEncoder.withIndent(' ').convert(expected)}'); - stderr.writeln('Actual: ${const JsonEncoder.withIndent(' ').convert(value)}'); + stderr.writeln('Expected: ${jsonEncode(expected)}'); + stderr.writeln('Actual: ${jsonEncode(value)}'); exitCode = -1; } } @@ -114,6 +114,7 @@ void _checkConsts(String dillPath, Compiler compiler) { ); } +// Verify constants declared in a class specified in ignoredClasses will be ignored. void _checkDenyList(String dillPath, Compiler compiler) { stdout.writeln('Checking constant instances in a denylist are ignored with $compiler'); final ConstFinder finder = ConstFinder( @@ -239,6 +240,11 @@ void checkProcessResult(ProcessResult result) { expect(result.exitCode, 0); } +final String basePath = + path.canonicalize(path.join(path.dirname(Platform.script.toFilePath()), '..')); +final String fixtures = path.join(basePath, 'test', 'fixtures'); +final String packageConfig = path.join(fixtures, '.dart_tool', 'package_config.json'); + Future main(List args) async { if (args.length != 3) { stderr.writeln('The first argument must be the path to the frontend server dill.'); @@ -247,128 +253,105 @@ Future main(List args) async { exit(-1); } - TestRunner( - frontendServer: args[0], - sdkRoot: args[1], - librariesSpec: args[2], - ).test(); -} - -final String basePath = - path.canonicalize(path.join(path.dirname(Platform.script.toFilePath()), '..')); -final String fixtures = path.join(basePath, 'test', 'fixtures'); -final String packageConfig = path.join(fixtures, '.dart_tool', 'package_config.json'); - -class TestRunner { - TestRunner({ - required this.frontendServer, - required this.sdkRoot, - required this.librariesSpec, - }); - - //static final String box = path.join(fixtures, 'lib', 'box.dart'); - //static final String consts = path.join(fixtures, 'lib', 'consts.dart'); - //static final String constsAndNon = path.join(fixtures, 'lib', 'consts_and_non.dart'); - - final String frontendServer; - final String sdkRoot; - final String librariesSpec; - - void test() { - final List<_Test> tests = <_Test>[ - _Test( - name: 'box_frontend', - dartSource: path.join(fixtures, 'lib', 'box.dart'), - frontendServer: frontendServer, - sdkRoot: sdkRoot, - librariesSpec: librariesSpec, - verify: _checkRecursion, - compiler: Compiler.frontendServer, - ), - _Test( - name: 'box_web', - dartSource: path.join(fixtures, 'lib', 'box.dart'), - frontendServer: frontendServer, - sdkRoot: sdkRoot, - librariesSpec: librariesSpec, - verify: _checkRecursion, - compiler: Compiler.dart2js, - ), - _Test( - name: 'consts_frontend', - dartSource: path.join(fixtures, 'lib', 'consts.dart'), - frontendServer: frontendServer, - sdkRoot: sdkRoot, - librariesSpec: librariesSpec, - verify: _checkConsts, - compiler: Compiler.frontendServer, - ), - _Test( - name: 'consts_web', - dartSource: path.join(fixtures, 'lib', 'consts.dart'), - frontendServer: frontendServer, - sdkRoot: sdkRoot, - librariesSpec: librariesSpec, - verify: _checkConsts, - compiler: Compiler.dart2js, - ), - _Test( - name: 'consts_and_non_frontend', - dartSource: path.join(fixtures, 'lib', 'consts_and_non.dart'), - frontendServer: frontendServer, - sdkRoot: sdkRoot, - librariesSpec: librariesSpec, - verify: _checkNonConstsFrontend, - compiler: Compiler.frontendServer, - ), - _Test( - name: 'consts_and_non_web', - dartSource: path.join(fixtures, 'lib', 'consts_and_non.dart'), - frontendServer: frontendServer, - sdkRoot: sdkRoot, - librariesSpec: librariesSpec, - verify: _checkNonConstsWeb, - compiler: Compiler.dart2js, - ), - _Test( - name: 'denylist_frontend', - dartSource: path.join(fixtures, 'lib', 'denylist.dart'), - frontendServer: frontendServer, - sdkRoot: sdkRoot, - librariesSpec: librariesSpec, - verify: _checkDenyList, - compiler: Compiler.frontendServer, - ), - _Test( - name: 'denylist_web', - dartSource: path.join(fixtures, 'lib', 'denylist.dart'), - frontendServer: frontendServer, - sdkRoot: sdkRoot, - librariesSpec: librariesSpec, - verify: _checkDenyList, - compiler: Compiler.dart2js, - ), - ]; + final String frontendServer = args[0]; + final String sdkRoot = args[1]; + final String librariesSpec = args[2]; + + final List<_Test> tests = <_Test>[ + _Test( + name: 'box_frontend', + dartSource: path.join(fixtures, 'lib', 'box.dart'), + frontendServer: frontendServer, + sdkRoot: sdkRoot, + librariesSpec: librariesSpec, + verify: _checkRecursion, + compiler: Compiler.frontendServer, + ), + _Test( + name: 'box_web', + dartSource: path.join(fixtures, 'lib', 'box.dart'), + frontendServer: frontendServer, + sdkRoot: sdkRoot, + librariesSpec: librariesSpec, + verify: _checkRecursion, + compiler: Compiler.dart2js, + ), + _Test( + name: 'consts_frontend', + dartSource: path.join(fixtures, 'lib', 'consts.dart'), + frontendServer: frontendServer, + sdkRoot: sdkRoot, + librariesSpec: librariesSpec, + verify: _checkConsts, + compiler: Compiler.frontendServer, + ), + _Test( + name: 'consts_web', + dartSource: path.join(fixtures, 'lib', 'consts.dart'), + frontendServer: frontendServer, + sdkRoot: sdkRoot, + librariesSpec: librariesSpec, + verify: _checkConsts, + compiler: Compiler.dart2js, + ), + _Test( + name: 'consts_and_non_frontend', + dartSource: path.join(fixtures, 'lib', 'consts_and_non.dart'), + frontendServer: frontendServer, + sdkRoot: sdkRoot, + librariesSpec: librariesSpec, + verify: _checkNonConstsFrontend, + compiler: Compiler.frontendServer, + ), + _Test( + name: 'consts_and_non_web', + dartSource: path.join(fixtures, 'lib', 'consts_and_non.dart'), + frontendServer: frontendServer, + sdkRoot: sdkRoot, + librariesSpec: librariesSpec, + verify: _checkNonConstsWeb, + compiler: Compiler.dart2js, + ), + _Test( + name: 'denylist_frontend', + dartSource: path.join(fixtures, 'lib', 'denylist.dart'), + frontendServer: frontendServer, + sdkRoot: sdkRoot, + librariesSpec: librariesSpec, + verify: _checkDenyList, + compiler: Compiler.frontendServer, + ), + _Test( + name: 'denylist_web', + dartSource: path.join(fixtures, 'lib', 'denylist.dart'), + frontendServer: frontendServer, + sdkRoot: sdkRoot, + librariesSpec: librariesSpec, + verify: _checkDenyList, + compiler: Compiler.dart2js, + ), + ]; + try { + stdout.writeln('Generating kernel fixtures...'); + + for (final _Test test in tests) { + test.run(); + } + } finally { try { - stdout.writeln('Generating kernel fixtures...'); - for (final _Test test in tests) { - test.run(); + test.dispose(); } } finally { - try { - for (final _Test test in tests) { - test.dispose(); - } - } finally { - stdout.writeln('Tests ${exitCode == 0 ? 'succeeded' : 'failed'} - exit code: $exitCode'); - } + stdout.writeln('Tests ${exitCode == 0 ? 'succeeded' : 'failed'} - exit code: $exitCode'); } } } enum Compiler { + // Uses TFA tree-shaking. frontendServer, + // Does not have TFA tree-shaking. dart2js, } @@ -395,7 +378,7 @@ class _Test { final List resourcesToDispose = []; void run() { - stdout.writeln('Compiling $dartSource to $dillPath'); + stdout.writeln('Compiling $dartSource to $dillPath with $compiler'); if (compiler == Compiler.frontendServer) { _compileTFADill(); From 6d0431dfab1f1943fb823fb832111db63d24277f Mon Sep 17 00:00:00 2001 From: Christopher Fujino Date: Wed, 9 Nov 2022 11:16:39 -0800 Subject: [PATCH 08/13] add new options to cli --- tools/const_finder/bin/main.dart | 65 +++++++++++++++++++++++- tools/const_finder/lib/const_finder.dart | 15 ++---- 2 files changed, 66 insertions(+), 14 deletions(-) diff --git a/tools/const_finder/bin/main.dart b/tools/const_finder/bin/main.dart index 27f44a95fca42..02f388eb3057f 100644 --- a/tools/const_finder/bin/main.dart +++ b/tools/const_finder/bin/main.dart @@ -55,20 +55,46 @@ void main(List args) { ..addFlag('help', abbr: 'h', negatable: false, - help: 'Print usage and exit'); + help: 'Print usage and exit') + ..addMultiOption('ignored-class-name', + help: 'The name of a class whose containing constant instances that ' + 'should not be retained. This is useful in situations--such as ' + 'the web--where unused constants are not tree shaken from dill ' + 'files.\n\nNote: each provided class name must pair with an ' + 'invocation of "--ignored-class-library-uri" and appear in the ' + 'same order.', + valueHelp: 'Icons') + ..addMultiOption('ignored-class-library-uri', + help: 'The package: URI of a class to ignore.\n\nNote: each provided ' + 'class name must pair with an invocation of ' + '"--ignored-class-name" and appear in the same order.', + valueHelp: 'package:flutter/src/material/icons.dart'); final ArgResults argResults = parser.parse(args); - T getArg(String name) => argResults[name] as T; + T getArg(String name) { + try { + return argResults[name] as T; + } catch (err) { + stderr.writeln('Parsing error trying to parse the argument "$name"'); + rethrow; + } + } if (getArg('help')) { stdout.writeln(parser.usage); exit(0); } + final List> ignoredClasses = _pairIgnoredClassNamesAndUris( + getArg('ignored-class-name'), + getArg('ignored-class-library-uri'), + ); + final ConstFinder finder = ConstFinder( kernelFilePath: getArg('kernel-file'), classLibraryUri: getArg('class-library-uri'), className: getArg('class-name'), + ignoredClasses: ignoredClasses, ); final JsonEncoder encoder = getArg('pretty') @@ -77,3 +103,38 @@ void main(List args) { stdout.writeln(encoder.convert(finder.findInstances())); } + +List> _pairIgnoredClassNamesAndUris(Object? names, Object? uris) { + // If the user provided neither option then we will not ignore any classes. + if (names == null && uris == null) { + return const >[]; + } + // If the user provided one of the options but not the other, than this is a + // error! + if (names == null || uris == null) { + stderr.writeln( + 'To ignore specific classes, both "--ignored-class-name" AND ' + '"--ignored-class-library-uri" must be provided in order to uniquely ' + 'identify the class.', + ); + exit(1); + } + + names = names as List; + uris = uris as List; + final List> pairs = >[]; + + if (names.length != uris.length) { + stderr.writeln( + '"--ignored-class-name" was provided ${names.length} time(s) but ' + '"--ignored-class-library-uri" was provided ${uris.length} time(s). ' + 'Each ignored class name must be paired with exactly one ignored class ' + 'library uri, provided in the same order.', + ); + exit(1); + } + for (int i = 0; i < names.length; i++) { + pairs.add([uris[i], names[i]]); + } + return pairs; +} diff --git a/tools/const_finder/lib/const_finder.dart b/tools/const_finder/lib/const_finder.dart index 15c2d61973673..6fc53cbcf338f 100644 --- a/tools/const_finder/lib/const_finder.dart +++ b/tools/const_finder/lib/const_finder.dart @@ -99,7 +99,7 @@ class _ConstVisitor extends RecursiveVisitor { @override void visitInstanceConstantReference(InstanceConstant node) { super.visitInstanceConstantReference(node); - if (!_matches(node.classNode)) { + if (!_matches(node.classNode) || inIgnoredClass) { return; } @@ -111,20 +111,12 @@ class _ConstVisitor extends RecursiveVisitor { final PrimitiveConstant value = kvp.value as PrimitiveConstant; instance[kvp.key.asField.name.text] = value.value; } - if (!inIgnoredClass) { - if (_visitedInstances.add(instance.toString())) { - if (instance['stringValue'] == 'unused1') { - throw 'whoops'; - } - constantInstances.add(instance); - } + if (_visitedInstances.add(instance.toString())) { + constantInstances.add(instance); } } } -/// For debugging. -Library? lastLibrary; - /// A kernel AST visitor that finds const references. class ConstFinder { /// Creates a new ConstFinder class. All arguments are required and must not @@ -149,7 +141,6 @@ class ConstFinder { Map findInstances() { _visitor._visitedInstances.clear(); for (final Library library in loadComponentFromBinary(_visitor.kernelFilePath).libraries) { - lastLibrary = library; library.visitChildren(_visitor); } return { From 4f2a9a348971adbe98445670d3f2adb70596c3eb Mon Sep 17 00:00:00 2001 From: Christopher Fujino Date: Wed, 9 Nov 2022 14:45:29 -0800 Subject: [PATCH 09/13] use annotation rather than explicit deny list --- tools/const_finder/bin/main.dart | 56 +------------------ tools/const_finder/lib/const_finder.dart | 38 +++++++------ .../const_finder/test/const_finder_test.dart | 20 +++---- ...enylist.dart => static_icon_provider.dart} | 9 +++ 4 files changed, 39 insertions(+), 84 deletions(-) rename tools/const_finder/test/fixtures/lib/{denylist.dart => static_icon_provider.dart} (63%) diff --git a/tools/const_finder/bin/main.dart b/tools/const_finder/bin/main.dart index 02f388eb3057f..f8a899f65abc2 100644 --- a/tools/const_finder/bin/main.dart +++ b/tools/const_finder/bin/main.dart @@ -55,20 +55,7 @@ void main(List args) { ..addFlag('help', abbr: 'h', negatable: false, - help: 'Print usage and exit') - ..addMultiOption('ignored-class-name', - help: 'The name of a class whose containing constant instances that ' - 'should not be retained. This is useful in situations--such as ' - 'the web--where unused constants are not tree shaken from dill ' - 'files.\n\nNote: each provided class name must pair with an ' - 'invocation of "--ignored-class-library-uri" and appear in the ' - 'same order.', - valueHelp: 'Icons') - ..addMultiOption('ignored-class-library-uri', - help: 'The package: URI of a class to ignore.\n\nNote: each provided ' - 'class name must pair with an invocation of ' - '"--ignored-class-name" and appear in the same order.', - valueHelp: 'package:flutter/src/material/icons.dart'); + help: 'Print usage and exit'); final ArgResults argResults = parser.parse(args); T getArg(String name) { @@ -85,16 +72,10 @@ void main(List args) { exit(0); } - final List> ignoredClasses = _pairIgnoredClassNamesAndUris( - getArg('ignored-class-name'), - getArg('ignored-class-library-uri'), - ); - final ConstFinder finder = ConstFinder( kernelFilePath: getArg('kernel-file'), classLibraryUri: getArg('class-library-uri'), className: getArg('class-name'), - ignoredClasses: ignoredClasses, ); final JsonEncoder encoder = getArg('pretty') @@ -103,38 +84,3 @@ void main(List args) { stdout.writeln(encoder.convert(finder.findInstances())); } - -List> _pairIgnoredClassNamesAndUris(Object? names, Object? uris) { - // If the user provided neither option then we will not ignore any classes. - if (names == null && uris == null) { - return const >[]; - } - // If the user provided one of the options but not the other, than this is a - // error! - if (names == null || uris == null) { - stderr.writeln( - 'To ignore specific classes, both "--ignored-class-name" AND ' - '"--ignored-class-library-uri" must be provided in order to uniquely ' - 'identify the class.', - ); - exit(1); - } - - names = names as List; - uris = uris as List; - final List> pairs = >[]; - - if (names.length != uris.length) { - stderr.writeln( - '"--ignored-class-name" was provided ${names.length} time(s) but ' - '"--ignored-class-library-uri" was provided ${uris.length} time(s). ' - 'Each ignored class name must be paired with exactly one ignored class ' - 'library uri, provided in the same order.', - ); - exit(1); - } - for (int i = 0; i < names.length; i++) { - pairs.add([uris[i], names[i]]); - } - return pairs; -} diff --git a/tools/const_finder/lib/const_finder.dart b/tools/const_finder/lib/const_finder.dart index 6fc53cbcf338f..7abd493603b61 100644 --- a/tools/const_finder/lib/const_finder.dart +++ b/tools/const_finder/lib/const_finder.dart @@ -11,7 +11,6 @@ class _ConstVisitor extends RecursiveVisitor { this.kernelFilePath, this.classLibraryUri, this.className, - this.ignoredClasses, ) : _visitedInstances = {}, constantInstances = >[], nonConstantLocations = >[]; @@ -25,10 +24,6 @@ class _ConstVisitor extends RecursiveVisitor { /// The name of the class to find. final String className; - /// A list of two-element tuples corresponding to library URI and class name - /// that should be ignored. - final List> ignoredClasses; - final Set _visitedInstances; final List> constantInstances; final List> nonConstantLocations; @@ -82,20 +77,32 @@ class _ConstVisitor extends RecursiveVisitor { @override void visitClass(Class node) { // check if this is a class that we should ignore - for (final List tuple in ignoredClasses) { - final String libraryUri = tuple[0]; - final String className = tuple[1]; - if (node.name == className && node.enclosingLibrary.importUri.toString() == libraryUri) { - inIgnoredClass = true; - super.visitClass(node); - inIgnoredClass = false; - return; - } + if (_isStaticIconProvider(node)) { + inIgnoredClass = true; + super.visitClass(node); + inIgnoredClass = false; + return; } // not an ignored class super.visitClass(node); } + // Constants from classes annotated with an instance of StaticIconProvider + // should be ignored. + bool _isStaticIconProvider(Class node) { + return node.annotations.any((Expression expression) { + if (expression is! ConstantExpression) { + return false; + } + + final DartType type = expression.type; + if (type is InterfaceType && type.classNode.name == 'StaticIconProvider') { + return true; + } + return false; + }); + } + @override void visitInstanceConstantReference(InstanceConstant node) { super.visitInstanceConstantReference(node); @@ -127,12 +134,10 @@ class ConstFinder { required String kernelFilePath, required String classLibraryUri, required String className, - List> ignoredClasses = const >[], }) : _visitor = _ConstVisitor( kernelFilePath, classLibraryUri, className, - ignoredClasses, ); final _ConstVisitor _visitor; @@ -146,7 +151,6 @@ class ConstFinder { return { 'constantInstances': _visitor.constantInstances, 'nonConstantLocations': _visitor.nonConstantLocations, - //'skippedConstantInstances': _visitor.skippedConstantInstances, }; } } diff --git a/tools/const_finder/test/const_finder_test.dart b/tools/const_finder/test/const_finder_test.dart index ca3f17a33abe8..87dc2c7003a8e 100644 --- a/tools/const_finder/test/const_finder_test.dart +++ b/tools/const_finder/test/const_finder_test.dart @@ -115,16 +115,12 @@ void _checkConsts(String dillPath, Compiler compiler) { } // Verify constants declared in a class specified in ignoredClasses will be ignored. -void _checkDenyList(String dillPath, Compiler compiler) { - stdout.writeln('Checking constant instances in a denylist are ignored with $compiler'); +void _checkAnnotation(String dillPath, Compiler compiler) { + stdout.writeln('Checking constant instances in a class annotated @staticConstantProvider are ignored with $compiler'); final ConstFinder finder = ConstFinder( kernelFilePath: dillPath, classLibraryUri: 'package:const_finder_fixtures/target.dart', className: 'Target', - ignoredClasses: >[[ - 'package:const_finder_fixtures/denylist.dart', - 'Targets', - ]], ); expectInstances( finder.findInstances(), @@ -313,21 +309,21 @@ Future main(List args) async { compiler: Compiler.dart2js, ), _Test( - name: 'denylist_frontend', - dartSource: path.join(fixtures, 'lib', 'denylist.dart'), + name: 'static_icon_provider_frontend', + dartSource: path.join(fixtures, 'lib', 'static_icon_provider.dart'), frontendServer: frontendServer, sdkRoot: sdkRoot, librariesSpec: librariesSpec, - verify: _checkDenyList, + verify: _checkAnnotation, compiler: Compiler.frontendServer, ), _Test( - name: 'denylist_web', - dartSource: path.join(fixtures, 'lib', 'denylist.dart'), + name: 'static_icon_provider_web', + dartSource: path.join(fixtures, 'lib', 'static_icon_provider.dart'), frontendServer: frontendServer, sdkRoot: sdkRoot, librariesSpec: librariesSpec, - verify: _checkDenyList, + verify: _checkAnnotation, compiler: Compiler.dart2js, ), ]; diff --git a/tools/const_finder/test/fixtures/lib/denylist.dart b/tools/const_finder/test/fixtures/lib/static_icon_provider.dart similarity index 63% rename from tools/const_finder/test/fixtures/lib/denylist.dart rename to tools/const_finder/test/fixtures/lib/static_icon_provider.dart index 84435f9c6bfb8..ec9121c1c0fc4 100644 --- a/tools/const_finder/test/fixtures/lib/denylist.dart +++ b/tools/const_finder/test/fixtures/lib/static_icon_provider.dart @@ -9,8 +9,17 @@ void main() { Targets.used2.hit(); } +@staticIconProvider class Targets { static const Target used1 = Target('used1', 1, null); static const Target used2 = Target('used2', 2, null); static const Target unused1 = Target('unused1', 1, null); } + +// const_finder explicitly does not retain constants appearing within a class +// with this annotation. +class StaticIconProvider { + const StaticIconProvider(); +} + +const StaticIconProvider staticIconProvider = StaticIconProvider(); From 1263b6d06ae013fc3a99a0a62789f7cb5cbd3f63 Mon Sep 17 00:00:00 2001 From: Christopher Fujino Date: Wed, 9 Nov 2022 15:20:24 -0800 Subject: [PATCH 10/13] clean up --- tools/const_finder/test/const_finder_test.dart | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/tools/const_finder/test/const_finder_test.dart b/tools/const_finder/test/const_finder_test.dart index 87dc2c7003a8e..bb0f283fb6411 100644 --- a/tools/const_finder/test/const_finder_test.dart +++ b/tools/const_finder/test/const_finder_test.dart @@ -31,6 +31,8 @@ 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(); @@ -114,9 +116,8 @@ void _checkConsts(String dillPath, Compiler compiler) { ); } -// Verify constants declared in a class specified in ignoredClasses will be ignored. void _checkAnnotation(String dillPath, Compiler compiler) { - stdout.writeln('Checking constant instances in a class annotated @staticConstantProvider are ignored with $compiler'); + stdout.writeln('Checking constant instances in a class annotated with instance of StaticIconProvider are ignored with $compiler'); final ConstFinder finder = ConstFinder( kernelFilePath: dillPath, classLibraryUri: 'package:const_finder_fixtures/target.dart', @@ -421,9 +422,6 @@ class _Test { '--cfe-only', dartSource, ]); - if ((result.stderr as String).trim().isNotEmpty) { - print(result.stderr); - } checkProcessResult(result); resourcesToDispose.add(dillPath); From ac499c723e3a8cc27cd894b1632860677f20b07a Mon Sep 17 00:00:00 2001 From: Christopher Fujino Date: Wed, 9 Nov 2022 16:20:09 -0800 Subject: [PATCH 11/13] Apply suggestions from code review Co-authored-by: Zachary Anderson --- tools/const_finder/lib/const_finder.dart | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tools/const_finder/lib/const_finder.dart b/tools/const_finder/lib/const_finder.dart index 7abd493603b61..1103a460c9e40 100644 --- a/tools/const_finder/lib/const_finder.dart +++ b/tools/const_finder/lib/const_finder.dart @@ -76,15 +76,15 @@ class _ConstVisitor extends RecursiveVisitor { @override void visitClass(Class node) { - // check if this is a class that we should ignore - if (_isStaticIconProvider(node)) { - inIgnoredClass = true; + // Check if this is a class that we should ignore. + if (!_isStaticIconProvider(node)) { + // Not an ignored class. super.visitClass(node); - inIgnoredClass = false; return; } - // not an ignored class + inIgnoredClass = true; super.visitClass(node); + inIgnoredClass = false; } // Constants from classes annotated with an instance of StaticIconProvider From 09771182edf8c4741094de52e1e509093b885d69 Mon Sep 17 00:00:00 2001 From: Christopher Fujino Date: Wed, 9 Nov 2022 16:34:41 -0800 Subject: [PATCH 12/13] clean up --- tools/const_finder/bin/main.dart | 17 +++++++---------- tools/const_finder/lib/const_finder.dart | 5 +---- 2 files changed, 8 insertions(+), 14 deletions(-) diff --git a/tools/const_finder/bin/main.dart b/tools/const_finder/bin/main.dart index f8a899f65abc2..32321cce43a5b 100644 --- a/tools/const_finder/bin/main.dart +++ b/tools/const_finder/bin/main.dart @@ -42,12 +42,16 @@ void main(List args) { ..addOption('kernel-file', valueHelp: 'path/to/main.dill', help: 'The path to a kernel file to parse, which was created from the ' - 'main-package-uri library.') + 'main-package-uri library.', + mandatory: true) ..addOption('class-library-uri', + mandatory: true, help: 'The package: URI of the class to find.', valueHelp: 'package:flutter/src/widgets/icon_data.dart') ..addOption('class-name', - help: 'The class name for the class to find.', valueHelp: 'IconData') + help: 'The class name for the class to find.', + valueHelp: 'IconData', + mandatory: true) ..addSeparator('Optional arguments:') ..addFlag('pretty', negatable: false, @@ -58,14 +62,7 @@ void main(List args) { help: 'Print usage and exit'); final ArgResults argResults = parser.parse(args); - T getArg(String name) { - try { - return argResults[name] as T; - } catch (err) { - stderr.writeln('Parsing error trying to parse the argument "$name"'); - rethrow; - } - } + T getArg(String name) => argResults[name] as T; if (getArg('help')) { stdout.writeln(parser.usage); diff --git a/tools/const_finder/lib/const_finder.dart b/tools/const_finder/lib/const_finder.dart index 1103a460c9e40..ca176f04dd2a8 100644 --- a/tools/const_finder/lib/const_finder.dart +++ b/tools/const_finder/lib/const_finder.dart @@ -96,10 +96,7 @@ class _ConstVisitor extends RecursiveVisitor { } final DartType type = expression.type; - if (type is InterfaceType && type.classNode.name == 'StaticIconProvider') { - return true; - } - return false; + return type is InterfaceType && type.classNode.name == 'StaticIconProvider'; }); } From 5894e4b329b133f979285bdff7ea554bd2333b73 Mon Sep 17 00:00:00 2001 From: Christopher Fujino Date: Fri, 11 Nov 2022 09:35:52 -0800 Subject: [PATCH 13/13] code review --- tools/const_finder/bin/main.dart | 23 ++++++++++- tools/const_finder/lib/const_finder.dart | 38 +++++++++++++------ .../const_finder/test/const_finder_test.dart | 18 +++++---- 3 files changed, 58 insertions(+), 21 deletions(-) diff --git a/tools/const_finder/bin/main.dart b/tools/const_finder/bin/main.dart index 32321cce43a5b..d20423687ed4d 100644 --- a/tools/const_finder/bin/main.dart +++ b/tools/const_finder/bin/main.dart @@ -59,11 +59,30 @@ void main(List args) { ..addFlag('help', abbr: 'h', negatable: false, - help: 'Print usage and exit'); + help: 'Print usage and exit') + ..addOption('annotation-class-name', + help: 'The class name of the annotation for classes that should be ' + 'ignored.', + valueHelp: 'StaticIconProvider') + ..addOption('annotation-class-library-uri', + help: 'The package: URI of the class of the annotation for classes ' + 'that should be ignored.', + valueHelp: 'package:flutter/src/material/icons.dart'); final ArgResults argResults = parser.parse(args); T getArg(String name) => argResults[name] as T; + final String? annotationClassName = getArg('annotation-class-name'); + final String? annotationClassLibraryUri = getArg('annotation-class-library-uri'); + + final bool annotationClassNameProvided = annotationClassName != null; + final bool annotationClassLibraryUriProvided = annotationClassLibraryUri != null; + if (annotationClassNameProvided != annotationClassLibraryUriProvided) { + throw StateError( + 'If either "--annotation-class-name" or "--annotation-class-library-uri" are provided they both must be', + ); + } + if (getArg('help')) { stdout.writeln(parser.usage); exit(0); @@ -73,6 +92,8 @@ void main(List args) { kernelFilePath: getArg('kernel-file'), classLibraryUri: getArg('class-library-uri'), className: getArg('class-name'), + annotationClassName: annotationClassName, + annotationClassLibraryUri: annotationClassLibraryUri, ); final JsonEncoder encoder = getArg('pretty') diff --git a/tools/const_finder/lib/const_finder.dart b/tools/const_finder/lib/const_finder.dart index ca176f04dd2a8..4af94de2aa285 100644 --- a/tools/const_finder/lib/const_finder.dart +++ b/tools/const_finder/lib/const_finder.dart @@ -11,6 +11,8 @@ class _ConstVisitor extends RecursiveVisitor { this.kernelFilePath, this.classLibraryUri, this.className, + this.annotationClassLibraryUri, + this.annotationClassName, ) : _visitedInstances = {}, constantInstances = >[], nonConstantLocations = >[]; @@ -30,6 +32,14 @@ class _ConstVisitor extends RecursiveVisitor { bool inIgnoredClass = false; + /// The name of the name of the class of the annotation marking classes + /// whose constant references should be ignored. + final String? annotationClassName; + + /// The library URI of the class of the annotation marking classes whose + /// constant references should be ignored. + final String? annotationClassLibraryUri; + // A cache of previously evaluated classes. static final Map _classHeirarchyCache = {}; bool _matches(Class node) { @@ -76,27 +86,27 @@ class _ConstVisitor extends RecursiveVisitor { @override void visitClass(Class node) { - // Check if this is a class that we should ignore. - if (!_isStaticIconProvider(node)) { - // Not an ignored class. - super.visitClass(node); - return; - } - inIgnoredClass = true; + // check if this is a class that we should ignore + inIgnoredClass = _classShouldBeIgnored(node); super.visitClass(node); inIgnoredClass = false; } - // Constants from classes annotated with an instance of StaticIconProvider - // should be ignored. - bool _isStaticIconProvider(Class node) { + // If any annotations on the class match annotationClassName AND + // annotationClassLibraryUri. + bool _classShouldBeIgnored(Class node) { + if (annotationClassName == null || annotationClassLibraryUri == null) { + return false; + } return node.annotations.any((Expression expression) { if (expression is! ConstantExpression) { return false; } - final DartType type = expression.type; - return type is InterfaceType && type.classNode.name == 'StaticIconProvider'; + final Constant constant = expression.constant; + return constant is InstanceConstant + && constant.classNode.name == annotationClassName + && constant.classNode.enclosingLibrary.importUri.toString() == annotationClassLibraryUri; }); } @@ -131,10 +141,14 @@ class ConstFinder { required String kernelFilePath, required String classLibraryUri, required String className, + String? annotationClassLibraryUri, + String? annotationClassName, }) : _visitor = _ConstVisitor( kernelFilePath, classLibraryUri, className, + annotationClassLibraryUri, + annotationClassName, ); final _ConstVisitor _visitor; diff --git a/tools/const_finder/test/const_finder_test.dart b/tools/const_finder/test/const_finder_test.dart index bb0f283fb6411..6c6da5d2bcd8c 100644 --- a/tools/const_finder/test/const_finder_test.dart +++ b/tools/const_finder/test/const_finder_test.dart @@ -122,6 +122,8 @@ void _checkAnnotation(String dillPath, Compiler compiler) { kernelFilePath: dillPath, classLibraryUri: 'package:const_finder_fixtures/target.dart', className: 'Target', + annotationClassName: 'StaticIconProvider', + annotationClassLibraryUri: 'package:const_finder_fixtures/static_icon_provider.dart', ); expectInstances( finder.findInstances(), @@ -262,7 +264,7 @@ Future main(List args) async { sdkRoot: sdkRoot, librariesSpec: librariesSpec, verify: _checkRecursion, - compiler: Compiler.frontendServer, + compiler: Compiler.aot, ), _Test( name: 'box_web', @@ -280,7 +282,7 @@ Future main(List args) async { sdkRoot: sdkRoot, librariesSpec: librariesSpec, verify: _checkConsts, - compiler: Compiler.frontendServer, + compiler: Compiler.aot, ), _Test( name: 'consts_web', @@ -298,7 +300,7 @@ Future main(List args) async { sdkRoot: sdkRoot, librariesSpec: librariesSpec, verify: _checkNonConstsFrontend, - compiler: Compiler.frontendServer, + compiler: Compiler.aot, ), _Test( name: 'consts_and_non_web', @@ -316,7 +318,7 @@ Future main(List args) async { sdkRoot: sdkRoot, librariesSpec: librariesSpec, verify: _checkAnnotation, - compiler: Compiler.frontendServer, + compiler: Compiler.aot, ), _Test( name: 'static_icon_provider_web', @@ -347,7 +349,7 @@ Future main(List args) async { enum Compiler { // Uses TFA tree-shaking. - frontendServer, + aot, // Does not have TFA tree-shaking. dart2js, } @@ -377,8 +379,8 @@ class _Test { void run() { stdout.writeln('Compiling $dartSource to $dillPath with $compiler'); - if (compiler == Compiler.frontendServer) { - _compileTFADill(); + if (compiler == Compiler.aot) { + _compileAOTDill(); } else { _compileWebDill(); } @@ -395,7 +397,7 @@ class _Test { } } - void _compileTFADill() { + void _compileAOTDill() { checkProcessResult(Process.runSync(dart, [ frontendServer, '--sdk-root=$sdkRoot',