From 8592cc31168a11fde31fd8983c33987d7c0f8cba Mon Sep 17 00:00:00 2001 From: Pamela Lozano Date: Mon, 11 Dec 2023 16:15:35 -0800 Subject: [PATCH] Detect dependency loops in module migrator --- CHANGELOG.md | 3 ++ lib/src/migrators/module.dart | 31 +++++++++++++++++ lib/src/util/dependency_graph.dart | 33 +++++++++++++++++++ pubspec.yaml | 2 +- .../namespace_references/loop_error.hrx | 28 ++++++++++++++++ .../loop_error_one_reference.hrx | 24 ++++++++++++++ 6 files changed, 120 insertions(+), 1 deletion(-) create mode 100644 lib/src/util/dependency_graph.dart create mode 100644 test/migrators/module/namespace_references/loop_error.hrx create mode 100644 test/migrators/module/namespace_references/loop_error_one_reference.hrx diff --git a/CHANGELOG.md b/CHANGELOG.md index f2a876be..27bd1429 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,6 @@ +## 2.0.4 +* Detect dependency loops in module migrator fix. + ## 2.0.3 ### Module Migrator diff --git a/lib/src/migrators/module.dart b/lib/src/migrators/module.dart index e1afbf80..0c3b16ba 100644 --- a/lib/src/migrators/module.dart +++ b/lib/src/migrators/module.dart @@ -15,6 +15,7 @@ import '../migration_visitor.dart'; import '../migrator.dart'; import '../patch.dart'; import '../utils.dart'; +import '../util/dependency_graph.dart'; import '../util/member_declaration.dart'; import '../util/node_modules_importer.dart'; @@ -199,6 +200,33 @@ class _ModuleMigrationVisitor extends MigrationVisitor { /// The values of the --forward flag. final Set forwards; + /// Dependencies where keys represent source URIs and values represent imported URIs. + final DependencyGraph _dependencies = DependencyGraph(); + + /// Checks for dependency loops between source and imported paths. + /// + /// This method verifies whether importing a path introduces a circular dependency + /// by checking if the imported path is already mapped as a dependency of the source path. + /// + /// Throws a [MigrationException] if a dependency loop is detected. + /// + /// The [source] parameter is the path where the dependency is checked. + /// The [importedPath] parameter is the path being imported. + void _checkDependency(Uri source, Uri importedPath, FileSpan span) { + if (_dependencies.hasDependency(importedPath, source)) { + // Throw an error indicating a potential loop. + var (sourceUrl, _) = _absoluteUrlToDependency(source); + var (importedPathUrl, _) = _absoluteUrlToDependency(importedPath); + throw MigrationSourceSpanException( + 'Dependency loop detected: ${sourceUrl} -> ${importedPathUrl}.\n' + 'To resolve this issue, consider either of the following:\n' + '1. Remove the import statement that causes the dependency loop.\n' + '2. Declare the variables used in the other stylesheet within the same stylesheet.\n', + span); + } + _dependencies.add(source, importedPath); + } + /// Constructs a new module migration visitor. /// /// [importCache] must be the same one used by [references]. @@ -848,6 +876,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor { withClause.isNotEmpty || references.anyMemberReferenced(canonicalUrl, currentUrl)) { _usedUrls.add(canonicalUrl); + _checkDependency(currentUrl, canonicalUrl, context); rules.add('@use $quotedUrl$asClause$withClause'); } if (normalForwardRules != null) rules.addAll(normalForwardRules); @@ -1223,6 +1252,8 @@ class _ModuleMigrationVisitor extends MigrationVisitor { String? _namespaceForDeclaration(MemberDeclaration declaration) { var url = declaration.sourceUrl; if (url == currentUrl) return null; + // Trace dependencies for loop detection. + _checkDependency(currentUrl, url, declaration.member.span); // If we can load [declaration] from a library entrypoint URL, do so. Choose // the shortest one if there are multiple options. diff --git a/lib/src/util/dependency_graph.dart b/lib/src/util/dependency_graph.dart new file mode 100644 index 00000000..cb3363ac --- /dev/null +++ b/lib/src/util/dependency_graph.dart @@ -0,0 +1,33 @@ +// Copyright 2023 Google LLC +// +// Use of this source code is governed by an MIT-style +// license that can be found in the LICENSE file or at +// https://opensource.org/licenses/MIT. + +/// A graph data structure to manage dependencies between URIs. +/// +/// This class allows adding, removing, and querying dependencies +/// between source URIs and their imported paths. +class DependencyGraph { + final Map> _graph = {}; + + /// Adds a dependency relationship between source and importedPath. + void add(Uri source, Uri importedPath) { + _graph.putIfAbsent(source, () => {}).add(importedPath); + } + + /// Removes a dependency relationship between source and importedPath. + void remove(Uri source, Uri importedPath) { + _graph[source]?.remove(importedPath); + } + + /// Finds all dependencies of a given source. + Set? find(Uri source) { + return _graph[source]; + } + + /// Checks if a specific dependency exists. + bool hasDependency(Uri source, Uri importedPath) { + return _graph.containsKey(source) && _graph[source]!.contains(importedPath); + } +} diff --git a/pubspec.yaml b/pubspec.yaml index df61c114..0b6ecd3e 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -1,5 +1,5 @@ name: sass_migrator -version: 2.0.3 +version: 2.0.4 description: A tool for running migrations on Sass files homepage: https://github.com/sass/migrator diff --git a/test/migrators/module/namespace_references/loop_error.hrx b/test/migrators/module/namespace_references/loop_error.hrx new file mode 100644 index 00000000..ebb5d664 --- /dev/null +++ b/test/migrators/module/namespace_references/loop_error.hrx @@ -0,0 +1,28 @@ +<==> arguments +--migrate-deps + +<==> input/entrypoint.scss +$a: red; +@import "ejemplo"; +a { + background: $b; +} + +<==> input/_ejemplo.scss +$b: blue; +a { + color: $a; +} + +<==> error.txt +Error: Dependency loop detected: entrypoint -> ejemplo. +To resolve this issue, consider either of the following: +1. Remove the import statement that causes the dependency loop. +2. Declare the variables used in the other stylesheet within the same stylesheet. + + , +2 | @import "ejemplo"; + | ^^^^^^^^^ + ' + entrypoint.scss 2:9 root stylesheet +Migration failed! diff --git a/test/migrators/module/namespace_references/loop_error_one_reference.hrx b/test/migrators/module/namespace_references/loop_error_one_reference.hrx new file mode 100644 index 00000000..b25a183e --- /dev/null +++ b/test/migrators/module/namespace_references/loop_error_one_reference.hrx @@ -0,0 +1,24 @@ +<==> arguments +--migrate-deps + +<==> input/entrypoint.scss +$var1: 2; +@import "library"; + +<==> input/_library.scss +a { + margin: $var1; +} + +<==> error.txt +Error: Dependency loop detected: entrypoint -> library. +To resolve this issue, consider either of the following: +1. Remove the import statement that causes the dependency loop. +2. Declare the variables used in the other stylesheet within the same stylesheet. + + , +2 | @import "library"; + | ^^^^^^^^^ + ' + entrypoint.scss 2:9 root stylesheet +Migration failed!