Don't throw conflict errors for identical members (#947)

See sass/sass#2820
Closes #946

CHANGELOG.md

@@ -1,4 +1,7 @@
-## 1.25.2
+## 1.26.0
+
+* Don't throw errors if the exact same member is loaded or forwarded from
+  multiple modules at the same time.
 
 * Fix a bug where, under extremely rare circumstances, a valid variable could
   become unassigned.

lib/src/async_environment.dart

@@ -276,10 +276,11 @@ class AsyncEnvironment {
     var view = ForwardedModuleView(module, rule);
     for (var other in _forwardedModules) {
       _assertNoConflicts(
-          view.variables, other.variables, "variable", other, rule);
+          view.variables, other.variables, module, other, "variable", rule);
       _assertNoConflicts(
-          view.functions, other.functions, "function", other, rule);
-      _assertNoConflicts(view.mixins, other.mixins, "mixin", other, rule);
+          view.functions, other.functions, module, other, "function", rule);
+      _assertNoConflicts(
+          view.mixins, other.mixins, module, other, "mixin", rule);
     }
 
     // Add the original module to [_allModules] (rather than the
@@ -291,15 +292,16 @@ class AsyncEnvironment {
     _forwardedModuleNodes[view] = rule;
   }
 
-  /// Throws a [SassScriptException] if [newMembers] has any keys that overlap
-  /// with [oldMembers].
+  /// Throws a [SassScriptException] if [newMembers] from [newModule] has any
+  /// keys that overlap with [oldMembers] from [oldModule].
   ///
