From 773cf6b1d58175033ea8c3360ecec0657260f0b2 Mon Sep 17 00:00:00 2001 From: Nate Biggs Date: Tue, 18 Feb 2025 13:08:50 -0800 Subject: [PATCH] [ddc] Fix issues with duplicate library name aliases. The attached bug shows an issue users have been encountering where a constructor seems to be undefined. It turns out this is because DDC is trying to read the constructor from the wrong library. This is happening because both 'package:dio' and a sister package 'package:dio_web_adapter' both contain a library with the same path: 'src/adapter.dart'. 'BrowserHttpClientAdapter' the class they are trying to reference is defined in 'package:dio/src/adapter.dart'. However, due to a naming collision, their import is referencing 'package:dio_web_adapter/src/adapter.dart'. This naming collision happens because of the logic in '_jsLibraryAlias'. By truncating the start of the import URI (i.e. 'dio/' and 'dio_web_adapter/') the two libraries map to the same alias. This alias is then used to as the key in the AMD module export object and since both libraries are in the same module, only the second one gets exported. This code may have been written with the assumption that libraries from different packages would always be in different modules (in which case the shortened paths shouldn't collide) but this is not the case. The fix is to use the full import URI including the package name. In writing the attached modular test I discovered another issue that only affects es6 imports. The ScopedId resolver was not considering NameSpecifier as a declaration point for variables. This lead to a similar name collision since the import alias's name was also being derived from a truncated import URI. In the test, both 'f1/foo.dart' and 'f2/foo.dart' were being imported 'as foo'. Now one is 'as foo' and the other is 'as foo$'. The first issue affects both AMD and es6 while the second issue only affects es6. The modular tests run with es6 so the new test fails if either of these fixes is not in place. The new DDC module system is not affected by either issue since it doesn't use NameSpecifiers and it uses the full import URI as a string to register libraries rather than a shortened alias. Tested on TGP and with a local Flutter application. Bug: https://github.com/dart-lang/sdk/issues/56498 Change-Id: I5bdb945cfbe615874b40e2fc4ebba31b661cf3b7 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/410260 Reviewed-by: Nicholas Shahan Commit-Queue: Nate Biggs --- .../lib/src/compiler/module_builder.dart | 18 +++++++++ pkg/dev_compiler/lib/src/js_ast/printer.dart | 27 +++++++++++++ pkg/dev_compiler/lib/src/kernel/compiler.dart | 38 +++++++------------ .../expression_compiler_e2e_suite.dart | 3 +- tests/modular/issue56498/f1/foo.dart | 7 ++++ tests/modular/issue56498/f2/foo.dart | 5 +++ tests/modular/issue56498/main.dart | 9 +++++ tests/modular/issue56498/modules.yaml | 16 ++++++++ 8 files changed, 98 insertions(+), 25 deletions(-) create mode 100644 tests/modular/issue56498/f1/foo.dart create mode 100644 tests/modular/issue56498/f2/foo.dart create mode 100644 tests/modular/issue56498/main.dart create mode 100644 tests/modular/issue56498/modules.yaml diff --git a/pkg/dev_compiler/lib/src/compiler/module_builder.dart b/pkg/dev_compiler/lib/src/compiler/module_builder.dart index 0a4e2753dfc..f898251706e 100644 --- a/pkg/dev_compiler/lib/src/compiler/module_builder.dart +++ b/pkg/dev_compiler/lib/src/compiler/module_builder.dart @@ -567,6 +567,13 @@ bool isSdkInternalRuntimeUri(Uri importUri) { return importUri.isScheme('dart') && importUri.path == '_runtime'; } +/// Returns a name that can be used to represent a library within the context +/// of a module. This name is not globally unique and therefore should not be +/// used as an import/export name for the library as this can lead to naming +/// collisions. Use [libraryUriToImportName] to ensure global uniqueness. +/// +/// The name should be given to a [ScopedId] to ensure there are no local +/// collisions. String libraryUriToJsIdentifier(Uri importUri) { if (importUri.isScheme('dart')) { return isSdkInternalRuntimeUri(importUri) ? 'dart' : importUri.path; @@ -574,6 +581,17 @@ String libraryUriToJsIdentifier(Uri importUri) { return pathToJSIdentifier(p.withoutExtension(importUri.pathSegments.last)); } +/// Returns a globally unique name that can be used to represent a library. +/// Since this name is unique, it can safely be used for imports and exports +/// to/from JS modules. If global uniqueness is not necessary, use +/// [libraryUriToJsIdentifier] which produces shorter names. +String libraryUriToImportName(Uri importUri) { + if (importUri.isScheme('dart')) { + return isSdkInternalRuntimeUri(importUri) ? 'dart' : importUri.path; + } + return pathToJSIdentifier(p.withoutExtension(importUri.path)); +} + /// Creates function name given [moduleName]. String loadFunctionName(String moduleName) => 'load__${pathToJSIdentifier(moduleName.replaceAll('.', '_'))}'; diff --git a/pkg/dev_compiler/lib/src/js_ast/printer.dart b/pkg/dev_compiler/lib/src/js_ast/printer.dart index aa43c8cdfac..2d578f8c32e 100644 --- a/pkg/dev_compiler/lib/src/js_ast/printer.dart +++ b/pkg/dev_compiler/lib/src/js_ast/printer.dart @@ -1760,6 +1760,8 @@ class MinifyRenamer implements LocalNamer { /// Like [BaseVisitor], but calls [declare] for [Identifier] declarations, and /// [visitIdentifier] otherwise. abstract class VariableDeclarationVisitor extends BaseVisitorVoid { + bool _inImportDeclaration = false; + void declare(Identifier node); @override @@ -1827,4 +1829,29 @@ abstract class VariableDeclarationVisitor extends BaseVisitorVoid { element.accept(this); } } + + @override + void visitImportDeclaration(ImportDeclaration node) { + if (node.defaultBinding != null) { + declare(node.defaultBinding!); + } + if (node.namedImports != null) { + _inImportDeclaration = true; + for (var namedImport in node.namedImports!) { + namedImport.accept(this); + } + _inImportDeclaration = false; + } + } + + @override + void visitNameSpecifier(NameSpecifier node) { + final asName = node.asName; + // The specified 'as' name only declares a local name in the context of an + // import. + if (_inImportDeclaration && asName != null) { + declare(asName); + } + node.name?.accept(this); + } } diff --git a/pkg/dev_compiler/lib/src/kernel/compiler.dart b/pkg/dev_compiler/lib/src/kernel/compiler.dart index c1575b33fcd..6cdc5e77156 100644 --- a/pkg/dev_compiler/lib/src/kernel/compiler.dart +++ b/pkg/dev_compiler/lib/src/kernel/compiler.dart @@ -19,14 +19,16 @@ import 'package:kernel/library_index.dart'; import 'package:kernel/src/dart_type_equivalence.dart'; import 'package:kernel/type_algebra.dart'; import 'package:kernel/type_environment.dart'; -import 'package:path/path.dart' as p; import 'package:source_span/source_span.dart' show SourceLocation; import '../command/options.dart' show Options; import '../compiler/js_names.dart' as js_ast; import '../compiler/js_utils.dart' as js_ast; import '../compiler/module_builder.dart' - show isSdkInternalRuntimeUri, libraryUriToJsIdentifier; + show + isSdkInternalRuntimeUri, + libraryUriToImportName, + libraryUriToJsIdentifier; import '../compiler/module_containers.dart' show ModuleItemContainer; import '../compiler/rewrite_async.dart'; import '../js_ast/js_ast.dart' as js_ast; @@ -818,11 +820,6 @@ class ProgramCompiler extends ComputeOnceConstantVisitor return module; } - /// Choose a canonical name from the [library] element. - String _jsLibraryName(Library library) { - return libraryUriToJsIdentifier(library.importUri); - } - /// Choose a module-unique name from the [library] element. /// /// Returns null if no alias exists or there are multiple output paths @@ -834,17 +831,7 @@ class ProgramCompiler extends ComputeOnceConstantVisitor var uri = library.importUri.normalizePath(); if (uri.isScheme('dart')) return null; - Iterable segments; - if (uri.isScheme('package')) { - // Strip the package name. - segments = uri.pathSegments.skip(1); - } else { - segments = uri.pathSegments; - } - - var qualifiedPath = - js_ast.pathToJSIdentifier(p.withoutExtension(segments.join('/'))); - return qualifiedPath == _jsLibraryName(library) ? null : qualifiedPath; + return libraryUriToImportName(uri); } /// Debugger friendly name for a Dart [library]. @@ -7751,7 +7738,8 @@ class ProgramCompiler extends ComputeOnceConstantVisitor void _setEmitIfIncrementalLibrary(Library library) { if (_incrementalMode) { - _setEmitIfIncremental(_libraryToModule(library), _jsLibraryName(library)); + _setEmitIfIncremental(_libraryToModule(library), + libraryUriToJsIdentifier(library.importUri)); } } @@ -7994,7 +7982,7 @@ class ProgramCompiler extends ComputeOnceConstantVisitor } var libraryId = _isBuildingSdk && _isDartLibrary(library, '_rti') ? _rtiLibraryId - : js_ast.ScopedId(_jsLibraryName(library)); + : js_ast.ScopedId(libraryUriToJsIdentifier(library.importUri)); _libraries[library] = libraryId; var alias = _jsLibraryAlias(library); @@ -8048,8 +8036,8 @@ class ProgramCompiler extends ComputeOnceConstantVisitor // It's either one of the libraries in this module, or it's an import. return _libraries[library] ?? - _imports.putIfAbsent( - library, () => js_ast.ScopedId(_jsLibraryName(library))); + _imports.putIfAbsent(library, + () => js_ast.ScopedId(libraryUriToJsIdentifier(library.importUri))); } /// Emits imports into [items]. @@ -8080,7 +8068,8 @@ class ProgramCompiler extends ComputeOnceConstantVisitor var imports = []; for (var library in libraries) { if (!_incrementalMode || - usedLibraries!.contains(_jsLibraryName(library))) { + usedLibraries! + .contains(libraryUriToJsIdentifier(library.importUri))) { var alias = _jsLibraryAlias(library); if (alias != null) { var aliasId = js_ast.ScopedId(alias); @@ -8180,7 +8169,8 @@ class ProgramCompiler extends ComputeOnceConstantVisitor if (usedLibraries.isNotEmpty) { _libraries.forEach((library, libraryId) { - if (usedLibraries.contains(_jsLibraryName(library))) { + if (usedLibraries + .contains(libraryUriToJsIdentifier(library.importUri))) { var alias = _jsLibraryAlias(library); var aliasId = alias == null ? libraryId : js_ast.ScopedId(alias); var asName = alias == null ? null : libraryId; diff --git a/pkg/dev_compiler/test/expression_compiler/expression_compiler_e2e_suite.dart b/pkg/dev_compiler/test/expression_compiler/expression_compiler_e2e_suite.dart index 6cf2aaa4db9..c20d266262b 100644 --- a/pkg/dev_compiler/test/expression_compiler/expression_compiler_e2e_suite.dart +++ b/pkg/dev_compiler/test/expression_compiler/expression_compiler_e2e_suite.dart @@ -171,7 +171,8 @@ class ExpressionEvaluationTestDriver { htmlBootstrapper = testDir.uri.resolve('bootstrapper.html'); var bootstrapFile = File(htmlBootstrapper.toFilePath())..createSync(); var moduleName = compiler.metadata!.name; - var mainLibraryName = compiler.metadataForLibraryUri(input).name; + var mainLibraryName = libraryUriToImportName( + Uri.parse(compiler.metadataForLibraryUri(input).importUri)); var appName = p.relative( p.withoutExtension(compiler.metadataForLibraryUri(input).importUri)); diff --git a/tests/modular/issue56498/f1/foo.dart b/tests/modular/issue56498/f1/foo.dart new file mode 100644 index 00000000000..da18d02c6ba --- /dev/null +++ b/tests/modular/issue56498/f1/foo.dart @@ -0,0 +1,7 @@ +// Copyright (c) 2025, 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. + +export 'package:f2/foo.dart'; + +class F1 {} diff --git a/tests/modular/issue56498/f2/foo.dart b/tests/modular/issue56498/f2/foo.dart new file mode 100644 index 00000000000..441fea993ba --- /dev/null +++ b/tests/modular/issue56498/f2/foo.dart @@ -0,0 +1,5 @@ +// Copyright (c) 2025, 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. + +class F2 {} diff --git a/tests/modular/issue56498/main.dart b/tests/modular/issue56498/main.dart new file mode 100644 index 00000000000..75acf0234de --- /dev/null +++ b/tests/modular/issue56498/main.dart @@ -0,0 +1,9 @@ +// Copyright (c) 2025, 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:f1/foo.dart'; + +main() { + print(F1()); + print(F2()); +} diff --git a/tests/modular/issue56498/modules.yaml b/tests/modular/issue56498/modules.yaml new file mode 100644 index 00000000000..93a434d0dac --- /dev/null +++ b/tests/modular/issue56498/modules.yaml @@ -0,0 +1,16 @@ +# Copyright (c) 2025, 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. +# +# Test ensuring that the modular compiler works properly with `package:` +# imports. This test also ensures that the dart2js implementation of the modular +# test pipeline works as intended. The test is not designed to cover any +# compiler or language feature explicitly. +dependencies: + main: [f1, expect] + f1: f2 + +packages: + f0: . + f1: f1 + f2: f2