Allow a module loaded multiple times by the same configuration (#1739)

* Allow a module loaded multiple times by the same configuration

* use object references as opaque ID

Fixes #1716

CHANGELOG.md

@@ -10,6 +10,9 @@
    `random()` currently ignores units. A future version will no longer ignore
   units.
 
+* Don't throw an error when the same module is `@forward`ed multiple times
+  through a configured module.
+
 ## 1.54.4
 
 * Improve error messages when passing incorrect units that are also

lib/src/configuration.dart

@@ -3,7 +3,6 @@
 // https://opensource.org/licenses/MIT.
 
 import 'dart:collection';
-
 import 'ast/node.dart';
 import 'ast/sass.dart';
 import 'configured_value.dart';
@@ -27,14 +26,39 @@ class Configuration {
   final Map<String, ConfiguredValue> _values;
 
   /// Creates an implicit configuration with the given [values].
-  Configuration.implicit(this._values);
+  Configuration.implicit(this._values) : __originalConfiguration = null;
+
+  /// The backing value for [_originalConfiguration].
+  ///
+  /// This is null if [_originalConfiguration] refers to itself since `this`
+  /// can't be assigned to a final field.
+  final Configuration? __originalConfiguration;
+
+  /// The configuration from which this was modified with `@forward ... with`.
+  ///
+  /// This reference serves as an opaque ID.
+  Configuration get _originalConfiguration => __originalConfiguration ?? this;
+
+  /// Returns whether `this` and [that] [Configuration]s have the same
+  /// [_originalConfiguration].
+  ///
+  /// An implicit configuration will always return `false` because it was not
+  /// created through another configuration.
+  ///
+  /// [ExplicitConfiguration]s will and configurations created [throughForward]
+  /// 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;
 
   /// The empty configuration, which indicates that the module has not been
   /// configured.
   ///
   /// Empty configurations are always considered implicit, since they are
   /// ignored if the module has already been loaded.
-  const Configuration.empty() : _values = const {};
+  const Configuration.empty()
+      : _values = const {},
+        __originalConfiguration = null;
 
   bool get isEmpty => values.isEmpty;
 
@@ -65,9 +89,15 @@ class Configuration {
     return _withValues(newValues);
   }
 
-  /// Returns a copy of [this] with the given [values] map.
+  /// Returns a copy of `this` [Configuration] with the given [values] map.
+  ///
+  /// The copy will have the same [_originalConfiguration] as `this` config.
   Configuration _withValues(Map<String, ConfiguredValue> values) =>
-      Configuration.implicit(values);
+      Configuration._(values, _originalConfiguration);
+
+  /// Creates a [Configuration] with the given [_values] map and an
+  /// [_originalConfiguration] reference.
+  Configuration._(this._values, this.__originalConfiguration);
 
   String toString() =>
       "(" +
@@ -88,10 +118,20 @@ class ExplicitConfiguration extends Configuration {
   /// The node whose span indicates where the configuration was declared.
   final AstNode nodeWithSpan;
 
+  /// Creates a base [ExplicitConfiguration] with a [values] map and a
+  /// [nodeWithSpan].
   ExplicitConfiguration(Map<String, ConfiguredValue> values, this.nodeWithSpan)
       : super.implicit(values);
 
-  /// Returns a copy of [this] with the given [values] map.
+  /// Creates an [ExplicitConfiguration] with a [values] map, a [nodeWithSpan]
+  /// and if this is a copy a reference to the [_originalConfiguration].
+  ExplicitConfiguration._(Map<String, ConfiguredValue> values,
+      this.nodeWithSpan, Configuration? originalConfiguration)
+      : super._(values, originalConfiguration);
+
+  /// Returns a copy of `this` with the given [values] map.
+  ///
+  /// The copy will have the same [_originalConfiguration] as `this` config.
   Configuration _withValues(Map<String, ConfiguredValue> values) =>
-      ExplicitConfiguration(values, nodeWithSpan);
+      ExplicitConfiguration._(values, nodeWithSpan, _originalConfiguration);
 }

lib/src/visitor/async_evaluate.dart

@@ -141,6 +141,9 @@ class _EvaluateVisitor
   /// All modules that have been loaded and evaluated so far.
   final _modules = <Uri, Module>{};
 
+  /// The first [Configuration] used to load a module [Uri].
+  final _moduleConfigurations = <Uri, Configuration>{};
+
   /// A map from canonical module URLs to the nodes whose spans indicate where
   /// those modules were originally loaded.
   ///
@@ -670,7 +673,8 @@ class _EvaluateVisitor
     var alreadyLoaded = _modules[url];
     if (alreadyLoaded != null) {
       var currentConfiguration = configuration ?? _configuration;
-      if (currentConfiguration is ExplicitConfiguration) {
+      if (!_moduleConfigurations[url]!.sameOriginal(currentConfiguration) &&
+          currentConfiguration is ExplicitConfiguration) {
         var message = namesInErrors
             ? "${p.prettyUri(url)} was already loaded, so it can't be "
                 "configured using \"with\"."
@@ -751,6 +755,7 @@ class _EvaluateVisitor
     var module = environment.toModule(css, extensionStore);
     if (url != null) {
       _modules[url] = module;
+      _moduleConfigurations[url] = _configuration;
       if (nodeWithSpan != null) _moduleNodes[url] = nodeWithSpan;
     }
 

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: c3bc020b07dc2376f57ef8e9990ff6b97cde3417
+// Checksum: ad5691d0c80b924308f6ee03863fa8c7f5e25ca0
 //
 // ignore_for_file: unused_import
 
@@ -149,6 +149,9 @@ class _EvaluateVisitor
   /// All modules that have been loaded and evaluated so far.
   final _modules = <Uri, Module<Callable>>{};
 
+  /// The first [Configuration] used to load a module [Uri].
+  final _moduleConfigurations = <Uri, Configuration>{};
+
   /// A map from canonical module URLs to the nodes whose spans indicate where
   /// those modules were originally loaded.
   ///
@@ -675,7 +678,8 @@ class _EvaluateVisitor
     var alreadyLoaded = _modules[url];
     if (alreadyLoaded != null) {
       var currentConfiguration = configuration ?? _configuration;
-      if (currentConfiguration is ExplicitConfiguration) {
+      if (!_moduleConfigurations[url]!.sameOriginal(currentConfiguration) &&
+          currentConfiguration is ExplicitConfiguration) {
         var message = namesInErrors
             ? "${p.prettyUri(url)} was already loaded, so it can't be "
                 "configured using \"with\"."
@@ -756,6 +760,7 @@ class _EvaluateVisitor
     var module = environment.toModule(css, extensionStore);
     if (url != null) {
       _modules[url] = module;
+      _moduleConfigurations[url] = _configuration;
       if (nodeWithSpan != null) _moduleNodes[url] = nodeWithSpan;
     }