Skip to content

Commit 1b3c2f9

Browse files
nex3nex3
committed
Allow unused configurations to go past @forward 
1 parent 491e95d commit 1b3c2f9

File tree

9 files changed

+148
-46
lines changed

9 files changed

+148
-46
lines changed

‎CHANGELOG.md

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,8 @@
11
## 1.90.0
22

3-
* **Potentially breaking bug fix:** Fix a situation where Sass wasn't emitting
4-
an error when loading a `@forward`ed module with a configuration when that
5-
module had already been loaded with a different configuration *and* all
6-
configured variables in the new configuration had already been used.
3+
* Allow a `@forward`ed module to be loaded with a configuration when that module
4+
has already been loaded with a different configuration *and* the module
5+
doesn't define any variables that would have been configured anyway.
76

87
## 1.89.2
98

‎lib/src/async_environment.dart

Lines changed: 42 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,10 @@ final class AsyncEnvironment {
121121
UserDefinedCallable<AsyncEnvironment>? get content => _content;
122122
UserDefinedCallable<AsyncEnvironment>? _content;
123123

124+
/// The set of variable names that could be configured when loading the
125+
/// module.
126+
final Set<String> _configurableVariables;
127+
124128
/// Whether the environment is lexically at the root of the document.
125129
bool get atRoot => _variables.length == 1;
126130

@@ -160,27 +164,28 @@ final class AsyncEnvironment {
160164
_functions = [{}],
161165
_functionIndices = {},
162166
_mixins = [{}],
163-
_mixinIndices = {};
167+
_mixinIndices = {},
168+
_configurableVariables = {};
164169

165170
AsyncEnvironment._(
166-
this._modules,
167-
this._namespaceNodes,
168-
this._globalModules,
169-
this._importedModules,
170-
this._forwardedModules,
171-
this._nestedForwardedModules,
172-
this._allModules,
173-
this._variables,
174-
this._variableNodes,
175-
this._functions,
176-
this._mixins,
177-
this._content,
178-
)
179-
// Lazily fill in the indices rather than eagerly copying them from the
180-
// existing environment in closure() because the copying took a lot of
181-
// time and was rarely helpful. This saves a bunch of time on Susy's
182-
// tests.
183-
: _variableIndices = {},
171+
this._modules,
172+
this._namespaceNodes,
173+
this._globalModules,
174+
this._importedModules,
175+
this._forwardedModules,
176+
this._nestedForwardedModules,
177+
this._allModules,
178+
this._variables,
179+
this._variableNodes,
180+
this._functions,
181+
this._mixins,
182+
this._content,
183+
this._configurableVariables)
184+
// Lazily fill in the indices rather than eagerly copying them from the
185+
// existing environment in closure() because the copying took a lot of
186+
// time and was rarely helpful. This saves a bunch of time on Susy's
187+
// tests.
188+
: _variableIndices = {},
184189
_functionIndices = {},
185190
_mixinIndices = {};
186191

@@ -202,6 +207,9 @@ final class AsyncEnvironment {
202207
_functions.toList(),
203208
_mixins.toList(),
204209
_content,
210+
// Closures are always in nested contexts where configurable variables
211+
// are never added.
212+
const {},
205213
);
206214

207215
/// Returns a new environment to use for an imported file.
@@ -222,6 +230,7 @@ final class AsyncEnvironment {
222230
_functions.toList(),
223231
_mixins.toList(),
224232
_content,
233+
_configurableVariables,
225234
);
226235

227236
/// Adds [module] to the set of modules visible in this environment.
@@ -640,6 +649,15 @@ final class AsyncEnvironment {
640649
_variableNodes[index][name] = nodeWithSpan;
641650
}
642651

652+
/// Records that [name] is a variable that could have been configured for this
653+
/// module, whether or not it actually was.
654+
///
655+
/// This is used to determine whether to throw an error when passing a new
656+
/// configuration through `@forward` to an already-loaded module.
657+
void markVariableConfigurable(String name) {
658+
_configurableVariables.add(name);
659+
}
660+
643661
/// Returns the value of the function named [name], optionally with the given
644662
/// [namespace], or `null` if no such variable is declared.
645663
///
@@ -1070,6 +1088,11 @@ final class _EnvironmentModule implements Module {
10701088
return module == null ? this : module.variableIdentity(name);
10711089
}
10721090

1091+
bool couldHaveBeenConfigured(Set<String> variables) =>
1092+
variables.length < _environment._configurableVariables.length
1093+
? variables.any(_environment._configurableVariables.contains)
1094+
: _environment._configurableVariables.any(variables.contains);
1095+
10731096
Module cloneCss() {
10741097
if (!transitivelyContainsCss) return this;
10751098

‎lib/src/environment.dart

Lines changed: 43 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
// DO NOT EDIT. This file was generated from async_environment.dart.
66
// See tool/grind/synchronize.dart for details.
77
//
8-
// Checksum: add8a3972aec53ef29de3639af2bc84edcbde9e7
8+
// Checksum: f947505f8d0057a36a175e81ba8279c70e59e9dc
99
//
1010
// ignore_for_file: unused_import
1111

@@ -128,6 +128,10 @@ final class Environment {
128128
UserDefinedCallable<Environment>? get content => _content;
129129
UserDefinedCallable<Environment>? _content;
130130

131+
/// The set of variable names that could be configured when loading the
132+
/// module.
133+
final Set<String> _configurableVariables;
134+
131135
/// Whether the environment is lexically at the root of the document.
132136
bool get atRoot => _variables.length == 1;
133137

@@ -167,27 +171,28 @@ final class Environment {
167171
_functions = [{}],
168172
_functionIndices = {},
169173
_mixins = [{}],
170-
_mixinIndices = {};
174+
_mixinIndices = {},
175+
_configurableVariables = {};
171176

172177
Environment._(
173-
this._modules,
174-
this._namespaceNodes,
175-
this._globalModules,
176-
this._importedModules,
177-
this._forwardedModules,
178-
this._nestedForwardedModules,
179-
this._allModules,
180-
this._variables,
181-
this._variableNodes,
182-
this._functions,
183-
this._mixins,
184-
this._content,
185-
)
186-
// Lazily fill in the indices rather than eagerly copying them from the
187-
// existing environment in closure() because the copying took a lot of
188-
// time and was rarely helpful. This saves a bunch of time on Susy's
189-
// tests.
190-
: _variableIndices = {},
178+
this._modules,
179+
this._namespaceNodes,
180+
this._globalModules,
181+
this._importedModules,
182+
this._forwardedModules,
183+
this._nestedForwardedModules,
184+
this._allModules,
185+
this._variables,
186+
this._variableNodes,
187+
this._functions,
188+
this._mixins,
189+
this._content,
190+
this._configurableVariables)
191+
// Lazily fill in the indices rather than eagerly copying them from the
192+
// existing environment in closure() because the copying took a lot of
193+
// time and was rarely helpful. This saves a bunch of time on Susy's
194+
// tests.
195+
: _variableIndices = {},
191196
_functionIndices = {},
192197
_mixinIndices = {};
193198

@@ -209,6 +214,9 @@ final class Environment {
209214
_functions.toList(),
210215
_mixins.toList(),
211216
_content,
217+
// Closures are always in nested contexts where configurable variables
218+
// are never added.
219+
const {},
212220
);
213221

214222
/// Returns a new environment to use for an imported file.
@@ -229,6 +237,7 @@ final class Environment {
229237
_functions.toList(),
230238
_mixins.toList(),
231239
_content,
240+
_configurableVariables,
232241
);
233242

234243
/// Adds [module] to the set of modules visible in this environment.
@@ -648,6 +657,15 @@ final class Environment {
648657
_variableNodes[index][name] = nodeWithSpan;
649658
}
650659

660+
/// Records that [name] is a variable that could have been configured for this
661+
/// module, whether or not it actually was.
662+
///
663+
/// This is used to determine whether to throw an error when passing a new
664+
/// configuration through `@forward` to an already-loaded module.
665+
void markVariableConfigurable(String name) {
666+
_configurableVariables.add(name);
667+
}
668+
651669
/// Returns the value of the function named [name], optionally with the given
652670
/// [namespace], or `null` if no such variable is declared.
653671
///
@@ -1080,6 +1098,11 @@ final class _EnvironmentModule implements Module<Callable> {
10801098
return module == null ? this : module.variableIdentity(name);
10811099
}
10821100

1101+
bool couldHaveBeenConfigured(Set<String> variables) =>
1102+
variables.length < _environment._configurableVariables.length
1103+
? variables.any(_environment._configurableVariables.contains)
1104+
: _environment._configurableVariables.any(variables.contains);
1105+
10831106
Module<Callable> cloneCss() {
10841107
if (!transitivelyContainsCss) return this;
10851108

‎lib/src/module.dart

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,10 @@ abstract interface class Module<T extends AsyncCallable> {
8080
/// question, as defined by the Sass spec.
8181
Object variableIdentity(String name);
8282

83+
/// Whether this module exposes any variables from among [variables] that
84+
/// could have been configured when the module was loaded.
85+
bool couldHaveBeenConfigured(Set<String> variables);
86+
8387
/// Creates a copy of this module with new [css] and [extender].
8488
Module<T> cloneCss();
8589
}

‎lib/src/module/built_in.dart

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,5 +62,7 @@ final class BuiltInModule<T extends AsyncCallable> implements Module<T> {
6262
return this;
6363
}
6464

65+
bool couldHaveBeenConfigured(Set<String> _) => false;
66+
6567
Module<T> cloneCss() => this;
6668
}

‎lib/src/module/forwarded_view.dart

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,31 @@ class ForwardedModuleView<T extends AsyncCallable> implements Module<T> {
140140
return _inner.variableIdentity(name);
141141
}
142142

143+
bool couldHaveBeenConfigured(Set<String> variables) {
144+
assert(_rule.shownVariables == null || _rule.hiddenVariables == null);
145+
if (_rule.prefix == null &&
146+
_rule.shownVariables == null &&
147+
(_rule.hiddenVariables?.isEmpty ?? true)) {
148+
return _inner.couldHaveBeenConfigured(variables);
149+
}
150+
151+
if (_rule.prefix case var prefix?) {
152+
variables = {
153+
for (var name in variables)
154+
if (name.startsWith(prefix)) name.substring(prefix.length)
155+
};
156+
}
157+
158+
if (_rule.shownVariables case var safelist?) {
159+
return _inner.couldHaveBeenConfigured(variables.intersection(safelist));
160+
} else if (_rule.hiddenVariables case var blocklist?
161+
when blocklist.isNotEmpty) {
162+
return _inner.couldHaveBeenConfigured(variables.difference(blocklist));
163+
} else {
164+
return _inner.couldHaveBeenConfigured(variables);
165+
}
166+
}
167+
143168
bool operator ==(Object other) =>
144169
other is ForwardedModuleView &&
145170
_inner == other._inner &&

‎lib/src/module/shadowed_view.dart

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,14 @@ final class ShadowedModuleView<T extends AsyncCallable> implements Module<T> {
107107
return _inner.variableIdentity(name);
108108
}
109109

110+
bool couldHaveBeenConfigured(Set<String> variables) =>
111+
this.variables == _inner.variables
112+
? _inner.couldHaveBeenConfigured(variables)
113+
: _inner.couldHaveBeenConfigured({
114+
for (var name in this.variables.keys)
115+
if (variables.contains(name)) name
116+
});
117+
110118
bool operator ==(Object other) =>
111119
other is ShadowedModuleView &&
112120
_inner == other._inner &&

‎lib/src/visitor/async_evaluate.dart

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -857,7 +857,15 @@ final class _EvaluateVisitor
857857
var currentConfiguration = configuration ?? _configuration;
858858
if (_modules[url] case var alreadyLoaded?) {
859859
if (!_moduleConfigurations[url]!.sameOriginal(currentConfiguration) &&
860-
currentConfiguration is ExplicitConfiguration) {
860+
currentConfiguration is ExplicitConfiguration &&
861+
// Don't throw an error if the module being loaded doesn't expose any
862+
// configurable variables that could have been affected by the
863+
// configuration in the first place. If the configuration defines a
864+
// value that's not in the module, it'll still throw an error, but
865+
// this avoids throwing confusing errors for `@forward`ed modules
866+
// without configuration.
867+
alreadyLoaded.couldHaveBeenConfigured(
868+
MapKeySet(currentConfiguration.values))) {
861869
var message = namesInErrors
862870
? "${p.prettyUri(url)} was already loaded, so it can't be "
863871
"configured using \"with\"."
@@ -2599,6 +2607,7 @@ final class _EvaluateVisitor
25992607
Future<Value?> visitVariableDeclaration(VariableDeclaration node) async {
26002608
if (node.isGuarded) {
26012609
if (node.namespace == null && _environment.atRoot) {
2610+
_environment.markVariableConfigurable(node.name);
26022611
if (_configuration.remove(node.name) case var override?
26032612
when override.value != sassNull) {
26042613
_addExceptionSpan(node, () {

‎lib/src/visitor/evaluate.dart

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
// DO NOT EDIT. This file was generated from async_evaluate.dart.
66
// See tool/grind/synchronize.dart for details.
77
//
8-
// Checksum: 30755d20ab8cdb065eb2c24d27a5f8a248591fcd
8+
// Checksum: 70d926fd13a7d35cd805bf7053f073cc123b260f
99
//
1010
// ignore_for_file: unused_import
1111

@@ -865,7 +865,15 @@ final class _EvaluateVisitor
865865
var currentConfiguration = configuration ?? _configuration;
866866
if (_modules[url] case var alreadyLoaded?) {
867867
if (!_moduleConfigurations[url]!.sameOriginal(currentConfiguration) &&
868-
currentConfiguration is ExplicitConfiguration) {
868+
currentConfiguration is ExplicitConfiguration &&
869+
// Don't throw an error if the module being loaded doesn't expose any
870+
// configurable variables that could have been affected by the
871+
// configuration in the first place. If the configuration defines a
872+
// value that's not in the module, it'll still throw an error, but
873+
// this avoids throwing confusing errors for `@forward`ed modules
874+
// without configuration.
875+
alreadyLoaded.couldHaveBeenConfigured(
876+
MapKeySet(currentConfiguration.values))) {
869877
var message = namesInErrors
870878
? "${p.prettyUri(url)} was already loaded, so it can't be "
871879
"configured using \"with\"."
@@ -2604,6 +2612,7 @@ final class _EvaluateVisitor
26042612
Value? visitVariableDeclaration(VariableDeclaration node) {
26052613
if (node.isGuarded) {
26062614
if (node.namespace == null && _environment.atRoot) {
2615+
_environment.markVariableConfigurable(node.name);
26072616
if (_configuration.remove(node.name) case var override?
26082617
when override.value != sassNull) {
26092618
_addExceptionSpan(node, () {

0 commit comments

Comments
 (0)