Skip to content

Commit

Permalink
Avoid namespace conflicts (#78)
Browse files Browse the repository at this point in the history 
* Avoid reusing the same namespace twice

Until now, the module migrator always used the default namespace for
`@use` rules. This would result in an invalid migration if two `@use`
rules within the same stylesheet have the same default namespace.

This adds a number to the default namespace if a previous `@use` rule
already used it.

* Handle namespace conflicts with built-in modules

* Better logic for resolving namespace conflicts

References now tracks the "source" of a member being referenced, which
can be an existing `@use` rule, an `@import` rule, a built-in module, or
the current stylesheet.

The main migration visitor now uses that source information to determine
most namespaces for a stylesheet before visiting it.

When resolving namespace conflicts, the migrator attempts to use
additional path segments within the `@import` rule before it falls back
to numerical suffixes. Built-in modules also get priority for their
default namespaces.

* Address code review comments

Highlights:
- Each type of ReferenceSource is now its own subtype
- Fixed some bugs in namespace conflict resolution to ensure that
existing `@use` rules always keep their namespaces

* Add more test cases + code review changes
  • Loading branch information
@jathak
jathak authored Sep 30, 2019
1 parent 44efa5c commit 15c3edd
Show file tree
Hide file tree
Showing 11 changed files with 600 additions and 80 deletions.
256 changes: 201 additions & 55 deletions lib/src/migrators/module.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import 'package:sass/src/import_cache.dart';
import 'package:args/args.dart';
import 'package:collection/collection.dart';
import 'package:path/path.dart' as p;
import 'package:sass_migrator/src/util/node_modules_importer.dart';
import 'package:source_span/source_span.dart';
import 'package:tuple/tuple.dart';

Expand All @@ -23,9 +22,11 @@ import '../migration_visitor.dart';
import '../migrator.dart';
import '../patch.dart';
import '../utils.dart';
import '../util/node_modules_importer.dart';

import 'module/built_in_functions.dart';
import 'module/forward_type.dart';
import 'module/reference_source.dart';
import 'module/references.dart';
import 'module/unreferencable_members.dart';
import 'module/unreferencable_type.dart';
Expand Down Expand Up @@ -109,11 +110,17 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
/// Namespaces of modules used in this stylesheet.
Map<Uri, String> _namespaces;

/// Set of canonical URLs that have a `@use` rule in the current stylesheet.
///
/// This includes both `@use` rules migrated from `@import` rules and
/// additional `@use` rules in the set below.
Set<Uri> _usedUrls;

/// Set of additional `@use` rules necessary for referencing members of
/// implicit dependencies / built-in modules.
///
/// This set contains the path provided in the `@use` rule, not the canonical
/// path (e.g. "a" rather than "dir/a.scss").
/// This set contains the full `@use` rule without a semicolon or line break
/// at the end.
Set<String> _additionalUseRules;

/// Set of variables declared outside the current stylesheet that overrode
Expand Down Expand Up @@ -211,9 +218,11 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
String getMigratedContents() {
var results = super.getMigratedContents();
if (results == null) return null;
var uses = _additionalUseRules
.map((use) => '@use "$use"$_semicolonIfNotIndented\n');
return _getEntrypointForwards() + uses.join() + results;
return _getEntrypointForwards() +
_additionalUseRules
.map((use) => "$use$_semicolonIfNotIndented\n")
.join() +
results;
}

/// Returns whether the member named [name] should be forwarded in the
Expand Down Expand Up @@ -304,22 +313,149 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
"Could not find Sass file at '${p.prettyUri(dependency)}'.", context);
}

/// Stores per-file state before visiting [node] and restores it afterwards.
/// Stores per-file state and determines namespaces for this stylesheet before
/// visiting it and restores the per-file state afterwards.
@override
void visitStylesheet(Stylesheet node) {
var oldNamespaces = _namespaces;
var oldHasUseRule = _usedUrls;
var oldAdditionalUseRules = _additionalUseRules;
var oldUseAllowed = _useAllowed;
_namespaces = {};
_additionalUseRules = Set();
_usedUrls = {};
_additionalUseRules = {};
_useAllowed = true;
_namespaces = _determineNamespaces(node.span.sourceUrl);
super.visitStylesheet(node);
_namespaces = oldNamespaces;
_usedUrls = oldHasUseRule;
_additionalUseRules = oldAdditionalUseRules;
_lastUrl = node.span.sourceUrl;
_useAllowed = oldUseAllowed;
}

/// Determines namespaces for all `@use` rules that the stylesheet at [url]
/// will contain after migration.
Map<Uri, String> _determineNamespaces(Uri url) {
var namespaces = <Uri, String>{};
var sourcesByNamespace = <String, Set<ReferenceSource>>{};
for (var reference in references.sources.keys) {
if (reference.span.sourceUrl != url) continue;
var source = references.sources[reference];
var namespace = source.defaultNamespace;
if (namespace == null) continue;

// Existing `@use` rules should always keep their namespaces.
if (source is UseSource) {
namespaces[source.url] = namespace;
} else {
sourcesByNamespace.putIfAbsent(namespace, () => {}).add(source);
}
}

// First assign namespaces to module URLs without conflicts.
var conflictingNamespaces = <String, Set<ReferenceSource>>{};
sourcesByNamespace.forEach((namespace, sources) {
if (sources.length == 1 && !namespaces.containsValue(namespace)) {
namespaces[sources.first.url] = namespace;
} else {
conflictingNamespaces[namespace] = sources;
}
});

// Then resolve conflicts where they exist.
conflictingNamespaces.forEach((namespace, sources) {
_resolveNamespaceConflict(namespace, sources, namespaces);
});
return namespaces;
}