-  /// The [type], [other], [newModuleNodeWithSpan] are used for error reporting.
+  /// The [type] and [newModuleNodeWithSpan] are used for error reporting.
   void _assertNoConflicts(
       Map<String, Object> newMembers,
       Map<String, Object> oldMembers,
+      Module newModule,
+      Module oldModule,
       String type,
-      Module other,
       AstNode newModuleNodeWithSpan) {
     Map<String, Object> smaller;
     Map<String, Object> larger;
@@ -312,13 +314,17 @@ class AsyncEnvironment {
     }
 
     for (var name in smaller.keys) {
-      if (larger.containsKey(name)) {
-        if (type == "variable") name = "\$$name";
-        throw MultiSpanSassScriptException(
-            'Two forwarded modules both define a $type named $name.',
-            "new @forward",
-            {_forwardedModuleNodes[other].span: "original @forward"});
+      if (!larger.containsKey(name)) continue;
+      if (newModule.variableIdentity(name) ==
+          oldModule.variableIdentity(name)) {
+        continue;
       }
+
+      if (type == "variable") name = "\$$name";
+      throw MultiSpanSassScriptException(
+          'Two forwarded modules both define a $type named $name.',
+          "new @forward",
+          {_forwardedModuleNodes[oldModule].span: "original @forward"});
     }
   }
 
@@ -436,7 +442,7 @@ class AsyncEnvironment {
   /// module, or `null` if no such variable is declared in any namespaceless
   /// module.
   Value _getVariableFromGlobalModule(String name) =>
-      _fromOneModule("variable", (module) => module.variables[name]);
+      _fromOneModule(name, "variable", (module) => module.variables[name]);
 
   /// Returns the node for the variable named [name], or `null` if no such
   /// variable is declared.
@@ -553,7 +559,7 @@ class AsyncEnvironment {
       // If this module doesn't already contain a variable named [name], try
       // setting it in a global module.
       if (!_variables.first.containsKey(name) && _globalModules != null) {
-        var moduleWithName = _fromOneModule("variable",
+        var moduleWithName = _fromOneModule(name, "variable",
             (module) => module.variables.containsKey(name) ? module : null);
         if (moduleWithName != null) {
           moduleWithName.setVariable(name, value, nodeWithSpan);
@@ -636,7 +642,7 @@ class AsyncEnvironment {
   /// module, or `null` if no such function is declared in any namespaceless
   /// module.
   AsyncCallable _getFunctionFromGlobalModule(String name) =>
-      _fromOneModule("function", (module) => module.functions[name]);
+      _fromOneModule(name, "function", (module) => module.functions[name]);
 
   /// Returns the index of the last map in [_functions] that has a [name] key,
   /// or `null` if none exists.
@@ -685,7 +691,7 @@ class AsyncEnvironment {
   /// module, or `null` if no such mixin is declared in any namespaceless
   /// module.
   AsyncCallable _getMixinFromGlobalModule(String name) =>
-      _fromOneModule("mixin", (module) => module.mixins[name]);
+      _fromOneModule(name, "mixin", (module) => module.mixins[name]);
 
   /// Returns the index of the last map in [_mixins] that has a [name] key, or
   /// `null` if none exists.
@@ -841,9 +847,11 @@ class AsyncEnvironment {
   /// Returns `null` if [callback] returns `null` for all modules. Throws an
   /// error if [callback] returns non-`null` for more than one module.
   ///
+  /// The [name] is the name of the member being looked up.
+  ///
   /// 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)) {
+  T _fromOneModule<T>(String name, String type, T callback(Module module)) {
     if (_nestedForwardedModules != null) {
       for (var modules in _nestedForwardedModules.reversed) {
         for (var module in modules.reversed) {
@@ -856,10 +864,16 @@ class AsyncEnvironment {
     if (_globalModules == null) return null;
 
     T value;
+    Object identity;
     for (var module in _globalModules) {
       var valueInModule = callback(module);
       if (valueInModule == null) continue;
 
+      var identityFromModule = valueInModule is AsyncCallable
+          ? valueInModule
+          : module.variableIdentity(name);
+      if (identityFromModule == identity) continue;
+
       if (value != null) {
         throw MultiSpanSassScriptException(
             'This $type is available from multiple global modules.',
@@ -870,6 +884,7 @@ class AsyncEnvironment {
       }
 
       value = valueInModule;
+      identity = identityFromModule;
     }
     return value;
   }
@@ -994,6 +1009,12 @@ class _EnvironmentModule implements Module {
     return;
   }
 
+  Object variableIdentity(String name) {
+    assert(variables.containsKey(name));
+    var module = _modulesByVariable[name];
+    return module == null ? this : module.variableIdentity(name);
+  }
+
   Module cloneCss() {
     if (css.children.isEmpty) return this;
 

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: b5212ffc7c50a8e7e436b25c7c16eb2996da2def
+// Checksum: 6666522945667f7f041530ee545444b7b40cfc80
 //
 // ignore_for_file: unused_import
 
@@ -283,10 +283,11 @@ class Environment {
     var view = ForwardedModuleView(module, rule);
     for (var other in _forwardedModules) {
       _assertNoConflicts(
-          view.variables, other.variables, "variable", other, rule);
+          view.variables, other.variables, module, other, "variable", rule);
       _assertNoConflicts(
-          view.functions, other.functions, "function", other, rule);
-      _assertNoConflicts(view.mixins, other.mixins, "mixin", other, rule);
+          view.functions, other.functions, module, other, "function", rule);
+      _assertNoConflicts(
+          view.mixins, other.mixins, module, other, "mixin", rule);
     }
 
     // Add the original module to [_allModules] (rather than the
@@ -298,15 +299,16 @@ class Environment {
     _forwardedModuleNodes[view] = rule;
   }
 
-  /// Throws a [SassScriptException] if [newMembers] has any keys that overlap
-  /// with [oldMembers].
+  /// Throws a [SassScriptException] if [newMembers] from [newModule] has any
+  /// keys that overlap with [oldMembers] from [oldModule].
   ///
-  /// The [type], [other], [newModuleNodeWithSpan] are used for error reporting.
+  /// The [type] and [newModuleNodeWithSpan] are used for error reporting.
   void _assertNoConflicts(
       Map<String, Object> newMembers,
       Map<String, Object> oldMembers,
+      Module<Callable> newModule,
+      Module<Callable> oldModule,
       String type,
-      Module<Callable> other,
       AstNode newModuleNodeWithSpan) {
     Map<String, Object> smaller;
     Map<String, Object> larger;
@@ -319,13 +321,17 @@ class Environment {
     }
 
     for (var name in smaller.keys) {
-      if (larger.containsKey(name)) {
-        if (type == "variable") name = "\$$name";
-        throw MultiSpanSassScriptException(
-            'Two forwarded modules both define a $type named $name.',
-            "new @forward",
-            {_forwardedModuleNodes[other].span: "original @forward"});
+      if (!larger.containsKey(name)) continue;
+      if (newModule.variableIdentity(name) ==
+          oldModule.variableIdentity(name)) {
+        continue;
       }
+
+      if (type == "variable") name = "\$$name";
+      throw MultiSpanSassScriptException(
+          'Two forwarded modules both define a $type named $name.',
+          "new @forward",
+          {_forwardedModuleNodes[oldModule].span: "original @forward"});
     }
   }
 
@@ -443,7 +449,7 @@ class Environment {
   /// module, or `null` if no such variable is declared in any namespaceless
   /// module.
   Value _getVariableFromGlobalModule(String name) =>
-      _fromOneModule("variable", (module) => module.variables[name]);
+      _fromOneModule(name, "variable", (module) => module.variables[name]);
 
   /// Returns the node for the variable named [name], or `null` if no such
   /// variable is declared.
@@ -560,7 +566,7 @@ class Environment {
       // If this module doesn't already contain a variable named [name], try
       // setting it in a global module.
       if (!_variables.first.containsKey(name) && _globalModules != null) {
-        var moduleWithName = _fromOneModule("variable",
+        var moduleWithName = _fromOneModule(name, "variable",
             (module) => module.variables.containsKey(name) ? module : null);
         if (moduleWithName != null) {
           moduleWithName.setVariable(name, value, nodeWithSpan);
@@ -643,7 +649,7 @@ class Environment {
   /// module, or `null` if no such function is declared in any namespaceless
   /// module.
   Callable _getFunctionFromGlobalModule(String name) =>
-      _fromOneModule("function", (module) => module.functions[name]);
+      _fromOneModule(name, "function", (module) => module.functions[name]);
 
   /// Returns the index of the last map in [_functions] that has a [name] key,
   /// or `null` if none exists.
@@ -692,7 +698,7 @@ class Environment {
   /// module, or `null` if no such mixin is declared in any namespaceless
   /// module.
   Callable _getMixinFromGlobalModule(String name) =>
-      _fromOneModule("mixin", (module) => module.mixins[name]);
+      _fromOneModule(name, "mixin", (module) => module.mixins[name]);
 
   /// Returns the index of the last map in [_mixins] that has a [name] key, or
   /// `null` if none exists.
@@ -846,9 +852,12 @@ class Environment {
   /// Returns `null` if [callback] returns `null` for all modules. Throws an
   /// error if [callback] returns non-`null` for more than one module.
   ///
+  /// The [name] is the name of the member being looked up.
+  ///
   /// 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)) {
+  T _fromOneModule<T>(
+      String name, String type, T callback(Module<Callable> module)) {
     if (_nestedForwardedModules != null) {
       for (var modules in _nestedForwardedModules.reversed) {
         for (var module in modules.reversed) {
@@ -861,10 +870,16 @@ class Environment {
     if (_globalModules == null) return null;
 
     T value;
+    Object identity;
     for (var module in _globalModules) {
       var valueInModule = callback(module);
       if (valueInModule == null) continue;
 
+      var identityFromModule = valueInModule is Callable
+          ? valueInModule
+          : module.variableIdentity(name);
+      if (identityFromModule == identity) continue;
+
       if (value != null) {
         throw MultiSpanSassScriptException(
             'This $type is available from multiple global modules.',
@@ -875,6 +890,7 @@ class Environment {
       }
 
       value = valueInModule;
+      identity = identityFromModule;
     }
     return value;
   }
@@ -1000,6 +1016,12 @@ class _EnvironmentModule implements Module<Callable> {
     return;
   }
 
+  Object variableIdentity(String name) {
+    assert(variables.containsKey(name));
+    var module = _modulesByVariable[name];
+    return module == null ? this : module.variableIdentity(name);
+  }
+
   Module<Callable> cloneCss() {
     if (css.children.isEmpty) return this;
 

lib/src/module.dart

@@ -73,6 +73,12 @@ abstract class Module<T extends AsyncCallable> {
   /// named [name].
   void setVariable(String name, Value value, AstNode nodeWithSpan);
 
+  /// Returns an opaque object that will be equal to another
+  /// `variableIdentity()` return value for the same name in another module if
+  /// and only if both modules expose identical definitions of the variable in
+  /// question, as defined by the Sass spec.
+  Object variableIdentity(String name);
+
   /// Creates a copy of this module with new [css] and [extender].
   Module<T> cloneCss();
 }

lib/src/module/built_in.dart

@@ -49,5 +49,10 @@ class BuiltInModule<T extends AsyncCallable> implements Module<T> {
     throw SassScriptException("Cannot modify built-in variable.");
   }
 
+  Object variableIdentity(String name) {
+    assert(variables.containsKey(name));
+    return this;
+  }
+
   Module<T> cloneCss() => this;
 }

lib/src/module/forwarded_view.dart

@@ -91,5 +91,16 @@ class ForwardedModuleView<T extends AsyncCallable> implements Module<T> {
     return _inner.setVariable(name, value, nodeWithSpan);
   }
 
+  Object variableIdentity(String name) {
+    assert(variables.containsKey(name));
+
+    if (_rule.prefix != null) {
+      assert(name.startsWith(_rule.prefix));
+      name = name.substring(_rule.prefix.length);
+    }
+
+    return _inner.variableIdentity(name);
+  }
+
   Module<T> cloneCss() => ForwardedModuleView(_inner.cloneCss(), _rule);
 }

lib/src/module/shadowed_view.dart

@@ -74,6 +74,11 @@ class ShadowedModuleView<T extends AsyncCallable> implements Module<T> {
     }
   }
 
+  Object variableIdentity(String name) {
+    assert(variables.containsKey(name));
+    return _inner.variableIdentity(name);
+  }
+
   Module<T> cloneCss() => ShadowedModuleView._(
       _inner.cloneCss(), variables, variableNodes, functions, mixins);
 }

pubspec.yaml

@@ -1,5 +1,5 @@
 name: sass
-version: 1.25.2-dev
+version: 1.26.0-dev
 description: A Sass implementation in Dart.
 author: Sass Team
 homepage: https://github.com/sass/dart-sass