[dartdevc] Analyze dartdevc and its platform libraries for dynamic access This adds two dartdevc tests that analyze dartdevc itself and the dartdevc platform libraries, respectively, for uses of dynamic invocation, dynamic get and dynamic set. This is to help reduce the number of these accesses, which are often accidental and unwanted. The current state is allowed using allow-lists. Change-Id: Ib2bb69cc311b0a3d6993da3cd31090a9321e0e64 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/327880 Commit-Queue: Johnni Winther <johnniwinther@google.com> Reviewed-by: Nicholas Shahan <nshahan@google.com>
diff --git a/pkg/dev_compiler/test/dynamic/dartdevc_allowed.json b/pkg/dev_compiler/test/dynamic/dartdevc_allowed.json new file mode 100644 index 0000000..fdec9ca --- /dev/null +++ b/pkg/dev_compiler/test/dynamic/dartdevc_allowed.json
@@ -0,0 +1,9 @@ +{ + "pkg/dev_compiler/lib/src/kernel/expression_compiler_worker.dart": { + "Dynamic invocation of '[]'.": 3 + }, + "pkg/dev_compiler/lib/src/js_ast/template.dart": { + "Dynamic invocation of '[]'.": 10, + "Dynamic invocation of 'toAssignExpression'.": 1 + } +} \ No newline at end of file
diff --git a/pkg/dev_compiler/test/dynamic/dartdevc_dynamic_test.dart b/pkg/dev_compiler/test/dynamic/dartdevc_dynamic_test.dart new file mode 100644 index 0000000..c5624c0 --- /dev/null +++ b/pkg/dev_compiler/test/dynamic/dartdevc_dynamic_test.dart
@@ -0,0 +1,46 @@ +// Copyright (c) 2023, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +import 'package:_fe_analyzer_shared/src/messages/diagnostic_message.dart'; +import 'package:front_end/src/testing/analysis_helper.dart'; +import 'package:front_end/src/testing/dynamic_analysis.dart'; +import 'package:kernel/ast.dart'; + +Future<void> main(List<String> args) async { + await run(dartdevcEntryPoints, + 'pkg/dev_compiler/test/dynamic/dartdevc_allowed.json', + analyzedUrisFilter: dartdevcOnly, + verbose: args.contains('-v'), + generate: args.contains('-g')); +} + +Future<void> run(List<Uri> entryPoints, String allowedListPath, + {bool verbose = false, + bool generate = false, + bool Function(Uri uri)? analyzedUrisFilter}) async { + await runAnalysis(entryPoints, + (DiagnosticMessageHandler onDiagnostic, Component component) { + DynamicVisitor(onDiagnostic, component, allowedListPath, analyzedUrisFilter) + .run(verbose: verbose, generate: generate); + }); +} + +/// Entry points used for analyzing dartdevc code. +final List<Uri> dartdevcEntryPoints = [ + Uri.base.resolve('pkg/dev_compiler/bin/dartdevc.dart') +]; + +/// Filter function used to only analyze dartdevc source code. +bool dartdevcOnly(Uri uri) { + var text = '$uri'; + for (var path in [ + 'package:_js_interop_checks/', + 'package:dev_compiler/', + ]) { + if (text.startsWith(path)) { + return true; + } + } + return false; +}
diff --git a/pkg/dev_compiler/test/dynamic/platform_allowed.json b/pkg/dev_compiler/test/dynamic/platform_allowed.json new file mode 100644 index 0000000..1d80d5c --- /dev/null +++ b/pkg/dev_compiler/test/dynamic/platform_allowed.json
@@ -0,0 +1,78 @@ +{ + "sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/types.dart": { + "Dynamic access of 'length'.": 2, + "Dynamic invocation of '[]'.": 1, + "Dynamic invocation of 'toList'.": 4, + "Dynamic invocation of 'map'.": 4, + "Dynamic access of 'types'.": 1, + "Dynamic access of 'shape'.": 1, + "Dynamic access of 'returnType'.": 1, + "Dynamic access of 'args'.": 1, + "Dynamic access of 'named'.": 2, + "Dynamic access of 'requiredNamed'.": 2, + "Dynamic access of 'isEmpty'.": 1, + "Dynamic access of 'optionals'.": 2, + "Dynamic access of 'typeFormals'.": 1, + "Dynamic invocation of 'instantiateTypeBounds'.": 1, + "Dynamic invocation of 'instantiate'.": 1 + }, + "sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/debugger.dart": { + "Dynamic access of 'last'.": 1, + "Dynamic invocation of 'split'.": 1 + }, + "sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/errors.dart": { + "Dynamic access of 'type'.": 2 + }, + "sdk/lib/_internal/js_dev_runtime/private/debugger.dart": { + "Dynamic invocation of 'toJsonML'.": 1, + "Dynamic update to 'style'.": 3, + "Dynamic access of 'style'.": 2, + "Dynamic invocation of '+'.": 1, + "Dynamic access of 'name'.": 1, + "Dynamic access of 'object'.": 1, + "Dynamic invocation of 'forEach'.": 1, + "Dynamic access of 'length'.": 1, + "Dynamic access of 'key'.": 1, + "Dynamic access of 'value'.": 1, + "Dynamic access of 'start'.": 1, + "Dynamic invocation of '-'.": 1, + "Dynamic access of 'end'.": 1, + "Dynamic invocation of 'children'.": 1 + }, + "sdk/lib/_internal/js_dev_runtime/private/isolate_helper.dart": { + "Dynamic invocation of 'call'.": 1 + }, + "sdk/lib/_internal/js_dev_runtime/private/js_helper.dart": { + "Dynamic access of 'length'.": 1 + }, + "sdk/lib/_internal/js_dev_runtime/private/string_helper.dart": { + "Dynamic access of 'isNotEmpty'.": 1, + "Dynamic invocation of 'allMatches'.": 1 + }, + "sdk/lib/_internal/js_dev_runtime/private/native_typed_data.dart": { + "Dynamic invocation of '|'.": 3 + }, + "sdk/lib/_internal/js_dev_runtime/patch/convert_patch.dart": { + "Dynamic invocation of 'clear'.": 1 + }, + "sdk/lib/convert/json.dart": { + "Dynamic invocation of 'toJson'.": 1 + }, + "sdk/lib/_internal/js_dev_runtime/patch/js_patch.dart": { + "Dynamic invocation of '[]'.": 1 + }, + "sdk/lib/html/dart2js/html_dart2js.dart": { + "Dynamic invocation of 'toList'.": 1, + "Dynamic invocation of 'call'.": 2, + "Dynamic access of 'attributes'.": 1, + "Dynamic invocation of '[]'.": 1, + "Dynamic invocation of 'toLowerCase'.": 1 + }, + "sdk/lib/core/errors.dart": { + "Dynamic access of 'length'.": 2 + }, + "sdk/lib/_internal/js_dev_runtime/patch/core_patch.dart": { + "Dynamic access of 'length'.": 1, + "Dynamic invocation of 'sublist'.": 1 + } +} \ No newline at end of file
diff --git a/pkg/dev_compiler/test/dynamic/platform_dynamic_test.dart b/pkg/dev_compiler/test/dynamic/platform_dynamic_test.dart new file mode 100644 index 0000000..204deac --- /dev/null +++ b/pkg/dev_compiler/test/dynamic/platform_dynamic_test.dart
@@ -0,0 +1,24 @@ +// Copyright (c) 2023, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +import 'package:_fe_analyzer_shared/src/messages/diagnostic_message.dart'; +import 'package:dev_compiler/dev_compiler.dart'; +import 'package:front_end/src/testing/analysis_helper.dart'; +import 'package:front_end/src/testing/dynamic_analysis.dart'; +import 'package:kernel/ast.dart'; +import 'package:kernel/target/targets.dart'; + +Future<void> main(List<String> args) async { + await run('pkg/dev_compiler/test/dynamic/platform_allowed.json', + verbose: args.contains('-v'), generate: args.contains('-g')); +} + +Future<void> run(String allowedListPath, + {bool verbose = false, bool generate = false}) async { + await runPlatformAnalysis(DevCompilerTarget(TargetFlags()), + (DiagnosticMessageHandler onDiagnostic, Component component) { + DynamicVisitor(onDiagnostic, component, allowedListPath, platformOnly) + .run(verbose: verbose, generate: generate); + }); +}
diff --git a/pkg/front_end/lib/src/testing/analysis_helper.dart b/pkg/front_end/lib/src/testing/analysis_helper.dart index b86e4ca..8fc5a3b 100644 --- a/pkg/front_end/lib/src/testing/analysis_helper.dart +++ b/pkg/front_end/lib/src/testing/analysis_helper.dart
@@ -13,27 +13,53 @@ import 'package:kernel/ast.dart'; import 'package:kernel/class_hierarchy.dart'; import 'package:kernel/core_types.dart'; +import 'package:kernel/target/targets.dart'; import 'package:kernel/type_environment.dart'; typedef PerformAnalysisFunction = void Function( DiagnosticMessageHandler onDiagnostic, Component component); typedef UriFilter = bool Function(Uri uri); +/// Analysis the [entryPoints] using [performAnalysis]. Future<void> runAnalysis( List<Uri> entryPoints, PerformAnalysisFunction performAnalysis) async { CompilerOptions options = new CompilerOptions(); options.sdkRoot = computePlatformBinariesLocation(forceBuildDir: true); - options.packagesFileUri = Uri.base.resolve('.dart_tool/package_config.json'); + await _runAnalysis(options, entryPoints, performAnalysis); +} +/// Analysis the platform libraries for [target] using [performAnalysis]. +Future<void> runPlatformAnalysis( + Target target, PerformAnalysisFunction performAnalysis) async { + CompilerOptions options = new CompilerOptions(); + options.target = target; + options.environmentDefines = {}; + options.librariesSpecificationUri = + Uri.base.resolve('sdk/lib/libraries.json'); + Set<Uri> additionalSources = {}; + for (String extraRequiredLibrary in target.extraRequiredLibraries) { + additionalSources.add(Uri.parse(extraRequiredLibrary)); + } + for (String extraRequiredLibrary in target.extraRequiredLibrariesPlatform) { + additionalSources.add(Uri.parse(extraRequiredLibrary)); + } + await _runAnalysis( + options, [Uri.parse('dart:core'), ...additionalSources], performAnalysis); +} + +Future<void> _runAnalysis(CompilerOptions options, Iterable<Uri> entryPoints, + PerformAnalysisFunction performAnalysis) async { + options.packagesFileUri = Uri.base.resolve('.dart_tool/package_config.json'); options.onDiagnostic = (DiagnosticMessage message) { printDiagnosticMessage(message, print); }; InternalCompilerResult compilerResult = await kernelForProgramInternal( - entryPoints.first, options, - retainDataForTesting: true, - requireMain: false, - additionalSources: entryPoints.skip(1).toList()) - as InternalCompilerResult; + entryPoints.first, + options, + retainDataForTesting: true, + requireMain: false, + additionalSources: entryPoints.take(1).toList(), + ) as InternalCompilerResult; performAnalysis(options.onDiagnostic!, compilerResult.component!); } @@ -308,3 +334,6 @@ } return false; } + +/// Filter function used to only analyze platform code. +bool platformOnly(Uri uri) => uri.isScheme('dart');
diff --git a/pkg/front_end/lib/src/testing/dynamic_analysis.dart b/pkg/front_end/lib/src/testing/dynamic_analysis.dart new file mode 100644 index 0000000..5f687bb --- /dev/null +++ b/pkg/front_end/lib/src/testing/dynamic_analysis.dart
@@ -0,0 +1,62 @@ +// Copyright (c) 2023, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +import 'package:_fe_analyzer_shared/src/messages/diagnostic_message.dart'; +import 'package:kernel/ast.dart'; + +import 'analysis_helper.dart'; +import 'verifying_analysis.dart'; + +class DynamicVisitor extends VerifyingAnalysis { + // TODO(johnniwinther): Enable this when it is less noisy. + static const bool checkReturnTypes = false; + + DynamicVisitor(DiagnosticMessageHandler onDiagnostic, Component component, + String? allowedListPath, UriFilter? analyzedUrisFilter) + : super(onDiagnostic, component, allowedListPath, analyzedUrisFilter); + + @override + void visitDynamicGet(DynamicGet node) { + registerError(node, "Dynamic access of '${node.name}'."); + super.visitDynamicGet(node); + } + + @override + void visitDynamicSet(DynamicSet node) { + registerError(node, "Dynamic update to '${node.name}'."); + super.visitDynamicSet(node); + } + + @override + void visitDynamicInvocation(DynamicInvocation node) { + registerError(node, "Dynamic invocation of '${node.name}'."); + super.visitDynamicInvocation(node); + } + + @override + void visitFunctionDeclaration(FunctionDeclaration node) { + if (checkReturnTypes && node.function.returnType is DynamicType) { + registerError(node, "Dynamic return type"); + } + super.visitFunctionDeclaration(node); + } + + @override + void visitFunctionExpression(FunctionExpression node) { + if (checkReturnTypes && node.function.returnType is DynamicType) { + registerError(node, "Dynamic return type"); + } + super.visitFunctionExpression(node); + } + + @override + void visitProcedure(Procedure node) { + if (checkReturnTypes && + node.function.returnType is DynamicType && + node.name.text != 'noSuchMethod') { + registerError(node, "Dynamic return type on $node"); + } + super.visitProcedure(node); + } +}
diff --git a/pkg/front_end/test/static_types/verifying_analysis.dart b/pkg/front_end/lib/src/testing/verifying_analysis.dart similarity index 94% rename from pkg/front_end/test/static_types/verifying_analysis.dart rename to pkg/front_end/lib/src/testing/verifying_analysis.dart index 28678af..ea723bf 100644 --- a/pkg/front_end/test/static_types/verifying_analysis.dart +++ b/pkg/front_end/lib/src/testing/verifying_analysis.dart
@@ -1,4 +1,4 @@ -// Copyright (c) 2020, the Dart project authors. Please see the AUTHORS file +// Copyright (c) 2023, the Dart project authors. Please see the AUTHORS file // for details. All rights reserved. Use of this source code is governed by a // BSD-style license that can be found in the LICENSE file. @@ -6,12 +6,12 @@ import 'dart:io'; import 'package:_fe_analyzer_shared/src/messages/diagnostic_message.dart'; -import 'package:expect/expect.dart'; -import 'package:front_end/src/fasta/command_line_reporting.dart'; -import 'package:front_end/src/fasta/fasta_codes.dart'; -import 'package:front_end/src/testing/analysis_helper.dart'; import 'package:kernel/ast.dart'; +import '../fasta/command_line_reporting.dart'; +import '../fasta/fasta_codes.dart'; +import 'analysis_helper.dart'; + /// [AnalysisVisitor] that supports tracking error/problem occurrences in an /// allowed list file. class VerifyingAnalysis extends AnalysisVisitor { @@ -30,7 +30,7 @@ try { _expectedJson = json.jsonDecode(file.readAsStringSync()); } catch (e) { - Expect.fail('Error reading allowed list from $_allowedListPath: $e'); + throw 'Error reading allowed list from $_allowedListPath: $e'; } } } @@ -175,6 +175,7 @@ }); if (total > 0) { print('${total} error(s) allowed in total.'); + print('Use option -v to see error details.'); } } }
diff --git a/pkg/front_end/test/spell_checking_list_code.txt b/pkg/front_end/test/spell_checking_list_code.txt index be43d88..f456bc9 100644 --- a/pkg/front_end/test/spell_checking_list_code.txt +++ b/pkg/front_end/test/spell_checking_list_code.txt
@@ -899,6 +899,7 @@ linux listenable listening +listing lives ll llub @@ -1007,6 +1008,7 @@ nm nnbd node's +noisy nomenclature nominality nonetheless
diff --git a/pkg/front_end/test/static_types/cfe_dynamic_test.dart b/pkg/front_end/test/static_types/cfe_dynamic_test.dart index 260823d..4815d72 100644 --- a/pkg/front_end/test/static_types/cfe_dynamic_test.dart +++ b/pkg/front_end/test/static_types/cfe_dynamic_test.dart
@@ -3,7 +3,7 @@ // BSD-style license that can be found in the LICENSE file. import 'package:front_end/src/testing/analysis_helper.dart'; -import 'verifying_analysis.dart'; +import 'package:front_end/src/testing/dynamic_analysis.dart'; import 'package:_fe_analyzer_shared/src/messages/diagnostic_message.dart'; import 'package:kernel/ast.dart'; @@ -27,56 +27,3 @@ .run(verbose: verbose, generate: generate); }); } - -class DynamicVisitor extends VerifyingAnalysis { - // TODO(johnniwinther): Enable this when it is less noisy. - static const bool checkReturnTypes = false; - - DynamicVisitor(DiagnosticMessageHandler onDiagnostic, Component component, - String? allowedListPath, UriFilter? analyzedUrisFilter) - : super(onDiagnostic, component, allowedListPath, analyzedUrisFilter); - - @override - void visitDynamicGet(DynamicGet node) { - registerError(node, "Dynamic access of '${node.name}'."); - super.visitDynamicGet(node); - } - - @override - void visitDynamicSet(DynamicSet node) { - registerError(node, "Dynamic update to '${node.name}'."); - super.visitDynamicSet(node); - } - - @override - void visitDynamicInvocation(DynamicInvocation node) { - registerError(node, "Dynamic invocation of '${node.name}'."); - super.visitDynamicInvocation(node); - } - - @override - void visitFunctionDeclaration(FunctionDeclaration node) { - if (checkReturnTypes && node.function.returnType is DynamicType) { - registerError(node, "Dynamic return type"); - } - super.visitFunctionDeclaration(node); - } - - @override - void visitFunctionExpression(FunctionExpression node) { - if (checkReturnTypes && node.function.returnType is DynamicType) { - registerError(node, "Dynamic return type"); - } - super.visitFunctionExpression(node); - } - - @override - void visitProcedure(Procedure node) { - if (checkReturnTypes && - node.function.returnType is DynamicType && - node.name.text != 'noSuchMethod') { - registerError(node, "Dynamic return type on $node"); - } - super.visitProcedure(node); - } -}
diff --git a/pkg/front_end/test/static_types/type_arguments_test.dart b/pkg/front_end/test/static_types/type_arguments_test.dart index 8998846..c3e2727 100644 --- a/pkg/front_end/test/static_types/type_arguments_test.dart +++ b/pkg/front_end/test/static_types/type_arguments_test.dart
@@ -3,7 +3,7 @@ // BSD-style license that can be found in the LICENSE file. import 'package:front_end/src/testing/analysis_helper.dart'; -import 'verifying_analysis.dart'; +import '../../lib/src/testing/verifying_analysis.dart'; import 'package:_fe_analyzer_shared/src/messages/diagnostic_message.dart'; import 'package:kernel/ast.dart';