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.