Allow unused configurations to go past @forward (#2600)

Closes #2598

Author: nex3

CHANGELOG.md

@@ -1,3 +1,9 @@
+## 1.90.0
+
+* Allow a `@forward`ed module to be loaded with a configuration when that module
+  has already been loaded with a different configuration *and* the module
+  doesn't define any variables that would have been configured anyway.
+
 ## 1.89.2
 
 ### Embedded Host

lib/src/async_environment.dart

@@ -121,6 +121,10 @@ final class AsyncEnvironment {
   UserDefinedCallable<AsyncEnvironment>? get content => _content;
   UserDefinedCallable<AsyncEnvironment>? _content;
 
+  /// The set of variable names that could be configured when loading the
+  /// module.
+  final Set<String> _configurableVariables;
+
   /// Whether the environment is lexically at the root of the document.
   bool get atRoot => _variables.length == 1;
 
@@ -160,27 +164,28 @@ final class AsyncEnvironment {
         _functions = [{}],
         _functionIndices = {},
         _mixins = [{}],
-        _mixinIndices = {};
+        _mixinIndices = {},
+        _configurableVariables = {};
 
   AsyncEnvironment._(
-    this._modules,
-    this._namespaceNodes,
-    this._globalModules,
-    this._importedModules,
-    this._forwardedModules,
-    this._nestedForwardedModules,
-    this._allModules,
-    this._variables,
-    this._variableNodes,
-    this._functions,
-    this._mixins,
-    this._content,
-  )   
-  // Lazily fill in the indices rather than eagerly copying them from the
-  // existing environment in closure() because the copying took a lot of
-  // time and was rarely helpful. This saves a bunch of time on Susy's
-  // tests.
-  : _variableIndices = {},
+      this._modules,
+      this._namespaceNodes,
+      this._globalModules,
+      this._importedModules,
+      this._forwardedModules,
+      this._nestedForwardedModules,
+      this._allModules,
+      this._variables,
+      this._variableNodes,
+      this._functions,
+      this._mixins,
+      this._content,
+      this._configurableVariables)
+      // Lazily fill in the indices rather than eagerly copying them from the
+      // existing environment in closure() because the copying took a lot of
+      // time and was rarely helpful. This saves a bunch of time on Susy's
+      // tests.
+      : _variableIndices = {},
         _functionIndices = {},
         _mixinIndices = {};
 
@@ -202,6 +207,9 @@ final class AsyncEnvironment {
         _functions.toList(),
         _mixins.toList(),
         _content,
+        // Closures are always in nested contexts where configurable variables
+        // are never added.
+        const {},
       );
 
   /// Returns a new environment to use for an imported file.
@@ -222,6 +230,7 @@ final class AsyncEnvironment {
         _functions.toList(),
         _mixins.toList(),
         _content,
+        _configurableVariables,
       );
 
   /// Adds [module] to the set of modules visible in this environment.
@@ -640,6 +649,15 @@ final class AsyncEnvironment {
     _variableNodes[index][name] = nodeWithSpan;
   }
 
+  /// Records that [name] is a variable that could have been configured for this
+  /// module, whether or not it actually was.
+  ///
+  /// This is used to determine whether to throw an error when passing a new
+  /// configuration through `@forward` to an already-loaded module.
+  void markVariableConfigurable(String name) {
+    _configurableVariables.add(name);
+  }
+
   /// Returns the value of the function named [name], optionally with the given
   /// [namespace], or `null` if no such variable is declared.
   ///
@@ -1070,6 +1088,26 @@ final class _EnvironmentModule implements Module {
     return module == null ? this : module.variableIdentity(name);
   }
 
+  bool couldHaveBeenConfigured(Set<String> variables) =>
+      // Check if this module defines a configurable variable with any of the
+      // given names.
+      (variables.length < _environment._configurableVariables.length
+          ? variables.any(_environment._configurableVariables.contains)
+          : _environment._configurableVariables.any(variables.contains)) ||
+      // Find forwarded modules whose variables overlap with [variables] and
+      // check if they define configurable variables with any of the given
+      // names.
+      (variables.length < _modulesByVariable.length
+              ? {
+                  for (var variable in variables)
+                    if (_modulesByVariable[variable] case var module?) module
+                }
+              : {
+                  for (var (variable, module) in _modulesByVariable.pairs)
+                    if (variables.contains(variable)) module
+                })
+          .any((module) => module.couldHaveBeenConfigured(variables));
+
   Module cloneCss() {
     if (!transitivelyContainsCss) return this;
 

lib/src/configuration.dart

@@ -50,7 +50,7 @@ final class Configuration {
   /// will be considered to have the same original config if they were created
   /// as a copy from the same base configuration.
   bool sameOriginal(Configuration that) =>
-      _originalConfiguration == that._originalConfiguration;
+      identical(_originalConfiguration, that._originalConfiguration);
 
   /// The empty configuration, which indicates that the module has not been
   /// configured.
@@ -70,7 +70,7 @@ final class Configuration {
 
   /// Creates a new configuration from this one based on a `@forward` rule.
   Configuration throughForward(ForwardRule forward) {
-    if (isEmpty) return const Configuration.empty();
+    if (isEmpty) return this;
     var newValues = _values;
 
     // Only allow variables that are visible through the `@forward` to be

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: add8a3972aec53ef29de3639af2bc84edcbde9e7
+// Checksum: 72b802e4004aae8a84f7aa78ec728861339b846b
 //
 // ignore_for_file: unused_import
 
@@ -128,6 +128,10 @@ final class Environment {
   UserDefinedCallable<Environment>? get content => _content;
   UserDefinedCallable<Environment>? _content;
 
+  /// The set of variable names that could be configured when loading the
+  /// module.
+  final Set<String> _configurableVariables;
+
   /// Whether the environment is lexically at the root of the document.
   bool get atRoot => _variables.length == 1;
 
@@ -167,27 +171,28 @@ final class Environment {
         _functions = [{}],
         _functionIndices = {},
         _mixins = [{}],
-        _mixinIndices = {};
+        _mixinIndices = {},
+        _configurableVariables = {};
 
   Environment._(
-    this._modules,
-    this._namespaceNodes,
-    this._globalModules,
-    this._importedModules,
-    this._forwardedModules,
-    this._nestedForwardedModules,
-    this._allModules,
-    this._variables,
-    this._variableNodes,
-    this._functions,
-    this._mixins,
-    this._content,
-  )   
-  // Lazily fill in the indices rather than eagerly copying them from the
-  // existing environment in closure() because the copying took a lot of
-  // time and was rarely helpful. This saves a bunch of time on Susy's
-  // tests.
-  : _variableIndices = {},
+      this._modules,
+      this._namespaceNodes,
+      this._globalModules,
+      this._importedModules,
+      this._forwardedModules,
+      this._nestedForwardedModules,
+      this._allModules,
+      this._variables,
+      this._variableNodes,
+      this._functions,
+      this._mixins,
+      this._content,
+      this._configurableVariables)
+      // Lazily fill in the indices rather than eagerly copying them from the
+      // existing environment in closure() because the copying took a lot of
+      // time and was rarely helpful. This saves a bunch of time on Susy's
+      // tests.
+      : _variableIndices = {},
         _functionIndices = {},
         _mixinIndices = {};
 
@@ -209,6 +214,9 @@ final class Environment {
         _functions.toList(),
         _mixins.toList(),
         _content,
+        // Closures are always in nested contexts where configurable variables
+        // are never added.
+        const {},
       );
 
   /// Returns a new environment to use for an imported file.
@@ -229,6 +237,7 @@ final class Environment {
         _functions.toList(),
         _mixins.toList(),
         _content,
+        _configurableVariables,
       );
 
   /// Adds [module] to the set of modules visible in this environment.
@@ -648,6 +657,15 @@ final class Environment {
     _variableNodes[index][name] = nodeWithSpan;
   }
 
+  /// Records that [name] is a variable that could have been configured for this
+  /// module, whether or not it actually was.
+  ///
+  /// This is used to determine whether to throw an error when passing a new
+  /// configuration through `@forward` to an already-loaded module.
+  void markVariableConfigurable(String name) {
+    _configurableVariables.add(name);
+  }
+
   /// Returns the value of the function named [name], optionally with the given
   /// [namespace], or `null` if no such variable is declared.
   ///
@@ -1080,6 +1098,26 @@ final class _EnvironmentModule implements Module<Callable> {
     return module == null ? this : module.variableIdentity(name);
   }
 
+  bool couldHaveBeenConfigured(Set<String> variables) =>
+      // Check if this module defines a configurable variable with any of the
+      // given names.
+      (variables.length < _environment._configurableVariables.length
+          ? variables.any(_environment._configurableVariables.contains)
+          : _environment._configurableVariables.any(variables.contains)) ||
+      // Find forwarded modules whose variables overlap with [variables] and
+      // check if they define configurable variables with any of the given
+      // names.
+      (variables.length < _modulesByVariable.length
+              ? {
+                  for (var variable in variables)
+                    if (_modulesByVariable[variable] case var module?) module
+                }
+              : {
+                  for (var (variable, module) in _modulesByVariable.pairs)
+                    if (variables.contains(variable)) module
+                })
+          .any((module) => module.couldHaveBeenConfigured(variables));
+
   Module<Callable> cloneCss() {
     if (!transitivelyContainsCss) return this;
 

lib/src/module.dart

@@ -30,8 +30,7 @@ abstract interface class Module<T extends AsyncCallable> {
   /// [AstNode.span] if the span isn't required, since some nodes need to do
   /// real work to manufacture a source span.
   ///
-  /// Implementations must ensure that this has the same keys as [variables] if
-  /// it's not `null`.
+  /// Implementations must ensure that this has the same keys as [variables].
   Map<String, AstNode> get variableNodes;
 
   /// The module's functions.
@@ -81,6 +80,10 @@ abstract interface class Module<T extends AsyncCallable> {
   /// question, as defined by the Sass spec.
   Object variableIdentity(String name);
 
+  /// Whether this module exposes any variables from among [variables] that
+  /// could have been configured when the module was loaded.
+  bool couldHaveBeenConfigured(Set<String> variables);
+
   /// Creates a copy of this module with new [css] and [extender].
   Module<T> cloneCss();
 }

lib/src/module/built_in.dart

@@ -62,5 +62,7 @@ final class BuiltInModule<T extends AsyncCallable> implements Module<T> {
     return this;
   }
 
+  bool couldHaveBeenConfigured(Set<String> _) => false;
+
   Module<T> cloneCss() => this;
 }

lib/src/module/forwarded_view.dart

@@ -140,6 +140,31 @@ class ForwardedModuleView<T extends AsyncCallable> implements Module<T> {
     return _inner.variableIdentity(name);
   }
 
+  bool couldHaveBeenConfigured(Set<String> variables) {
+    assert(_rule.shownVariables == null || _rule.hiddenVariables == null);
+    if (_rule.prefix == null &&
+        _rule.shownVariables == null &&
+        (_rule.hiddenVariables?.isEmpty ?? true)) {
+      return _inner.couldHaveBeenConfigured(variables);
+    }
+
+    if (_rule.prefix case var prefix?) {
+      variables = {
+        for (var name in variables)
+          if (name.startsWith(prefix)) name.substring(prefix.length)
+      };
+    }
+
+    if (_rule.shownVariables case var safelist?) {
+      return _inner.couldHaveBeenConfigured(variables.intersection(safelist));
+    } else if (_rule.hiddenVariables case var blocklist?
+        when blocklist.isNotEmpty) {
+      return _inner.couldHaveBeenConfigured(variables.difference(blocklist));
+    } else {
+      return _inner.couldHaveBeenConfigured(variables);
+    }
+  }
+
   bool operator ==(Object other) =>
       other is ForwardedModuleView &&
       _inner == other._inner &&

lib/src/module/shadowed_view.dart

@@ -107,6 +107,14 @@ final class ShadowedModuleView<T extends AsyncCallable> implements Module<T> {
     return _inner.variableIdentity(name);
   }
 
+  bool couldHaveBeenConfigured(Set<String> variables) =>
+      this.variables == _inner.variables
+          ? _inner.couldHaveBeenConfigured(variables)
+          : _inner.couldHaveBeenConfigured({
+              for (var name in this.variables.keys)
+                if (variables.contains(name)) name
+            });
+
   bool operator ==(Object other) =>
       other is ShadowedModuleView &&
       _inner == other._inner &&

lib/src/visitor/async_evaluate.dart

@@ -854,10 +854,18 @@ final class _EvaluateVisitor
   }) async {
     var url = stylesheet.span.sourceUrl;
 
+    var currentConfiguration = configuration ?? _configuration;
     if (_modules[url] case var alreadyLoaded?) {
-      var currentConfiguration = configuration ?? _configuration;
       if (!_moduleConfigurations[url]!.sameOriginal(currentConfiguration) &&
-          currentConfiguration is ExplicitConfiguration) {
+          currentConfiguration is ExplicitConfiguration &&
+          // Don't throw an error if the module being loaded doesn't expose any
+          // configurable variables that could have been affected by the
+          // configuration in the first place. If the configuration defines a
+          // value that's not in the module, it'll still throw an error, but
+          // this avoids throwing confusing errors for `@forward`ed modules
+          // without configuration.
+          alreadyLoaded.couldHaveBeenConfigured(
+              MapKeySet(currentConfiguration.values))) {
         var message = namesInErrors
             ? "${p.prettyUri(url)} was already loaded, so it can't be "
                 "configured using \"with\"."
@@ -946,7 +954,7 @@ final class _EvaluateVisitor
     );
     if (url != null) {
       _modules[url] = module;
-      _moduleConfigurations[url] = _configuration;
+      _moduleConfigurations[url] = currentConfiguration;
       if (nodeWithSpan != null) _moduleNodes[url] = nodeWithSpan;
     }
 
@@ -2599,6 +2607,7 @@ final class _EvaluateVisitor
   Future<Value?> visitVariableDeclaration(VariableDeclaration node) async {
     if (node.isGuarded) {
       if (node.namespace == null && _environment.atRoot) {
+        _environment.markVariableConfigurable(node.name);
         if (_configuration.remove(node.name) case var override?
             when override.value != sassNull) {
           _addExceptionSpan(node, () {