Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Detect dependency loops in module migrator #249

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
## 2.0.4
* Detect dependency loops in module migrator fix.

## 2.0.3

### Module Migrator
Expand Down
31 changes: 31 additions & 0 deletions lib/src/migrators/module.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -199,6 +200,33 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
/// The values of the --forward flag.
final Set<ForwardType> 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].
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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.
Expand Down
33 changes: 33 additions & 0 deletions lib/src/util/dependency_graph.dart
Original file line number Diff line number Diff line change
@@ -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<Uri, Set<Uri>> _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<Uri>? 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);
}
}
2 changes: 1 addition & 1 deletion pubspec.yaml
Original file line number Diff line number Diff line change
@@ -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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,7 @@ $d: 5;

// More than one interpolations
.a {
.b: calc($b - #{$c + 1} + #{$d});
.c: calc(100% - #{$TABLE_TITLE + 2px});
.b: calc($b - #{$c + 1} + #{$d});
}

// Nested
Expand All @@ -36,8 +35,7 @@ $d: 5;

// More than one interpolations
.a {
.b: calc($b - ($c + 1) + $d);
.c: calc(100% - ($TABLE-TITLE + 2px));
.b: calc($b - ($c + 1) + $d);
}

// Nested
Expand Down
28 changes: 28 additions & 0 deletions test/migrators/module/namespace_references/loop_error.hrx
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
<==> arguments
pamelalozano16 marked this conversation as resolved.
Show resolved Hide resolved
--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!
Original file line number Diff line number Diff line change
@@ -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!
Loading