Fix: Error checking of @use/@forward ... with... is too strict

[arikorn]

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

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[arikorn]

Just to add clarity: This PR resolves both issue #2598 and issue #2601

[arikorn]

From the second commit's message:
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.

[arikorn]

Added a commit to fix behavior for the case mentioned by @nex3 in issue #2601 in comment: #2601 (comment)

This solution uses a "delayed error-check" 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.)

[arikorn]

(Updated "lazy algorim" to "delayed error-check" which is more descriptive: the code traverses the node tree before deciding whether to keep the result -- a new module -- or discard it -- if it's an existing module -- and either throw an error or return the cached module.)

[nex3]

This solution isn't suitable because it breaks the core module system guarantee that each module will only be evaluated once. This is what I mean when I say it's difficult to determine if a variable would have been used by an already-loaded module: we can't re-run the module because the point of @use is to re-use the existing module rather than spend time evaluating it all over again (not to mention that this could cause numerous observable side effects).

[arikorn]

@nex3 the code effectively evaluates modules only once, i.e. it always returns the original evaluation of the module. So the only question is unwanted side-effects. Can you provide examples of unwanted side-effects? This would be of great help.

I think the real question, as I alluded to in issue #2601, is if you are willing to consider this idea at all, i.e. "Ensuring that no module is affected by variables that are not part of the module." for all the reasons I mentioned here and there?

If yes, then I would be happy to work with you to make the code work to your standards. I'm pretty sure I know how to do it already... Which takes us back to the first line of this comment -- can you help me with an example that demonstrates the "side-effects" problem?

[nex3]

Here's a straightforward example: if the stylesheet contains @debug "Some message", that will be printed twice under your code. You simply cannot call visitStylesheet() multiple times for the same stylesheet, and any attempt to do so is doomed to failure.

I'm trying not to totally write off the idea of allowing configuration to be passed to an already-loaded module that wouldn't have used it anyway, but I think this is a pretty narrow use-case and I don't want to add a bunch of extra tracking to the environment to allow us to determine whether or not a module actually did contain a !default variable with the given name. If you can come up with a clever way of determining that inexpensively, or in a place where the expense is only incurred when an error would have happened otherwise (and the code isn't too onerous), then that's fine. But failing that, I think the only reasonable course of action is to go with #2600 and make this an error more consistently.

[arikorn]

@nex3, Natalie, I am impressed that you managed to express your opinion of this idea in a way that was not entirely negative! 😁 Well done!

One thing you wrote though, I have to disagree with: Generally speaking, I find it difficult to call an issue that affects "millions" of developers (as mentioned in the issue) "a pretty narrow use-case."

But on a more serious note: there's an easy way to get to 90%-100% of what you want -- i.e. by adding an if-then at the beginning of _execute() so it only revisits the file dependencies if overrides were specified (i.e. with is not empty). That way, extra evaluations will only happen in cases where an error would have otherwise occurred (especially with PR #2600). If you find this acceptable I can probably post it tomorrow.

A more complete solution is no doubt possible as well -- I alluded to it earlier -- but it would take longer for me to find the time to continue working on it.

I think the real question is: Is there a side-effect of re-evaluation that would affect the CSS output? If not, then my first option should definitely be more than adequate, and probably represents the least-intrusive solution to the problem.

[nex3]

@nex3, Natalie, I am impressed that you managed to express your opinion of this idea in a way that was not entirely negative! 😁 Well done!

Snide comments aren't helpful.

One thing you wrote though, I have to disagree with: Generally speaking, I find it difficult to call an issue that affects "millions" of developers (as mentioned in the issue) "a pretty narrow use-case."

It does seem to be true that CoreUI's current releases triggers this. In that case, we'll probably need to do a deprecation process for it, but it should still change. Fortunately, the core issue should be quite fixable within CoreUI itself.

But on a more serious note: there's an easy way to get to 90%-100% of what you want -- i.e. by adding an if-then at the beginning of _execute() so it only revisits the file dependencies if overrides were specified (i.e. with is not empty). That way, extra evaluations will only happen in cases where an error would have otherwise occurred (especially with PR #2600). If you find this acceptable I can probably post it tomorrow.

A more complete solution is no doubt possible as well -- I alluded to it earlier -- but it would take longer for me to find the time to continue working on it.

I think the real question is: Is there a side-effect of re-evaluation that would affect the CSS output? If not, then my first option should definitely be more than adequate, and probably represents the least-intrusive solution to the problem.

You need to believe me when I say that evaluating modules multiple times is a non-starter. Loading a module only once is one of the core goals of the module system. And yes, it is trivially observable in the CSS output as well. Any CSS in the double-loaded module would appear twice, or if you want to omit the CSS from future loads, then the configuration won't do anything if it's used in the module itself. Any interactions that module has with the state of other, single-loaded modules would also be visible.

I've outlined what would be necessary to allow passing configuration "past" an already-configured module. That's the path forward here, if there is one. I recommend focusing on that rather than trying to relitigate something I've made clear is not a possibility.

[arikorn]

@nex3 , no snideness was intended. I was just amused at the way you worded your assent and truly felt that you should be complimented for saying something, that judging from the way you wrote it, was outside your comfort zone. Apologies if my gentle ribbing was taken poorly or if I misinterpreted your writing style! We really are both aligned in our intentions to make Sass a better product!

In response to you recent comment: I am very pleased that you have acknowledged the potential breadth of "breaking" consequences in your proposed PR #2600. This greatly relieves the pressure to have this PR accepted or done hastily, though I still believe my PR has plenty of merit. Just to summarize what I've written in issue #2601. The ideas introduced in issue 2601 and this PR would

• Increase modularity in Sass,
• Improve the specificity (actionability) of certain error messages and
• Improve the robustness, particularly by removing some order-based sensitivity.

I encountered all of these issues when updating a popular CoreUI/Sass-based application to work with @use/@forward instead of @import.

In terms of Sass' goals, I believe that this PR furthers both the high level goal of "Locality" and the low-level goal of "Backwards Compatibility" (making the transition from @import easier).

An finally, while I am no longer advocating the current state of the PR, I did just test it with a very simple example, and it did not generate two copies of the css. Presumably it's because the code in this PR discards all but the first visit to the module, but perhaps I merely misunderstand what conditions would generate duplicated outputs? Here's what I did:

// App2.scss
@use 'scss/coreui2' with (
      $newvar: #bbb,
      $white: #aaa
) ;
@use 'scss/coreui2' as _;

//coreui2.scss
@forward "variables";
@forward "functions";

$newvar: null !default;

body {
	background-color: $newvar;
}

@debug "loaded coureui2.scss"

(The remaining files are as described in Issue #2598, and in https://github.com/arikorn/sass-bug-demo)

The debug output shows that coureui2 was evaluated twice but this did not affect the css output.:

..\dart-tests\scss\coreui2.scss:11 Debug: loaded coureui2.scss
..\dart-tests\scss\coreui2.scss:11 Debug: loaded coureui2.scss
body {
background-color: #bbb;
}

With the first suggestion in my previous comment coreui2 would be evaluated only once, so even that anomaly would go away -- which is why I called it a 90%-100% solution. (Again I'm no longer advocating this, merely pointing out that my suggestion was not as "incompatible" with Sass goals as you have suggested -- an perhaps most importantly, it serves as a good POC for the goals stated in this PR.)

[nex3]

Closing in favor of #2600