Don't produce name conflict errors due to imports

Closes #888

CHANGELOG.md

@@ -3,6 +3,10 @@
 * **Potentially breaking bug fix:** Members loaded through a nested `@import`
   are no longer ever accessible outside that nested context.
 
+* Don't throw an error when importing two modules that both forward members with
+  the same name. The latter name now takes precedence over the former, as per
+  the specification.
+
 ## 1.23.7
 
 * No user-visible changes.

lib/src/async_environment.dart

@@ -17,6 +17,7 @@ import 'exception.dart';
 import 'extend/extender.dart';
 import 'module.dart';
 import 'module/forwarded_view.dart';
+import 'module/shadowed_view.dart';
 import 'util/merged_map_view.dart';
 import 'util/public_member_map_view.dart';
 import 'utils.dart';
@@ -42,6 +43,13 @@ class AsyncEnvironment {
   /// This is `null` if there are no forwarded modules.
   List<Module> _forwardedModules;
 
+  /// Modules forwarded by nested imports at each lexical scope level *beneath
+  /// the global scope*.
+  ///
+  /// This is `null` until it's needed, since most environments won't ever use
+  /// this.
+  List<List<Module>> _nestedForwardedModules;
+
   /// Modules from [_modules], [_globalModules], and [_forwardedModules], in the
   /// order in which they were `@use`d.
   final List<Module> _allModules;
@@ -128,6 +136,7 @@ class AsyncEnvironment {
       : _modules = {},
         _globalModules = null,
         _forwardedModules = null,
+        _nestedForwardedModules = null,
         _allModules = [],
         _variables = [{}],
         _variableNodes = sourceMap ? [{}] : null,
@@ -141,6 +150,7 @@ class AsyncEnvironment {
       this._modules,
       this._globalModules,
       this._forwardedModules,
+      this._nestedForwardedModules,
       this._allModules,
       this._variables,
       this._variableNodes,
@@ -164,6 +174,7 @@ class AsyncEnvironment {
       _modules,
       _globalModules,
       _forwardedModules,
+      _nestedForwardedModules,
       _allModules,
       _variables.toList(),
       _variableNodes?.toList(),
@@ -179,6 +190,7 @@ class AsyncEnvironment {
       {},
       null,
       null,
+      null,
       [],
       _variables.toList(),
       _variableNodes?.toList(),
@@ -270,30 +282,63 @@ class AsyncEnvironment {
   /// This is called when [module] is `@import`ed.
   void importForwards(Module module) {
     if (module is _EnvironmentModule) {
-      for (var forwarded
+      var forwarded = module._environment._forwardedModules;
-          in module._environment._forwardedModules ?? const <Module>[]) {
+      if (forwarded == null) return;
-        _globalModules ??= {};
-        _globalModules.add(forwarded);
 
-        // Remove existing definitions that the forwarded members are now
+      _globalModules ??= {};
-        // shadowing.
+      _forwardedModules ??= [];
-        for (var variable in forwarded.variables.keys) {
+
-          var index =
+      var forwardedVariableNames =
-              _variableIndices.remove(variable) ?? _variableIndex(variable);
+          forwarded.expand((module) => module.variables.keys).toSet();
-          if (index != null) {
+      var forwardedFunctionNames =
-            _variables[index].remove(variable);
+          forwarded.expand((module) => module.functions.keys).toSet();
-            if (_variableNodes != null) _variableNodes[index].remove(variable);
+      var forwardedMixinNames =
+          forwarded.expand((module) => module.mixins.keys).toSet();
+
+      if (atRoot) {
+        // Hide members from modules that have already been imported or
+        // forwarded that would otherwise conflict with the @imported members.
+        for (var module in _globalModules.toList()) {
+          var shadowed = ShadowedModuleView.ifNecessary(module,
+              variables: forwardedVariableNames,
+              mixins: forwardedMixinNames,
+              functions: forwardedFunctionNames);
+          if (shadowed != null) {
+            _globalModules.remove(module);
+            _globalModules.add(shadowed);
           }
         }
-        for (var function in forwarded.functions.keys) {
+        for (var i = 0; i < _forwardedModules.length; i++) {
-          var index =
+          var module = _forwardedModules[i];
-              _functionIndices.remove(function) ?? _functionIndex(function);
+          var shadowed = ShadowedModuleView.ifNecessary(module,
-          if (index != null) _functions[index].remove(function);
+              variables: forwardedVariableNames,
-        }
+              mixins: forwardedMixinNames,
-        for (var mixin in forwarded.mixins.keys) {
+              functions: forwardedFunctionNames);
-          var index = _mixinIndices.remove(mixin) ?? _mixinIndex(mixin);
+          if (shadowed != null) _forwardedModules[i] = shadowed;
-          if (index != null) _mixins[index].remove(mixin);
         }
+
+        _globalModules.addAll(forwarded);
+        _forwardedModules.addAll(forwarded);
+      } else {
+        _nestedForwardedModules ??=
+            List.generate(_variables.length - 1, (_) => []);
+        _nestedForwardedModules.last.addAll(forwarded);
+      }
+
+      // Remove existing member definitions that are now shadowed by the
+      // forwarded modules.
+      for (var variable in forwardedVariableNames) {
+        _variableIndices.remove(variable);
+        _variables.last.remove(variable);
+        if (_variableNodes != null) _variableNodes.last.remove(variable);
+      }
+      for (var function in forwardedFunctionNames) {
+        _functionIndices.remove(function);
+        _functions.last.remove(function);
+      }
+      for (var mixin in forwardedMixinNames) {
+        _mixinIndices.remove(mixin);
+        _mixins.last.remove(mixin);
       }
     }
   }
@@ -467,6 +512,19 @@ class AsyncEnvironment {
       return;
     }
 
+    if (_nestedForwardedModules != null &&
+        !_variableIndices.containsKey(name) &&
+        _variableIndex(name) == null) {
+      for (var modules in _nestedForwardedModules.reversed) {
+        for (var module in modules.reversed) {
+          if (module.variables.containsKey(name)) {
+            module.setVariable(name, value, nodeWithSpan);
+            return;
+          }
+        }
+      }
+    }
+
     var index = _lastVariableName == name
         ? _lastVariableIndex
         : _variableIndices.putIfAbsent(
@@ -652,6 +710,7 @@ class AsyncEnvironment {
     _variableNodes?.add({});
     _functions.add({});
     _mixins.add({});
+    _nestedForwardedModules?.add([]);
     try {
       return await callback();
     } finally {
@@ -667,14 +726,18 @@ class AsyncEnvironment {
       for (var name in _mixins.removeLast().keys) {
         _mixinIndices.remove(name);
       }
+      _nestedForwardedModules?.removeLast();
     }
   }
 
   /// Returns a module that represents the top-level members defined in [this],
   /// that contains [css] as its CSS tree, which can be extended using
   /// [extender].
-  Module toModule(CssStylesheet css, Extender extender) =>
+  Module toModule(CssStylesheet css, Extender extender) {
-      _EnvironmentModule(this, css, extender, forwarded: _forwardedModules);
+    assert(atRoot);
+    return _EnvironmentModule(this, css, extender,
+        forwarded: _forwardedModules);
+  }
 
   /// Returns the module with the given [namespace], or throws a
   /// [SassScriptException] if none exists.
@@ -687,7 +750,8 @@ class AsyncEnvironment {
   }
 
   /// Returns the result of [callback] if it returns non-`null` for exactly one
-  /// module in [_globalModules].
+  /// module in [_globalModules] *or* for any module in
+  /// [_nestedForwardedModules].
   ///
   /// Returns `null` if [callback] returns `null` for all modules. Throws an
   /// error if [callback] returns non-`null` for more than one module.
@@ -695,6 +759,15 @@ class AsyncEnvironment {
   /// The [type] should be the singular name of the value type being returned.
   /// It's used to format an appropriate error message.
   T _fromOneModule<T>(String type, T callback(Module module)) {
+    if (_nestedForwardedModules != null) {
+      for (var modules in _nestedForwardedModules.reversed) {
+        for (var module in modules.reversed) {
+          var value = callback(module);
+          if (value != null) return value;
+        }
+      }
+    }
+
     if (_globalModules == null) return null;
 
     T value;

lib/src/environment.dart

@@ -5,7 +5,7 @@
 // DO NOT EDIT. This file was generated from async_environment.dart.
 // See tool/grind/synchronize.dart for details.
 //
-// Checksum: 2522d5fbfb9d301b361a9bacac97228de2c6fd68
+// Checksum: 0459cbe5c439f3d45f24b739ed1f36b517d338b8
 //
 // ignore_for_file: unused_import
 
@@ -23,6 +23,7 @@ import 'exception.dart';
 import 'extend/extender.dart';
 import 'module.dart';
 import 'module/forwarded_view.dart';
+import 'module/shadowed_view.dart';
 import 'util/merged_map_view.dart';
 import 'util/public_member_map_view.dart';
 import 'utils.dart';
@@ -48,6 +49,13 @@ class Environment {
   /// This is `null` if there are no forwarded modules.
   List<Module<Callable>> _forwardedModules;
 
+  /// Modules forwarded by nested imports at each lexical scope level *beneath
+  /// the global scope*.
+  ///
+  /// This is `null` until it's needed, since most environments won't ever use
+  /// this.
+  List<List<Module<Callable>>> _nestedForwardedModules;
+
   /// Modules from [_modules], [_globalModules], and [_forwardedModules], in the
   /// order in which they were `@use`d.
   final List<Module<Callable>> _allModules;
@@ -134,6 +142,7 @@ class Environment {
       : _modules = {},
         _globalModules = null,
         _forwardedModules = null,
+        _nestedForwardedModules = null,
         _allModules = [],
         _variables = [{}],
         _variableNodes = sourceMap ? [{}] : null,
@@ -147,6 +156,7 @@ class Environment {
       this._modules,
       this._globalModules,
       this._forwardedModules,
+      this._nestedForwardedModules,
       this._allModules,
       this._variables,
       this._variableNodes,
@@ -170,6 +180,7 @@ class Environment {
       _modules,
       _globalModules,
       _forwardedModules,
+      _nestedForwardedModules,
       _allModules,
       _variables.toList(),
       _variableNodes?.toList(),
@@ -185,6 +196,7 @@ class Environment {
       {},
       null,
       null,
+      null,
       [],
       _variables.toList(),
       _variableNodes?.toList(),
@@ -276,30 +288,63 @@ class Environment {
   /// This is called when [module] is `@import`ed.
   void importForwards(Module<Callable> module) {
     if (module is _EnvironmentModule) {
-      for (var forwarded in module._environment._forwardedModules ??
+      var forwarded = module._environment._forwardedModules;
-          const <Module<Callable>>[]) {
+      if (forwarded == null) return;
-        _globalModules ??= {};
-        _globalModules.add(forwarded);
 
-        // Remove existing definitions that the forwarded members are now
+      _globalModules ??= {};
-        // shadowing.
+      _forwardedModules ??= [];
-        for (var variable in forwarded.variables.keys) {
+
-          var index =
+      var forwardedVariableNames =
-              _variableIndices.remove(variable) ?? _variableIndex(variable);
+          forwarded.expand((module) => module.variables.keys).toSet();
-          if (index != null) {
+      var forwardedFunctionNames =
-            _variables[index].remove(variable);
+          forwarded.expand((module) => module.functions.keys).toSet();
-            if (_variableNodes != null) _variableNodes[index].remove(variable);
+      var forwardedMixinNames =
+          forwarded.expand((module) => module.mixins.keys).toSet();
+
+      if (atRoot) {
+        // Hide members from modules that have already been imported or
+        // forwarded that would otherwise conflict with the @imported members.
+        for (var module in _globalModules.toList()) {
+          var shadowed = ShadowedModuleView.ifNecessary(module,
+              variables: forwardedVariableNames,
+              mixins: forwardedMixinNames,
+              functions: forwardedFunctionNames);
+          if (shadowed != null) {
+            _globalModules.remove(module);
+            _globalModules.add(shadowed);
           }
         }
-        for (var function in forwarded.functions.keys) {
+        for (var i = 0; i < _forwardedModules.length; i++) {
-          var index =
+          var module = _forwardedModules[i];
-              _functionIndices.remove(function) ?? _functionIndex(function);
+          var shadowed = ShadowedModuleView.ifNecessary(module,
-          if (index != null) _functions[index].remove(function);
+              variables: forwardedVariableNames,
-        }
+              mixins: forwardedMixinNames,
-        for (var mixin in forwarded.mixins.keys) {
+              functions: forwardedFunctionNames);
-          var index = _mixinIndices.remove(mixin) ?? _mixinIndex(mixin);
+          if (shadowed != null) _forwardedModules[i] = shadowed;
-          if (index != null) _mixins[index].remove(mixin);
         }
+
+        _globalModules.addAll(forwarded);
+        _forwardedModules.addAll(forwarded);
+      } else {
+        _nestedForwardedModules ??=
+            List.generate(_variables.length - 1, (_) => []);
+        _nestedForwardedModules.last.addAll(forwarded);
+      }
+
+      // Remove existing member definitions that are now shadowed by the
+      // forwarded modules.
+      for (var variable in forwardedVariableNames) {
+        _variableIndices.remove(variable);
+        _variables.last.remove(variable);
+        if (_variableNodes != null) _variableNodes.last.remove(variable);
+      }
+      for (var function in forwardedFunctionNames) {
+        _functionIndices.remove(function);
+        _functions.last.remove(function);
+      }
+      for (var mixin in forwardedMixinNames) {
+        _mixinIndices.remove(mixin);
+        _mixins.last.remove(mixin);
       }
     }
   }
@@ -473,6 +518,19 @@ class Environment {
       return;
     }
 
+    if (_nestedForwardedModules != null &&
+        !_variableIndices.containsKey(name) &&
+        _variableIndex(name) == null) {
+      for (var modules in _nestedForwardedModules.reversed) {
+        for (var module in modules.reversed) {
+          if (module.variables.containsKey(name)) {
+            module.setVariable(name, value, nodeWithSpan);
+            return;
+          }
+        }
+      }
+    }
+
     var index = _lastVariableName == name
         ? _lastVariableIndex
         : _variableIndices.putIfAbsent(
@@ -656,6 +714,7 @@ class Environment {
     _variableNodes?.add({});
     _functions.add({});
     _mixins.add({});
+    _nestedForwardedModules?.add([]);
     try {
       return callback();
     } finally {
@@ -671,14 +730,18 @@ class Environment {
       for (var name in _mixins.removeLast().keys) {
         _mixinIndices.remove(name);
       }
+      _nestedForwardedModules?.removeLast();
     }
   }
 
   /// Returns a module that represents the top-level members defined in [this],
   /// that contains [css] as its CSS tree, which can be extended using
   /// [extender].
-  Module<Callable> toModule(CssStylesheet css, Extender extender) =>
+  Module<Callable> toModule(CssStylesheet css, Extender extender) {
-      _EnvironmentModule(this, css, extender, forwarded: _forwardedModules);
+    assert(atRoot);
+    return _EnvironmentModule(this, css, extender,
+        forwarded: _forwardedModules);
+  }
 
   /// Returns the module with the given [namespace], or throws a
   /// [SassScriptException] if none exists.
@@ -691,7 +754,8 @@ class Environment {
   }
 
   /// Returns the result of [callback] if it returns non-`null` for exactly one
-  /// module in [_globalModules].
+  /// module in [_globalModules] *or* for any module in
+  /// [_nestedForwardedModules].
   ///
   /// Returns `null` if [callback] returns `null` for all modules. Throws an
   /// error if [callback] returns non-`null` for more than one module.
@@ -699,6 +763,15 @@ class Environment {
   /// The [type] should be the singular name of the value type being returned.
   /// It's used to format an appropriate error message.
   T _fromOneModule<T>(String type, T callback(Module<Callable> module)) {
+    if (_nestedForwardedModules != null) {
+      for (var modules in _nestedForwardedModules.reversed) {
+        for (var module in modules.reversed) {
+          var value = callback(module);
+          if (value != null) return value;
+        }
+      }
+    }
+
     if (_globalModules == null) return null;
 
     T value;

lib/src/module/shadowed_view.dart

@@ -0,0 +1,79 @@
+// Copyright 2019 Google Inc. 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.
+
+import '../ast/css.dart';
+import '../ast/node.dart';
+import '../callable.dart';
+import '../exception.dart';
+import '../extend/extender.dart';
+import '../module.dart';
+import '../util/limited_map_view.dart';
+import '../value.dart';
+
+/// A [Module] that only exposes members that aren't shadowed by a given
+/// blacklist of member names.
+class ShadowedModuleView<T extends AsyncCallable> implements Module<T> {
+  /// The wrapped module.
+  final Module<T> _inner;
+
+  Uri get url => _inner.url;
+  List<Module<T>> get upstream => _inner.upstream;
+  Extender get extender => _inner.extender;
+  CssStylesheet get css => _inner.css;
+  bool get transitivelyContainsCss => _inner.transitivelyContainsCss;
+  bool get transitivelyContainsExtensions =>
+      _inner.transitivelyContainsExtensions;
+
+  final Map<String, Value> variables;
+  final Map<String, AstNode> variableNodes;
+  final Map<String, T> functions;
+  final Map<String, T> mixins;
+
+  /// Like [ShadowedModuleView], but returns `null` if [inner] would be unchanged.
+  static ShadowedModuleView<T> ifNecessary<T extends AsyncCallable>(
+          Module<T> inner,
+          {Set<String> variables,
+          Set<String> functions,
+          Set<String> mixins}) =>
+      _needsBlacklist(inner.variables, variables) ||
+              _needsBlacklist(inner.functions, functions) ||
+              _needsBlacklist(inner.mixins, mixins)
+          ? ShadowedModuleView(inner,
+              variables: variables, functions: functions, mixins: mixins)
+          : null;
+
+  /// Returns a view of [inner] that doesn't include the given [variables],
+  /// [functions], or [mixins].
+  ShadowedModuleView(this._inner,
+      {Set<String> variables, Set<String> functions, Set<String> mixins})
+      : variables = _shadowedMap(_inner.variables, variables),
+        variableNodes = _shadowedMap(_inner.variableNodes, variables),
+        functions = _shadowedMap(_inner.functions, functions),
+        mixins = _shadowedMap(_inner.mixins, mixins);
+
+  ShadowedModuleView._(this._inner, this.variables, this.variableNodes,
+      this.functions, this.mixins);
+
+  /// Returns a view of [map] with all keys in [blacklist] omitted.
+  static Map<String, V> _shadowedMap<V>(
+      Map<String, V> map, Set<String> blacklist) {
+    if (map == null || !_needsBlacklist(map, blacklist)) return map;
+    return LimitedMapView.blacklist(map, blacklist);
+  }
+
+  /// Returns whether any of [map]'s keys are in [blacklist].
+  static bool _needsBlacklist(Map<String, Object> map, Set<String> blacklist) =>
+      blacklist != null && map.isNotEmpty && blacklist.any(map.containsKey);
+
+  void setVariable(String name, Value value, AstNode nodeWithSpan) {
+    if (!variables.containsKey(name)) {
+      throw SassScriptException("Undefined variable.");
+    } else {
+      return _inner.setVariable(name, value, nodeWithSpan);
+    }
+  }
+
+  Module<T> cloneCss() => ShadowedModuleView._(
+      _inner.cloneCss(), variables, variableNodes, functions, mixins);
+}

lib/src/visitor/async_evaluate.dart

@@ -1283,16 +1283,13 @@ class _EvaluateVisitor
       // need to put its CSS into an intermediate [ModifiableCssStylesheet] so
       // that we can hermetically resolve `@extend`s before injecting it.
       if (stylesheet.uses.isEmpty && stylesheet.forwards.isEmpty) {
-        var environment = _environment.global();
+        var oldImporter = _importer;
-        await _withEnvironment(environment, () async {
+        var oldStylesheet = _stylesheet;
-          var oldImporter = _importer;
+        _importer = importer;
-          var oldStylesheet = _stylesheet;
+        _stylesheet = stylesheet;
-          _importer = importer;
+        await visitStylesheet(stylesheet);
-          _stylesheet = stylesheet;
+        _importer = oldImporter;
-          await visitStylesheet(stylesheet);
+        _stylesheet = oldStylesheet;
-          _importer = oldImporter;
-          _stylesheet = oldStylesheet;
-        });
         _activeModules.remove(url);
         return;
       }

lib/src/visitor/evaluate.dart

@@ -5,7 +5,7 @@
 // DO NOT EDIT. This file was generated from async_evaluate.dart.
 // See tool/grind/synchronize.dart for details.
 //
-// Checksum: 9617d2d08b71858db9228b0858f2f27f87a35bf7
+// Checksum: 8b0be6a2009429b4a3a915f5ad5850e7891dd94d
 //
 // ignore_for_file: unused_import
 
@@ -1284,16 +1284,13 @@ class _EvaluateVisitor
       // need to put its CSS into an intermediate [ModifiableCssStylesheet] so
       // that we can hermetically resolve `@extend`s before injecting it.
       if (stylesheet.uses.isEmpty && stylesheet.forwards.isEmpty) {
-        var environment = _environment.global();
+        var oldImporter = _importer;
-        _withEnvironment(environment, () {
+        var oldStylesheet = _stylesheet;
-          var oldImporter = _importer;
+        _importer = importer;
-          var oldStylesheet = _stylesheet;
+        _stylesheet = stylesheet;
-          _importer = importer;
+        visitStylesheet(stylesheet);
-          _stylesheet = stylesheet;
+        _importer = oldImporter;
-          visitStylesheet(stylesheet);
+        _stylesheet = oldStylesheet;
-          _importer = oldImporter;
-          _stylesheet = oldStylesheet;
-        });
         _activeModules.remove(url);
         return;
       }