From 67c4e1bdea82856e53c2f482b9bdf38caadd045c Mon Sep 17 00:00:00 2001 From: Natalie Weizenbaum Date: Fri, 7 Feb 2020 15:17:05 -0800 Subject: [PATCH] Don't throw conflict errors for identical members (#947) See sass/sass#2820 Closes #946 --- CHANGELOG.md | 5 ++- lib/src/async_environment.dart | 57 +++++++++++++++++++--------- lib/src/environment.dart | 60 ++++++++++++++++++++---------- lib/src/module.dart | 6 +++ lib/src/module/built_in.dart | 5 +++ lib/src/module/forwarded_view.dart | 11 ++++++ lib/src/module/shadowed_view.dart | 5 +++ pubspec.yaml | 2 +- 8 files changed, 112 insertions(+), 39 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d7c560ec..5755aff6 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/lib/src/async_environment.dart b/lib/src/async_environment.dart index e19a14a5..c77182f1 100644 --- a/lib/src/async_environment.dart +++ b/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 newMembers, Map oldMembers, + Module newModule, + Module oldModule, String type, - Module other, AstNode newModuleNodeWithSpan) { Map smaller; Map 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(String type, T callback(Module module)) { + T _fromOneModule(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; diff --git a/lib/src/environment.dart b/lib/src/environment.dart index 7c8c9356..8f830bec 100644 --- a/lib/src/environment.dart +++ b/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 newMembers, Map oldMembers, + Module newModule, + Module oldModule, String type, - Module other, AstNode newModuleNodeWithSpan) { Map smaller; Map 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(String type, T callback(Module module)) { + T _fromOneModule( + String name, String type, T callback(Module 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 { 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; diff --git a/lib/src/module.dart b/lib/src/module.dart index 70f2d09e..189f4d73 100644 --- a/lib/src/module.dart +++ b/lib/src/module.dart @@ -73,6 +73,12 @@ abstract class Module { /// 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 cloneCss(); } diff --git a/lib/src/module/built_in.dart b/lib/src/module/built_in.dart index 8e4abf41..5e70aac2 100644 --- a/lib/src/module/built_in.dart +++ b/lib/src/module/built_in.dart @@ -49,5 +49,10 @@ class BuiltInModule implements Module { throw SassScriptException("Cannot modify built-in variable."); } + Object variableIdentity(String name) { + assert(variables.containsKey(name)); + return this; + } + Module cloneCss() => this; } diff --git a/lib/src/module/forwarded_view.dart b/lib/src/module/forwarded_view.dart index 606e4804..53a8ee8b 100644 --- a/lib/src/module/forwarded_view.dart +++ b/lib/src/module/forwarded_view.dart @@ -91,5 +91,16 @@ class ForwardedModuleView implements Module { 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 cloneCss() => ForwardedModuleView(_inner.cloneCss(), _rule); } diff --git a/lib/src/module/shadowed_view.dart b/lib/src/module/shadowed_view.dart index d85710bf..4a29c445 100644 --- a/lib/src/module/shadowed_view.dart +++ b/lib/src/module/shadowed_view.dart @@ -74,6 +74,11 @@ class ShadowedModuleView implements Module { } } + Object variableIdentity(String name) { + assert(variables.containsKey(name)); + return _inner.variableIdentity(name); + } + Module cloneCss() => ShadowedModuleView._( _inner.cloneCss(), variables, variableNodes, functions, mixins); } diff --git a/pubspec.yaml b/pubspec.yaml index c0fa6848..367bbc73 100644 --- a/pubspec.yaml +++ b/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