From 09240e4302da4ef8a064c7ce0a166c03b4ec74ef Mon Sep 17 00:00:00 2001 From: Dan Field Date: Tue, 21 Jan 2020 18:53:15 -0800 Subject: [PATCH 1/7] Fix issues in const_finder --- tools/const_finder/bin/main.dart | 5 --- tools/const_finder/lib/const_finder.dart | 40 ++----------------- .../const_finder/test/const_finder_test.dart | 2 - 3 files changed, 4 insertions(+), 43 deletions(-) diff --git a/tools/const_finder/bin/main.dart b/tools/const_finder/bin/main.dart index d29a957118983..3d0cdf4d89121 100644 --- a/tools/const_finder/bin/main.dart +++ b/tools/const_finder/bin/main.dart @@ -43,10 +43,6 @@ void main(List args) { valueHelp: 'path/to/main.dill', help: 'The path to a kernel file to parse, which was created from the ' 'main-package-uri library.') - ..addOption('main-library-uri', - help: 'The package: URI to treat as the main entrypoint library ' - '(the same package used to create the kernel-file).', - valueHelp: 'package:hello_world/main.dart') ..addOption('class-library-uri', help: 'The package: URI of the class to find.', valueHelp: 'package:flutter/src/widgets/icon_data.dart') @@ -73,7 +69,6 @@ void main(List args) { final ConstFinder finder = ConstFinder( kernelFilePath: getArg('kernel-file'), - targetLibraryUri: getArg('main-library-uri'), classLibraryUri: getArg('class-library-uri'), className: getArg('class-name'), ); diff --git a/tools/const_finder/lib/const_finder.dart b/tools/const_finder/lib/const_finder.dart index b40ac249d9901..13f58dde948e2 100644 --- a/tools/const_finder/lib/const_finder.dart +++ b/tools/const_finder/lib/const_finder.dart @@ -8,11 +8,9 @@ import 'package:meta/meta.dart'; class _ConstVisitor extends RecursiveVisitor { _ConstVisitor( this.kernelFilePath, - this.targetLibraryUri, this.classLibraryUri, this.className, ) : assert(kernelFilePath != null), - assert(targetLibraryUri != null), assert(classLibraryUri != null), assert(className != null), constantInstances = >[], @@ -21,9 +19,6 @@ class _ConstVisitor extends RecursiveVisitor { /// The path to the file to open. final String kernelFilePath; - /// The library URI for the main entrypoint of the target library. - final String targetLibraryUri; - /// The library URI for the class to find. final String classLibraryUri; @@ -42,7 +37,7 @@ class _ConstVisitor extends RecursiveVisitor { void visitConstructorInvocation(ConstructorInvocation node) { final Class parentClass = node.target.parent as Class; if (!_matches(parentClass)) { - super.visitConstructorInvocation(node); + return super.visitConstructorInvocation(node); } nonConstantLocations.add({ 'file': node.location.file.toString(), @@ -71,53 +66,26 @@ class ConstFinder { /// be null. /// /// The `kernelFilePath` is the path to a dill (kernel) file to process. - /// - /// The `targetLibraryUri` is the `package:` URI of the main entrypoint to - /// search from. - /// - /// - /// ConstFinder({ @required String kernelFilePath, - @required String targetLibraryUri, @required String classLibraryUri, @required String className, }) : _visitor = _ConstVisitor( kernelFilePath, - targetLibraryUri, classLibraryUri, className, ); final _ConstVisitor _visitor; - Library _getRoot() { - final Component binary = loadComponentFromBinary(_visitor.kernelFilePath); - return binary.libraries.firstWhere( - (Library library) => library.canonicalName.name == _visitor.targetLibraryUri, - orElse: () => throw LibraryNotFoundException._(_visitor.targetLibraryUri), - ); - } - /// Finds all instances Map findInstances() { - final Library root = _getRoot(); - root.visitChildren(_visitor); + for (Library library in loadComponentFromBinary(_visitor.kernelFilePath).libraries) { + library.visitChildren(_visitor); + } return { 'constantInstances': _visitor.constantInstances, 'nonConstantLocations': _visitor.nonConstantLocations, }; } } - -/// Exception thrown by [ConstFinder.findInstances] when the target library -/// is not found. -class LibraryNotFoundException implements Exception { - const LibraryNotFoundException._(this.targetLibraryUri); - - /// The library target URI that could not be found. - final String targetLibraryUri; - - @override - String toString() => 'Could not find target library for "$targetLibraryUri".'; -} diff --git a/tools/const_finder/test/const_finder_test.dart b/tools/const_finder/test/const_finder_test.dart index cc9a2761755f8..478c366070d6f 100644 --- a/tools/const_finder/test/const_finder_test.dart +++ b/tools/const_finder/test/const_finder_test.dart @@ -34,7 +34,6 @@ void _checkConsts() { print('Checking for expected constants.'); final ConstFinder finder = ConstFinder( kernelFilePath: constsDill, - targetLibraryUri: 'package:const_finder_fixtures/consts.dart', classLibraryUri: 'package:const_finder_fixtures/target.dart', className: 'Target', ); @@ -55,7 +54,6 @@ void _checkNonConsts() { print('Checking for non-constant instances.'); final ConstFinder finder = ConstFinder( kernelFilePath: constsAndNonDill, - targetLibraryUri: 'package:const_finder_fixtures/consts_and_non.dart', classLibraryUri: 'package:const_finder_fixtures/target.dart', className: 'Target', ); From 67643232aa131132c36efc62aed4022d880e8861 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Tue, 21 Jan 2020 18:57:36 -0800 Subject: [PATCH 2/7] test --- tools/const_finder/test/fixtures/lib/consts.dart | 6 ++++++ tools/const_finder/test/fixtures/lib/consts_and_non.dart | 7 +++++++ 2 files changed, 13 insertions(+) diff --git a/tools/const_finder/test/fixtures/lib/consts.dart b/tools/const_finder/test/fixtures/lib/consts.dart index ef1969c491deb..39670d989b06f 100644 --- a/tools/const_finder/test/fixtures/lib/consts.dart +++ b/tools/const_finder/test/fixtures/lib/consts.dart @@ -13,5 +13,11 @@ void main() { const Target target3 = Target('3', 3); // should be tree shaken out. target1.hit(); target2.hit(); + + const IgnoreMe ignoreMe = IgnoreMe(); // Should be ignored. + print(ignoreMe); } +class IgnoreMe { + const IgnoreMe(); +} diff --git a/tools/const_finder/test/fixtures/lib/consts_and_non.dart b/tools/const_finder/test/fixtures/lib/consts_and_non.dart index d717faaeae3ae..727b22992db45 100644 --- a/tools/const_finder/test/fixtures/lib/consts_and_non.dart +++ b/tools/const_finder/test/fixtures/lib/consts_and_non.dart @@ -14,4 +14,11 @@ void main() { final Target target3 = Target('3', 3); // should be tree shaken out. target1.hit(); target2.hit(); + + const IgnoreMe ignoreMe = IgnoreMe(); // Should be ignored. + print(ignoreMe); +} + +class IgnoreMe { + const IgnoreMe(); } From 2f7c065bda1023601adedbff365c9787082e462d Mon Sep 17 00:00:00 2001 From: Dan Field Date: Tue, 21 Jan 2020 18:59:08 -0800 Subject: [PATCH 3/7] void return --- tools/const_finder/lib/const_finder.dart | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tools/const_finder/lib/const_finder.dart b/tools/const_finder/lib/const_finder.dart index 13f58dde948e2..f739c7867fda8 100644 --- a/tools/const_finder/lib/const_finder.dart +++ b/tools/const_finder/lib/const_finder.dart @@ -37,7 +37,8 @@ class _ConstVisitor extends RecursiveVisitor { void visitConstructorInvocation(ConstructorInvocation node) { final Class parentClass = node.target.parent as Class; if (!_matches(parentClass)) { - return super.visitConstructorInvocation(node); + super.visitConstructorInvocation(node); + return; } nonConstantLocations.add({ 'file': node.location.file.toString(), From b3a6d4a335df180304b286ee7a441bba5c54d1a3 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Wed, 22 Jan 2020 00:32:43 -0800 Subject: [PATCH 4/7] more --- tools/const_finder/lib/const_finder.dart | 12 ++++++++++- .../const_finder/test/const_finder_test.dart | 18 ++++++++++++---- .../test/fixtures/lib/consts.dart | 18 +++++++++++----- .../test/fixtures/lib/consts_and_non.dart | 21 ++++++++++++++----- .../test/fixtures/lib/target.dart | 3 ++- 5 files changed, 56 insertions(+), 16 deletions(-) diff --git a/tools/const_finder/lib/const_finder.dart b/tools/const_finder/lib/const_finder.dart index f739c7867fda8..1eb8776a91f40 100644 --- a/tools/const_finder/lib/const_finder.dart +++ b/tools/const_finder/lib/const_finder.dart @@ -13,6 +13,7 @@ class _ConstVisitor extends RecursiveVisitor { ) : assert(kernelFilePath != null), assert(classLibraryUri != null), assert(className != null), + _visitedInstnaces = {}, constantInstances = >[], nonConstantLocations = >[]; @@ -25,6 +26,7 @@ class _ConstVisitor extends RecursiveVisitor { /// The name of the class to find. final String className; + final Set _visitedInstnaces; final List> constantInstances; final List> nonConstantLocations; @@ -49,15 +51,22 @@ class _ConstVisitor extends RecursiveVisitor { @override void visitInstanceConstantReference(InstanceConstant node) { + node.fieldValues.values.whereType().forEach(visitInstanceConstantReference); if (!_matches(node.classNode)) { + super.visitInstanceConstantReference(node); return; } final Map instance = {}; for (MapEntry kvp in node.fieldValues.entries) { + if (kvp.value is! PrimitiveConstant) { + continue; + } final PrimitiveConstant value = kvp.value as PrimitiveConstant; instance[kvp.key.canonicalName.name] = value.value; } - constantInstances.add(instance); + if (_visitedInstnaces.add(instance.toString())) { + constantInstances.add(instance); + } } } @@ -81,6 +90,7 @@ class ConstFinder { /// Finds all instances Map findInstances() { + _visitor._visitedInstnaces.clear(); for (Library library in loadComponentFromBinary(_visitor.kernelFilePath).libraries) { library.visitChildren(_visitor); } diff --git a/tools/const_finder/test/const_finder_test.dart b/tools/const_finder/test/const_finder_test.dart index 478c366070d6f..ba115ed1194e4 100644 --- a/tools/const_finder/test/const_finder_test.dart +++ b/tools/const_finder/test/const_finder_test.dart @@ -42,8 +42,11 @@ void _checkConsts() { jsonEncode(finder.findInstances()), jsonEncode({ 'constantInstances': >[ - {'stringValue': '1', 'intValue': 1}, - {'stringValue': '2', 'intValue': 2} + {'stringValue': '1', 'intValue': 1, 'targetValue': null}, + {'stringValue': '4', 'intValue': 4, 'targetValue': null}, + {'stringValue': '2', 'intValue': 2}, + {'stringValue': '6', 'intValue': 6, 'targetValue': null}, + {'stringValue': '7', 'intValue': 7, 'targetValue': null}, ], 'nonConstantLocations': [], }), @@ -62,7 +65,9 @@ void _checkNonConsts() { jsonEncode(finder.findInstances()), jsonEncode({ 'constantInstances': [ - {'stringValue': '1', 'intValue': 1} + {'stringValue': '1', 'intValue': 1, 'targetValue': null}, + {'stringValue': '6', 'intValue': 6, 'targetValue': null}, + {'stringValue': '7', 'intValue': 7, 'targetValue': null}, ], 'nonConstantLocations': [ { @@ -72,7 +77,12 @@ void _checkNonConsts() { }, { 'file': 'file://$fixtures/lib/consts_and_non.dart', - 'line': 14, + 'line': 15, + 'column': 26 + }, + { + 'file': 'file://$fixtures/lib/consts_and_non.dart', + 'line': 17, 'column': 26 }, ] diff --git a/tools/const_finder/test/fixtures/lib/consts.dart b/tools/const_finder/test/fixtures/lib/consts.dart index 39670d989b06f..b59e4a19ef3fc 100644 --- a/tools/const_finder/test/fixtures/lib/consts.dart +++ b/tools/const_finder/test/fixtures/lib/consts.dart @@ -7,17 +7,25 @@ import 'dart:core'; import 'target.dart'; void main() { - const Target target1 = Target('1', 1); - const Target target2 = Target('2', 2); + const Target target1 = Target('1', 1, null); + const Target target2 = Target('2', 2, Target('4', 4, null)); // ignore: unused_local_variable - const Target target3 = Target('3', 3); // should be tree shaken out. + const Target target3 = Target('3', 3, Target('5', 5, null)); // should be tree shaken out. target1.hit(); target2.hit(); - const IgnoreMe ignoreMe = IgnoreMe(); // Should be ignored. + blah(const Target('6', 6, null)); + + const IgnoreMe ignoreMe = IgnoreMe(Target('7', 7, null)); // IgnoreMe is ignored but 7 is not. print(ignoreMe); } class IgnoreMe { - const IgnoreMe(); + const IgnoreMe([this.target]); + + final Target target; +} + +void blah(Target target) { + print(target); } diff --git a/tools/const_finder/test/fixtures/lib/consts_and_non.dart b/tools/const_finder/test/fixtures/lib/consts_and_non.dart index 727b22992db45..2d8a0bbe8913f 100644 --- a/tools/const_finder/test/fixtures/lib/consts_and_non.dart +++ b/tools/const_finder/test/fixtures/lib/consts_and_non.dart @@ -8,17 +8,28 @@ import 'dart:core'; import 'target.dart'; void main() { - const Target target1 = Target('1', 1); - final Target target2 = Target('2', 2); + const Target target1 = Target('1', 1, null); + final Target target2 = Target('2', 2, const Target('4', 4, null)); + + // ignore: unused_local_variable + final Target target3 = Target('3', 3, Target('5', 5, null)); // should be tree shaken out. // ignore: unused_local_variable - final Target target3 = Target('3', 3); // should be tree shaken out. + final Target target6 = Target('6', 6, null); // should be tree shaken out. target1.hit(); target2.hit(); - const IgnoreMe ignoreMe = IgnoreMe(); // Should be ignored. + blah(const Target('6', 6, null)); + + const IgnoreMe ignoreMe = IgnoreMe(Target('7', 7, null)); // IgnoreMe is ignored but 7 is not. print(ignoreMe); } class IgnoreMe { - const IgnoreMe(); + const IgnoreMe([this.target]); + + final Target target; +} + +void blah(Target target) { + print(target); } diff --git a/tools/const_finder/test/fixtures/lib/target.dart b/tools/const_finder/test/fixtures/lib/target.dart index 779fa8691c334..4046216970ed9 100644 --- a/tools/const_finder/test/fixtures/lib/target.dart +++ b/tools/const_finder/test/fixtures/lib/target.dart @@ -3,10 +3,11 @@ // found in the LICENSE file. class Target { - const Target(this.stringValue, this.intValue); + const Target(this.stringValue, this.intValue, this.targetValue); final String stringValue; final int intValue; + final Target targetValue; void hit() { print('$stringValue $intValue'); From 3f869e23a9dfbbb755a18e69501695a5e524ed37 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Wed, 22 Jan 2020 07:39:40 -0800 Subject: [PATCH 5/7] test more cases --- tools/const_finder/test/const_finder_test.dart | 6 ++++++ tools/const_finder/test/fixtures/lib/consts.dart | 6 ++++++ tools/const_finder/test/fixtures/lib/consts_and_non.dart | 6 ++++++ 3 files changed, 18 insertions(+) diff --git a/tools/const_finder/test/const_finder_test.dart b/tools/const_finder/test/const_finder_test.dart index ba115ed1194e4..9ac9e7c376f12 100644 --- a/tools/const_finder/test/const_finder_test.dart +++ b/tools/const_finder/test/const_finder_test.dart @@ -46,6 +46,9 @@ void _checkConsts() { {'stringValue': '4', 'intValue': 4, 'targetValue': null}, {'stringValue': '2', 'intValue': 2}, {'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}, ], 'nonConstantLocations': [], @@ -67,6 +70,9 @@ void _checkNonConsts() { 'constantInstances': [ {'stringValue': '1', 'intValue': 1, '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}, ], 'nonConstantLocations': [ diff --git a/tools/const_finder/test/fixtures/lib/consts.dart b/tools/const_finder/test/fixtures/lib/consts.dart index b59e4a19ef3fc..63176e8dfe289 100644 --- a/tools/const_finder/test/fixtures/lib/consts.dart +++ b/tools/const_finder/test/fixtures/lib/consts.dart @@ -17,7 +17,13 @@ void main() { blah(const Target('6', 6, null)); const IgnoreMe ignoreMe = IgnoreMe(Target('7', 7, null)); // IgnoreMe is ignored but 7 is not. + // ignore: prefer_const_constructors + final IgnoreMe ignoreMe2 = IgnoreMe(const Target('8', 8, null)); + // ignore: prefer_const_constructors + final IgnoreMe ignoreMe3 = IgnoreMe(const Target('9', 9, Target('10', 10, null))); print(ignoreMe); + print(ignoreMe2); + print(ignoreMe3); } class IgnoreMe { diff --git a/tools/const_finder/test/fixtures/lib/consts_and_non.dart b/tools/const_finder/test/fixtures/lib/consts_and_non.dart index 2d8a0bbe8913f..9c832fa263170 100644 --- a/tools/const_finder/test/fixtures/lib/consts_and_non.dart +++ b/tools/const_finder/test/fixtures/lib/consts_and_non.dart @@ -21,7 +21,13 @@ void main() { blah(const Target('6', 6, null)); const IgnoreMe ignoreMe = IgnoreMe(Target('7', 7, null)); // IgnoreMe is ignored but 7 is not. + // ignore: prefer_const_constructors + final IgnoreMe ignoreMe2 = IgnoreMe(const Target('8', 8, null)); + // ignore: prefer_const_constructors + final IgnoreMe ignoreMe3 = IgnoreMe(const Target('9', 9, Target('10', 10, null))); print(ignoreMe); + print(ignoreMe2); + print(ignoreMe3); } class IgnoreMe { From 0d6040d1c297e441db008f6f4fbe845e0a797285 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Wed, 22 Jan 2020 10:46:58 -0800 Subject: [PATCH 6/7] test pkg --- tools/const_finder/test/const_finder_test.dart | 18 ++++++++++++------ tools/const_finder/test/fixtures/.packages | 1 + .../const_finder/test/fixtures/lib/consts.dart | 4 ++++ .../test/fixtures/lib/consts_and_non.dart | 4 ++++ .../test/fixtures/pkg/package.dart | 12 ++++++++++++ 5 files changed, 33 insertions(+), 6 deletions(-) create mode 100644 tools/const_finder/test/fixtures/pkg/package.dart diff --git a/tools/const_finder/test/const_finder_test.dart b/tools/const_finder/test/const_finder_test.dart index 9ac9e7c376f12..dac3a465a9ee2 100644 --- a/tools/const_finder/test/const_finder_test.dart +++ b/tools/const_finder/test/const_finder_test.dart @@ -50,6 +50,7 @@ void _checkConsts() { {'stringValue': '10', 'intValue': 10, 'targetValue': null}, {'stringValue': '9', 'intValue': 9}, {'stringValue': '7', 'intValue': 7, 'targetValue': null}, + {'stringValue': 'package', 'intValue':-1, 'targetValue': null}, ], 'nonConstantLocations': [], }), @@ -78,19 +79,24 @@ void _checkNonConsts() { 'nonConstantLocations': [ { 'file': 'file://$fixtures/lib/consts_and_non.dart', - 'line': 12, - 'column': 26 + 'line': 14, + 'column': 26, }, { 'file': 'file://$fixtures/lib/consts_and_non.dart', - 'line': 15, - 'column': 26 + 'line': 17, + 'column': 26, }, { 'file': 'file://$fixtures/lib/consts_and_non.dart', - 'line': 17, - 'column': 26 + 'line': 19, + 'column': 26, }, + { + 'file': 'file://$fixtures/pkg/package.dart', + 'line': 10, + 'column': 25, + } ] }), ); diff --git a/tools/const_finder/test/fixtures/.packages b/tools/const_finder/test/fixtures/.packages index 899ade66cd7b0..0ef0f0b0b3019 100644 --- a/tools/const_finder/test/fixtures/.packages +++ b/tools/const_finder/test/fixtures/.packages @@ -1,2 +1,3 @@ # Generated by pub on 2020-01-15 10:08:29.776333. const_finder_fixtures:lib/ +const_finder_fixtures_package:pkg/ \ No newline at end of file diff --git a/tools/const_finder/test/fixtures/lib/consts.dart b/tools/const_finder/test/fixtures/lib/consts.dart index 63176e8dfe289..1fd3a64bd67cb 100644 --- a/tools/const_finder/test/fixtures/lib/consts.dart +++ b/tools/const_finder/test/fixtures/lib/consts.dart @@ -4,6 +4,8 @@ import 'dart:core'; +import 'package:const_finder_fixtures_package/package.dart'; + import 'target.dart'; void main() { @@ -24,6 +26,8 @@ void main() { print(ignoreMe); print(ignoreMe2); print(ignoreMe3); + + createTargetInPackage(); } class IgnoreMe { diff --git a/tools/const_finder/test/fixtures/lib/consts_and_non.dart b/tools/const_finder/test/fixtures/lib/consts_and_non.dart index 9c832fa263170..9f4eeb8d04271 100644 --- a/tools/const_finder/test/fixtures/lib/consts_and_non.dart +++ b/tools/const_finder/test/fixtures/lib/consts_and_non.dart @@ -5,6 +5,8 @@ // ignore_for_file: prefer_const_constructors import 'dart:core'; +import 'package:const_finder_fixtures_package/package.dart'; + import 'target.dart'; void main() { @@ -28,6 +30,8 @@ void main() { print(ignoreMe); print(ignoreMe2); print(ignoreMe3); + + createNonConstTargetInPackage(); } class IgnoreMe { diff --git a/tools/const_finder/test/fixtures/pkg/package.dart b/tools/const_finder/test/fixtures/pkg/package.dart new file mode 100644 index 0000000000000..860f239be1f6d --- /dev/null +++ b/tools/const_finder/test/fixtures/pkg/package.dart @@ -0,0 +1,12 @@ +import 'package:const_finder_fixtures/target.dart'; + +void createTargetInPackage() { + const Target target = Target('package', -1, null); + target.hit(); +} + +void createNonConstTargetInPackage() { + // ignore: prefer_const_constructors + final Target target = Target('package_non', -2, null); + target.hit(); +} \ No newline at end of file From 340710ffdb2715243dc91a57607fc9de0bd0bfc6 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Wed, 22 Jan 2020 10:47:44 -0800 Subject: [PATCH 7/7] .. --- tools/const_finder/test/fixtures/pkg/package.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/const_finder/test/fixtures/pkg/package.dart b/tools/const_finder/test/fixtures/pkg/package.dart index 860f239be1f6d..62ba73c3575ab 100644 --- a/tools/const_finder/test/fixtures/pkg/package.dart +++ b/tools/const_finder/test/fixtures/pkg/package.dart @@ -9,4 +9,4 @@ void createNonConstTargetInPackage() { // ignore: prefer_const_constructors final Target target = Target('package_non', -2, null); target.hit(); -} \ No newline at end of file +}