/// Resolves a conflict between a set of sources with the same default
/// namespace, adding namespaces for all of them to [namespaces].
void _resolveNamespaceConflict(String namespace, Set<ReferenceSource> sources,
Map<Uri, String> namespaces) {
// Give first priority to a built-in module.
var builtIns = sources.whereType<BuiltInSource>();
if (builtIns.isNotEmpty) {
namespaces[builtIns.first.url] =
_resolveBuiltInNamespace(namespace, namespaces);
}
// Then handle `@import` rules, in order of path segment count.
for (var sources in _orderSources(sources.whereType<ImportSource>())) {
// We remove the last segment since it's already present in the
// namespace and any segments with dots since they're not valid in a
// namespace.
var paths = {
for (var source in sources)
source: Uri.parse(source.import.url).pathSegments.toList()
..removeLast()
..removeWhere((segment) => segment.contains('.'))
};
// Start each rule's namespace at the default.
var aliases = {for (var source in sources) source: namespace};

// While multiple rules have the same namespace or any rule's
// namespace is already present, add the next path segment to all
// namespaces at once.
while (!valuesAreUnique(aliases) ||
aliases.values.any(namespaces.containsValue)) {
// If any of the rules runs out of path segments, fallback to just
// adding numerical suffixes.
if (paths.values.any((segments) => segments.isEmpty)) {
for (var source in sources) {
namespaces[source.url] =
_incrementUntilAvailable(namespace, namespaces);
}
return;
}
aliases = {
for (var source in sources)
source: '${paths[source].removeLast()}-${aliases[source]}'
};
}
for (var source in sources) {
namespaces[source.url] = aliases[source];
}
}
}

/// If [module] is not already a value in [existingNamespaces], returns it.
/// If it is, but "sass-$module" is not, returns that. Otherwise, returns it
/// with the lowest available number appended to the end.
String _resolveBuiltInNamespace(
String module, Map<Uri, String> existingNamespaces) {
return existingNamespaces.containsValue(module) &&
!existingNamespaces.containsValue('sass-$module')
? 'sass-$module'
: _incrementUntilAvailable(module, existingNamespaces);
}

/// If [defaultNamespace] has not already been used in this
/// [existingNamespaces], returns it. Otherwise, returns it with the lowest
/// available number appended to the end.
String _incrementUntilAvailable(
String defaultNamespace, Map<Uri, String> existingNamespaces) {
var count = 1;
var namespace = defaultNamespace;
while (existingNamespaces.containsValue(namespace)) {
namespace = '$defaultNamespace${++count}';
}
return namespace;
}

/// Given a set of import sources, groups them by the number of path segments
/// and sorts those groups from fewer to more segments.
List<Set<ImportSource>> _orderSources(Iterable<ImportSource> sources) {
var byPathLength = <int, Set<ImportSource>>{};
for (var source in sources) {
var pathSegments = Uri.parse(source.import.url).pathSegments;
byPathLength.putIfAbsent(pathSegments.length, () => {}).add(source);
}
return [
for (var length in byPathLength.keys.toList()..sort())
byPathLength[length]
];
}

/// Visits the children of [node] with a new scope for tracking unreferencable
/// members.
@override
Expand All @@ -332,19 +468,18 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
/// Adds a namespace to any function call that requires it.
@override
void visitFunctionExpression(FunctionExpression node) {
visitInterpolation(node.name);
if (_isCssCompatibilityOverload(node)) return;

var declaration = references.functions[node];
_unreferencable.check(declaration, node);
_renameReference(nameSpan(node), declaration);
_patchNamespaceForFunction(node, declaration, (namespace) {
addPatch(patchBefore(node.name, '$namespace.'));
});
visitArgumentInvocation(node.arguments);
super.visitFunctionExpression(node);
if (references.sources.containsKey(node)) {
var declaration = references.functions[node];
_unreferencable.check(declaration, node);
_renameReference(nameSpan(node), declaration);
_patchNamespaceForFunction(node, declaration, (namespace) {
addPatch(patchBefore(node.name, '$namespace.'));
});
}

if (node.name.asPlain == "get-function") {
declaration = references.getFunctionReferences[node];
var declaration = references.getFunctionReferences[node];
_unreferencable.check(declaration, node);
_renameReference(getStaticNameForGetFunctionCall(node), declaration);

Expand Down Expand Up @@ -423,34 +558,10 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
return;
}
}
_additionalUseRules.add("sass:$namespace");
patchNamespace(namespace);
patchNamespace(_findOrAddBuiltInNamespace(namespace));
if (name != span.text.replaceAll('_', '-')) addPatch(Patch(span, name));
}

