From ce5cb3a07c0b8851b8ada2cd62bca8bdeb0fa225 Mon Sep 17 00:00:00 2001 From: arikorn Date: Thu, 3 Jul 2025 02:57:09 -0700 Subject: [PATCH 1/3] Fix: Error checking of @use/@forward ... with... is too strict This PR resolves issue #2601 Sass does not allow a previously-loaded module to be subsequently loaded using a `with` configuration. When numerous interdependent modules are loaded in one `@use` call, as in CoreUI, this can lead to spurious errors from modules whose contents do not include the specified variables. This commit limits `Error: This module was already loaded, so it can't be configured using "with"` errors to modules that actually are overriden by the supplied variables. As a side-effect this also improves error-reporting when the cause of the error is due to "overriding" a non-existent variable. See examples in Issue #2598 --- lib/src/configuration.dart | 2 ++ lib/src/visitor/async_evaluate.dart | 20 +++++++++++++++++++- lib/src/visitor/evaluate.dart | 22 ++++++++++++++++++++-- 3 files changed, 41 insertions(+), 3 deletions(-) diff --git a/lib/src/configuration.dart b/lib/src/configuration.dart index fb4138481..a9b254426 100644 --- a/lib/src/configuration.dart +++ b/lib/src/configuration.dart @@ -63,6 +63,8 @@ final class Configuration { bool get isEmpty => values.isEmpty; + int get length => values.length; + /// Removes a variable with [name] from this configuration, returning it. /// /// If no such variable exists in this configuration, returns null. diff --git a/lib/src/visitor/async_evaluate.dart b/lib/src/visitor/async_evaluate.dart index c1db0e72e..1541805d2 100644 --- a/lib/src/visitor/async_evaluate.dart +++ b/lib/src/visitor/async_evaluate.dart @@ -156,6 +156,9 @@ final class _EvaluateVisitor /// The first [Configuration] used to load a module [Uri]. final _moduleConfigurations = {}; + // Track whether a module used one of the variables passed in a "with" clause + final _moduleUsedConfigVars = {}; + /// A map from canonical module URLs to the nodes whose spans indicate where /// those modules were originally loaded. /// @@ -854,9 +857,17 @@ final class _EvaluateVisitor }) async { var url = stylesheet.span.sourceUrl; + // set up variables to determine if the module accessed (& consumed) any configuration variables. + var startingConfigPropVars = + _configuration.length; // number of this._config vars upon entry. + var startingConfigVars = (configuration?.length ?? + 0); // number of config vars passed in as a function arg. + if (_modules[url] case var alreadyLoaded?) { var currentConfiguration = configuration ?? _configuration; - if (!_moduleConfigurations[url]!.sameOriginal(currentConfiguration) && + // note: if module didn't use any of the config vars, treat it as if it was called without them. + if (_moduleUsedConfigVars[url]! && + !_moduleConfigurations[url]!.sameOriginal(currentConfiguration) && currentConfiguration is ExplicitConfiguration) { var message = namesInErrors ? "${p.prettyUri(url)} was already loaded, so it can't be " @@ -947,6 +958,13 @@ final class _EvaluateVisitor if (url != null) { _modules[url] = module; _moduleConfigurations[url] = _configuration; + // determine if the module accessed (& consumed) any configuration variables. + // Sometimes this._configuration changes and sometime the value passed to the function shows the change + // so test both: if the number of remaining variables has gone down, then the module used them. + _moduleUsedConfigVars[url] = + (startingConfigPropVars > _configuration.length) || + (startingConfigVars > + (configuration?.length ?? 0)); //module consumed config vars if (nodeWithSpan != null) _moduleNodes[url] = nodeWithSpan; } diff --git a/lib/src/visitor/evaluate.dart b/lib/src/visitor/evaluate.dart index 5727e56aa..cc65973df 100644 --- a/lib/src/visitor/evaluate.dart +++ b/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: a3068d04660dd2bed34b884aa6e1a21d423dc4e5 +// Checksum: cc8b7436b02c3f2fbd6763aa19514ca706c0fbe5 // // ignore_for_file: unused_import @@ -164,6 +164,9 @@ final class _EvaluateVisitor /// The first [Configuration] used to load a module [Uri]. final _moduleConfigurations = {}; + // Track whether a module used one of the variables passed in a "with" clause + final _moduleUsedConfigVars = {}; + /// A map from canonical module URLs to the nodes whose spans indicate where /// those modules were originally loaded. /// @@ -862,9 +865,17 @@ final class _EvaluateVisitor }) { var url = stylesheet.span.sourceUrl; + // set up variables to determine if the module accessed (& consumed) any configuration variables. + var startingConfigPropVars = + _configuration.length; // number of this._config vars upon entry. + var startingConfigVars = (configuration?.length ?? + 0); // number of config vars passed in as a function arg. + if (_modules[url] case var alreadyLoaded?) { var currentConfiguration = configuration ?? _configuration; - if (!_moduleConfigurations[url]!.sameOriginal(currentConfiguration) && + // note: if module didn't use any of the config vars, treat it as if it was called without them. + if (_moduleUsedConfigVars[url]! && + !_moduleConfigurations[url]!.sameOriginal(currentConfiguration) && currentConfiguration is ExplicitConfiguration) { var message = namesInErrors ? "${p.prettyUri(url)} was already loaded, so it can't be " @@ -955,6 +966,13 @@ final class _EvaluateVisitor if (url != null) { _modules[url] = module; _moduleConfigurations[url] = _configuration; + // determine if the module accessed (& consumed) any configuration variables. + // Sometimes this._configuration changes and sometime the value passed to the function shows the change + // so test both: if the number of remaining variables has gone down, then the module used them. + _moduleUsedConfigVars[url] = + (startingConfigPropVars > _configuration.length) || + (startingConfigVars > + (configuration?.length ?? 0)); //module consumed config vars if (nodeWithSpan != null) _moduleNodes[url] = nodeWithSpan; } From 6642862be360d4281dc0539b061cac2674001646 Mon Sep 17 00:00:00 2001 From: arikorn Date: Thu, 3 Jul 2025 12:57:31 -0700 Subject: [PATCH 2/3] Make code a little cleaner. Note, probably the "cleanest" way to do this would be to have `visitStylesheet()` pass back `true` if it used a configuration variable/override. As best as I can tell this is possible, but since it involves changing the return value types of `visitStylesheet()` and `visitVariableDeclaration()` -- even though those return values don't appear to be used, I'm hesitant to make those changes. This PR resolves issue #2601 and issue #2598 Sass does not allow a previously-loaded module to be subsequently loaded using a `with` configuration. When numerous interdependent modules are loaded in one `@use` call, as in CoreUI, this can lead to spurious errors from modules whose contents do not include the specified variables. This commit limits `Error: This module was already loaded, so it can't be configured using "with"` errors to modules that actually are overriden by the supplied variables. As a side-effect this also improves error-reporting when the cause of the error is due to "overriding" a non-existent variable. See examples in Issue #2598 --- lib/src/visitor/async_evaluate.dart | 23 +++++++++-------------- lib/src/visitor/evaluate.dart | 25 ++++++++++--------------- 2 files changed, 19 insertions(+), 29 deletions(-) diff --git a/lib/src/visitor/async_evaluate.dart b/lib/src/visitor/async_evaluate.dart index 1541805d2..fbc4c1e0d 100644 --- a/lib/src/visitor/async_evaluate.dart +++ b/lib/src/visitor/async_evaluate.dart @@ -856,16 +856,11 @@ final class _EvaluateVisitor bool namesInErrors = false, }) async { var url = stylesheet.span.sourceUrl; - - // set up variables to determine if the module accessed (& consumed) any configuration variables. - var startingConfigPropVars = - _configuration.length; // number of this._config vars upon entry. - var startingConfigVars = (configuration?.length ?? - 0); // number of config vars passed in as a function arg. + var moduleUsedOverrides = false; if (_modules[url] case var alreadyLoaded?) { var currentConfiguration = configuration ?? _configuration; - // note: if module didn't use any of the config vars, treat it as if it was called without them. + // note: if module didn't use any of the config vars, treat it as if it was called without "with". if (_moduleUsedConfigVars[url]! && !_moduleConfigurations[url]!.sameOriginal(currentConfiguration) && currentConfiguration is ExplicitConfiguration) { @@ -927,12 +922,18 @@ final class _EvaluateVisitor _inKeyframes = false; if (configuration != null) _configuration = configuration; + // setup testing if the module accessed (& consumed) any configuration variables. + var configLength1 = _configuration.length; + await visitStylesheet(stylesheet); css = _outOfOrderImports == null ? root : CssStylesheet(_addOutOfOrderImports(), stylesheet.span); preModuleComments = _preModuleComments; + // determine if the module accessed (& consumed) any configuration variables. + moduleUsedOverrides = configLength1 > _configuration.length; + _importer = oldImporter; __stylesheet = oldStylesheet; __root = oldRoot; @@ -958,13 +959,7 @@ final class _EvaluateVisitor if (url != null) { _modules[url] = module; _moduleConfigurations[url] = _configuration; - // determine if the module accessed (& consumed) any configuration variables. - // Sometimes this._configuration changes and sometime the value passed to the function shows the change - // so test both: if the number of remaining variables has gone down, then the module used them. - _moduleUsedConfigVars[url] = - (startingConfigPropVars > _configuration.length) || - (startingConfigVars > - (configuration?.length ?? 0)); //module consumed config vars + _moduleUsedConfigVars[url] = moduleUsedOverrides; if (nodeWithSpan != null) _moduleNodes[url] = nodeWithSpan; } diff --git a/lib/src/visitor/evaluate.dart b/lib/src/visitor/evaluate.dart index cc65973df..eebf5a949 100644 --- a/lib/src/visitor/evaluate.dart +++ b/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: cc8b7436b02c3f2fbd6763aa19514ca706c0fbe5 +// Checksum: 6721ae642930e4f4ff4c01c7f5376d0c3cfc4208 // // ignore_for_file: unused_import @@ -864,16 +864,11 @@ final class _EvaluateVisitor bool namesInErrors = false, }) { var url = stylesheet.span.sourceUrl; - - // set up variables to determine if the module accessed (& consumed) any configuration variables. - var startingConfigPropVars = - _configuration.length; // number of this._config vars upon entry. - var startingConfigVars = (configuration?.length ?? - 0); // number of config vars passed in as a function arg. + var moduleUsedOverrides = false; if (_modules[url] case var alreadyLoaded?) { var currentConfiguration = configuration ?? _configuration; - // note: if module didn't use any of the config vars, treat it as if it was called without them. + // note: if module didn't use any of the config vars, treat it as if it was called without "with". if (_moduleUsedConfigVars[url]! && !_moduleConfigurations[url]!.sameOriginal(currentConfiguration) && currentConfiguration is ExplicitConfiguration) { @@ -935,12 +930,18 @@ final class _EvaluateVisitor _inKeyframes = false; if (configuration != null) _configuration = configuration; + // setup testing if the module accessed (& consumed) any configuration variables. + var configLength1 = _configuration.length; + visitStylesheet(stylesheet); css = _outOfOrderImports == null ? root : CssStylesheet(_addOutOfOrderImports(), stylesheet.span); preModuleComments = _preModuleComments; + // determine if the module accessed (& consumed) any configuration variables. + moduleUsedOverrides = configLength1 > _configuration.length; + _importer = oldImporter; __stylesheet = oldStylesheet; __root = oldRoot; @@ -966,13 +967,7 @@ final class _EvaluateVisitor if (url != null) { _modules[url] = module; _moduleConfigurations[url] = _configuration; - // determine if the module accessed (& consumed) any configuration variables. - // Sometimes this._configuration changes and sometime the value passed to the function shows the change - // so test both: if the number of remaining variables has gone down, then the module used them. - _moduleUsedConfigVars[url] = - (startingConfigPropVars > _configuration.length) || - (startingConfigVars > - (configuration?.length ?? 0)); //module consumed config vars + _moduleUsedConfigVars[url] = moduleUsedOverrides; if (nodeWithSpan != null) _moduleNodes[url] = nodeWithSpan; } From 71cf264eb0e8332b6ea2b2522b17ed6a45749d92 Mon Sep 17 00:00:00 2001 From: arikorn Date: Tue, 8 Jul 2025 01:28:29 -0700 Subject: [PATCH 3/3] Fix for case mentioned by @nex3 in issue #2601 ..in comment: https://github.com/sass/dart-sass/issues/2601#issuecomment-3046528111 This solution uses a "lazy algorithm" for catching the error. Note that the diff makes things look more drastic than they are: I simply moved the entire if-then logic for dealing with already-loaded modules. A better method might be to write a dedicated function to traverse the node tree with fewer potential side-effects.(I started on this but eventually set it aside as being more complicated than the current solution.) --- lib/src/visitor/async_evaluate.dart | 62 +++++++++++++--------------- lib/src/visitor/evaluate.dart | 64 ++++++++++++++--------------- 2 files changed, 59 insertions(+), 67 deletions(-) diff --git a/lib/src/visitor/async_evaluate.dart b/lib/src/visitor/async_evaluate.dart index fbc4c1e0d..ec6fd9482 100644 --- a/lib/src/visitor/async_evaluate.dart +++ b/lib/src/visitor/async_evaluate.dart @@ -156,9 +156,6 @@ final class _EvaluateVisitor /// The first [Configuration] used to load a module [Uri]. final _moduleConfigurations = {}; - // Track whether a module used one of the variables passed in a "with" clause - final _moduleUsedConfigVars = {}; - /// A map from canonical module URLs to the nodes whose spans indicate where /// those modules were originally loaded. /// @@ -857,35 +854,7 @@ final class _EvaluateVisitor }) async { var url = stylesheet.span.sourceUrl; var moduleUsedOverrides = false; - - if (_modules[url] case var alreadyLoaded?) { - var currentConfiguration = configuration ?? _configuration; - // note: if module didn't use any of the config vars, treat it as if it was called without "with". - if (_moduleUsedConfigVars[url]! && - !_moduleConfigurations[url]!.sameOriginal(currentConfiguration) && - currentConfiguration is ExplicitConfiguration) { - var message = namesInErrors - ? "${p.prettyUri(url)} was already loaded, so it can't be " - "configured using \"with\"." - : "This module was already loaded, so it can't be configured using " - "\"with\"."; - - var existingSpan = _moduleNodes[url]?.span; - var configurationSpan = configuration == null - ? currentConfiguration.nodeWithSpan.span - : null; - var secondarySpans = { - if (existingSpan != null) existingSpan: "original load", - if (configurationSpan != null) configurationSpan: "configuration", - }; - - throw secondarySpans.isEmpty - ? _exception(message) - : _multiSpanException(message, "new load", secondarySpans); - } - - return alreadyLoaded; - } + var currentConfiguration = configuration ?? _configuration; var environment = AsyncEnvironment(); late CssStylesheet css; @@ -951,6 +920,34 @@ final class _EvaluateVisitor _configuration = oldConfiguration; }); + if (_modules[url] case var alreadyLoaded?) { + // note: only throw an error if module used any of the config vars, otherwise treat it as if it was called without "with". + if (moduleUsedOverrides && + !_moduleConfigurations[url]!.sameOriginal(currentConfiguration) && + currentConfiguration is ExplicitConfiguration) { + var message = namesInErrors + ? "${p.prettyUri(url)} was already loaded, so it can't be " + "configured using \"with\"." + : "This module was already loaded, so it can't be configured using " + "\"with\"."; + + var existingSpan = _moduleNodes[url]?.span; + var configurationSpan = configuration == null + ? currentConfiguration.nodeWithSpan.span + : null; + var secondarySpans = { + if (existingSpan != null) existingSpan: "original load", + if (configurationSpan != null) configurationSpan: "configuration", + }; + + throw secondarySpans.isEmpty + ? _exception(message) + : _multiSpanException(message, "new load", secondarySpans); + } + + return alreadyLoaded; + } + var module = environment.toModule( css, preModuleComments ?? const {}, @@ -959,7 +956,6 @@ final class _EvaluateVisitor if (url != null) { _modules[url] = module; _moduleConfigurations[url] = _configuration; - _moduleUsedConfigVars[url] = moduleUsedOverrides; if (nodeWithSpan != null) _moduleNodes[url] = nodeWithSpan; } diff --git a/lib/src/visitor/evaluate.dart b/lib/src/visitor/evaluate.dart index eebf5a949..cc097e278 100644 --- a/lib/src/visitor/evaluate.dart +++ b/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: 6721ae642930e4f4ff4c01c7f5376d0c3cfc4208 +// Checksum: 4c0db6ff92d072b0165af02288d9d3a2782d137d // // ignore_for_file: unused_import @@ -164,9 +164,6 @@ final class _EvaluateVisitor /// The first [Configuration] used to load a module [Uri]. final _moduleConfigurations = {}; - // Track whether a module used one of the variables passed in a "with" clause - final _moduleUsedConfigVars = {}; - /// A map from canonical module URLs to the nodes whose spans indicate where /// those modules were originally loaded. /// @@ -865,35 +862,7 @@ final class _EvaluateVisitor }) { var url = stylesheet.span.sourceUrl; var moduleUsedOverrides = false; - - if (_modules[url] case var alreadyLoaded?) { - var currentConfiguration = configuration ?? _configuration; - // note: if module didn't use any of the config vars, treat it as if it was called without "with". - if (_moduleUsedConfigVars[url]! && - !_moduleConfigurations[url]!.sameOriginal(currentConfiguration) && - currentConfiguration is ExplicitConfiguration) { - var message = namesInErrors - ? "${p.prettyUri(url)} was already loaded, so it can't be " - "configured using \"with\"." - : "This module was already loaded, so it can't be configured using " - "\"with\"."; - - var existingSpan = _moduleNodes[url]?.span; - var configurationSpan = configuration == null - ? currentConfiguration.nodeWithSpan.span - : null; - var secondarySpans = { - if (existingSpan != null) existingSpan: "original load", - if (configurationSpan != null) configurationSpan: "configuration", - }; - - throw secondarySpans.isEmpty - ? _exception(message) - : _multiSpanException(message, "new load", secondarySpans); - } - - return alreadyLoaded; - } + var currentConfiguration = configuration ?? _configuration; var environment = Environment(); late CssStylesheet css; @@ -959,6 +928,34 @@ final class _EvaluateVisitor _configuration = oldConfiguration; }); + if (_modules[url] case var alreadyLoaded?) { + // note: only throw an error if module used any of the config vars, otherwise treat it as if it was called without "with". + if (moduleUsedOverrides && + !_moduleConfigurations[url]!.sameOriginal(currentConfiguration) && + currentConfiguration is ExplicitConfiguration) { + var message = namesInErrors + ? "${p.prettyUri(url)} was already loaded, so it can't be " + "configured using \"with\"." + : "This module was already loaded, so it can't be configured using " + "\"with\"."; + + var existingSpan = _moduleNodes[url]?.span; + var configurationSpan = configuration == null + ? currentConfiguration.nodeWithSpan.span + : null; + var secondarySpans = { + if (existingSpan != null) existingSpan: "original load", + if (configurationSpan != null) configurationSpan: "configuration", + }; + + throw secondarySpans.isEmpty + ? _exception(message) + : _multiSpanException(message, "new load", secondarySpans); + } + + return alreadyLoaded; + } + var module = environment.toModule( css, preModuleComments ?? const {}, @@ -967,7 +964,6 @@ final class _EvaluateVisitor if (url != null) { _modules[url] = module; _moduleConfigurations[url] = _configuration; - _moduleUsedConfigVars[url] = moduleUsedOverrides; if (nodeWithSpan != null) _moduleNodes[url] = nodeWithSpan; }