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, diff --git a/tools/const_finder/bin/main.dart b/tools/const_finder/bin/main.dart index 27f44a95fca42..d20423687ed4d 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, @@ -55,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); @@ -69,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 ebf81962baff4..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 = >[]; @@ -28,6 +30,16 @@ class _ConstVisitor extends RecursiveVisitor { final List> constantInstances; final List> nonConstantLocations; + 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) { @@ -72,12 +84,39 @@ class _ConstVisitor extends RecursiveVisitor { super.visitConstructorInvocation(node); } + @override + void visitClass(Class node) { + // check if this is a class that we should ignore + inIgnoredClass = _classShouldBeIgnored(node); + super.visitClass(node); + inIgnoredClass = false; + } + + // 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 Constant constant = expression.constant; + return constant is InstanceConstant + && constant.classNode.name == annotationClassName + && constant.classNode.enclosingLibrary.importUri.toString() == annotationClassLibraryUri; + }); + } + @override void visitInstanceConstantReference(InstanceConstant node) { super.visitInstanceConstantReference(node); - if (!_matches(node.classNode)) { + if (!_matches(node.classNode) || inIgnoredClass) { return; } + final Map instance = {}; for (final MapEntry kvp in node.fieldValues.entries) { if (kvp.value is! PrimitiveConstant) { @@ -102,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/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 9eac1900b7e97..6c6da5d2bcd8c 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,33 +28,33 @@ void expectInstances(dynamic value, dynamic expected) { } value['constantInstances'].sort(compareByStringValue); expected['constantInstances'].sort(compareByStringValue); - if (!const DeepCollectionEquality().equals(value, expected)) { + + 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(); + } + if (!equality.equals(value, expected)) { stderr.writeln('Expected: ${jsonEncode(expected)}'); stderr.writeln('Actual: ${jsonEncode(value)}'); exitCode = -1; } } -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, Compiler compiler) { stdout.writeln('Checking recursive calls.'); final ConstFinder finder = ConstFinder( - kernelFilePath: boxDill, + kernelFilePath: dillPath, classLibraryUri: 'package:const_finder_fixtures/box.dart', className: 'Box', ); @@ -62,10 +62,10 @@ void _checkRecursion() { jsonEncode(finder.findInstances()); } -void _checkConsts() { +void _checkConsts(String dillPath, Compiler compiler) { stdout.writeln('Checking for expected constants.'); final ConstFinder finder = ConstFinder( - kernelFilePath: constsDill, + kernelFilePath: dillPath, classLibraryUri: 'package:const_finder_fixtures/target.dart', className: 'Target', ); @@ -96,10 +96,11 @@ void _checkConsts() { ], 'nonConstantLocations': [], }, + compiler, ); final ConstFinder finder2 = ConstFinder( - kernelFilePath: constsDill, + kernelFilePath: dillPath, classLibraryUri: 'package:const_finder_fixtures/target.dart', className: 'MixedInTarget', ); @@ -111,13 +112,44 @@ void _checkConsts() { ], 'nonConstantLocations': [], }, + compiler, ); } -void _checkNonConsts() { - stdout.writeln('Checking for non-constant instances.'); +void _checkAnnotation(String dillPath, Compiler compiler) { + stdout.writeln('Checking constant instances in a class annotated with instance of StaticIconProvider are ignored with $compiler'); final ConstFinder finder = ConstFinder( - kernelFilePath: constsAndNonDill, + 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(), + { + 'constantInstances': >[ + { + 'stringValue': 'used1', + 'intValue': 1, + 'targetValue': null, + }, + { + 'stringValue': 'used2', + 'intValue': 2, + 'targetValue': null, + }, + ], + 'nonConstantLocations': [], + }, + compiler, + ); +} + +void _checkNonConstsFrontend(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', className: 'Target', ); @@ -165,40 +197,207 @@ void _checkNonConsts() { } ] }, + compiler, + ); +} + +// 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'); + 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, ); } +void checkProcessResult(ProcessResult result) { + if (result.exitCode != 0) { + stdout.writeln(result.stdout); + stderr.writeln(result.stderr); + } + 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 != 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]; + 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.aot, + ), + _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.aot, + ), + _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.aot, + ), + _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: 'static_icon_provider_frontend', + dartSource: path.join(fixtures, 'lib', 'static_icon_provider.dart'), + frontendServer: frontendServer, + sdkRoot: sdkRoot, + librariesSpec: librariesSpec, + verify: _checkAnnotation, + compiler: Compiler.aot, + ), + _Test( + name: 'static_icon_provider_web', + dartSource: path.join(fixtures, 'lib', 'static_icon_provider.dart'), + frontendServer: frontendServer, + sdkRoot: sdkRoot, + librariesSpec: librariesSpec, + verify: _checkAnnotation, + compiler: Compiler.dart2js, + ), + ]; try { - void checkProcessResult(ProcessResult result) { - if (result.exitCode != 0) { - stdout.writeln(result.stdout); - stderr.writeln(result.stderr); + stdout.writeln('Generating kernel fixtures...'); + + for (final _Test test in tests) { + test.run(); + } + } finally { + try { + for (final _Test test in tests) { + test.dispose(); } - expect(result.exitCode, 0); + } finally { + stdout.writeln('Tests ${exitCode == 0 ? 'succeeded' : 'failed'} - exit code: $exitCode'); } + } +} - stdout.writeln('Generating kernel fixtures...'); - stdout.writeln(consts); +enum Compiler { + // Uses TFA tree-shaking. + aot, + // Does not have TFA tree-shaking. + dart2js, +} - 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, + required this.compiler, + }) : dillPath = path.join(fixtures, '$name.dill'); + + final String name; + final String dartSource; + final String sdkRoot; + final String frontendServer; + final String librariesSpec; + final String dillPath; + void Function(String, Compiler) verify; + final Compiler compiler; + final List resourcesToDispose = []; + + void run() { + stdout.writeln('Compiling $dartSource to $dillPath with $compiler'); + + if (compiler == Compiler.aot) { + _compileAOTDill(); + } else { + _compileWebDill(); + } + + stdout.writeln('Testing $dillPath'); + + verify(dillPath, compiler); + } + + void dispose() { + for (final String resource in resourcesToDispose) { + stdout.writeln('Deleting $resource'); + File(resource).deleteSync(); + } + } + + void _compileAOTDill() { checkProcessResult(Process.runSync(dart, [ frontendServer, '--sdk-root=$sdkRoot', @@ -206,30 +405,40 @@ Future main(List args) async { '--aot', '--tfa', '--packages=$packageConfig', - '--output-dill=$constsDill', - consts, + '--output-dill=$dillPath', + dartSource, ])); - checkProcessResult(Process.runSync(dart, [ - frontendServer, - '--sdk-root=$sdkRoot', - '--target=flutter', - '--aot', - '--tfa', + resourcesToDispose.add(dillPath); + } + + void _compileWebDill() { + final ProcessResult result = Process.runSync(dart, [ + 'compile', + 'js', + '--libraries-spec=$librariesSpec', + '-Ddart.vm.product=true', + '-o', + dillPath, '--packages=$packageConfig', - '--output-dill=$constsAndNonDill', - constsAndNon, - ])); + '--cfe-only', + dartSource, + ]); + checkProcessResult(result); - _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); + } +} + +/// 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/static_icon_provider.dart b/tools/const_finder/test/fixtures/lib/static_icon_provider.dart new file mode 100644 index 0000000000000..ec9121c1c0fc4 --- /dev/null +++ b/tools/const_finder/test/fixtures/lib/static_icon_provider.dart @@ -0,0 +1,25 @@ +// 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(); +} + +@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();