/// Returns true if [node] is a function overload that exists to provide
/// compatiblity with plain CSS function calls, and should therefore not be
/// migrated to the module version.
bool _isCssCompatibilityOverload(FunctionExpression node) {
var argument = getOnlyArgument(node.arguments);
switch (node.name.asPlain) {
case 'grayscale':
case 'invert':
case 'opacity':
return argument is NumberExpression;
case 'saturate':
return argument != null;
case 'alpha':
var totalArgs =
node.arguments.positional.length + node.arguments.named.length;
if (totalArgs > 1) return true;
return argument is BinaryOperationExpression &&
argument.operator == BinaryOperator.singleEquals;
default:
return false;
}
}

/// Given a named argument [arg], returns a span from the start of the name
/// to the start of the argument itself (e.g. "$amount: ").
FileSpan _findArgNameSpan(Expression arg) {
Expand Down Expand Up @@ -515,6 +626,7 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
visitDependency(Uri.parse(import.url), import.span);
_upstreamStylesheets.remove(currentUrl);
_originalImports[_lastUrl] = Tuple2(import.url, importer);
var asClause = '';
if (!_useAllowed) {
_unreferencable = _unreferencable.parent;
for (var declaration in references.allDeclarations) {
Expand All @@ -523,7 +635,14 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
declaration, UnreferencableType.globalFromNestedImport);
}
} else {
_namespaces[_lastUrl] = namespaceForPath(import.url);
var defaultNamespace = namespaceForPath(import.url);
// If a member from this dependency is actually referenced, it should
// already have a namespace from [_determineNamespaces], so we just use
// a simple number suffix to resolve conflicts at this point.
_namespaces.putIfAbsent(_lastUrl,
() => _incrementUntilAvailable(defaultNamespace, _namespaces));
var namespace = _namespaces[_lastUrl];
if (namespace != defaultNamespace) asClause = ' as $namespace';
}

// Pass the variables that were configured by the importing file to `with`,
Expand Down Expand Up @@ -591,12 +710,14 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
" with (\n$indent " + configured.join(',\n$indent ') + "\n$indent)";
}
if (!_useAllowed) {
_additionalUseRules.add('sass:meta');
var namespace = _findOrAddBuiltInNamespace('meta');
configuration = configuration.replaceFirst(' with', r', $with:');
addPatch(Patch(node.span,
'@include meta.load-css(${import.span.text}$configuration)'));
'@include $namespace.load-css(${import.span.text}$configuration)'));
} else {
addPatch(Patch(node.span, '@use ${import.span.text}$configuration'));
_usedUrls.add(_lastUrl);
addPatch(
Patch(node.span, '@use ${import.span.text}$asClause$configuration'));
}
}

Expand Down Expand Up @@ -684,18 +805,43 @@ class _ModuleMigrationVisitor extends MigrationVisitor {
return name.substring(prefixToRemove.length);
}

/// Returns the namespace that built-in module [module] is loaded under.
///
/// This adds an additional `@use` rule if [module] has not been loaded yet.
String _findOrAddBuiltInNamespace(String module) {
var url = Uri.parse("sass:$module");
_namespaces.putIfAbsent(
url, () => _resolveBuiltInNamespace(module, _namespaces));
var namespace = _namespaces[url];
if (!_usedUrls.contains(url)) {
_usedUrls.add(url);
var asClause = namespace == module ? '' : ' as $namespace';
_additionalUseRules.add('@use "sass:$module"$asClause');
}
return namespace;
}

/// Finds the namespace for the stylesheet containing [node], adding a new
/// `@use` rule if necessary.
String _namespaceForNode(SassNode node) {
if (node == null) return null;
if (node.span.sourceUrl == currentUrl) return null;
if (!_namespaces.containsKey(node.span.sourceUrl)) {
var url = node.span.sourceUrl;
if (url == currentUrl) return null;
if (!_usedUrls.contains(url)) {
// Add new `@use` rule for indirect dependency
var simplePath = _absoluteUrlToDependency(node.span.sourceUrl);
_additionalUseRules.add(simplePath);
_namespaces[node.span.sourceUrl] = namespaceForPath(simplePath);
var simplePath = _absoluteUrlToDependency(url);
var defaultNamespace = namespaceForPath(simplePath);
// There are a few edge cases where the reference in [node] wasn't tracked
// by [references.sources], so we add a namespace with simple conflict
// resolution if one for this URL doesn't already exist.
_namespaces.putIfAbsent(
url, () => _incrementUntilAvailable(defaultNamespace, _namespaces));
var namespace = _namespaces[url];
var asClause = defaultNamespace == namespace ? '' : ' as $namespace';
_usedUrls.add(url);
_additionalUseRules.add('@use "$simplePath"$asClause');
}
return _namespaces[node.span.sourceUrl];
return _namespaces[url];
}

/// Converts an absolute URL for a stylesheet into the simplest string that
Expand Down
Loading

0 comments on commit 15c3edd

Please sign in to comment.