Skip to content

Commit

Permalink
Always use a non-null map for _EvaluateVisitor._configuration (#827)
Browse files Browse the repository at this point in the history 
We had been using null to represent an empty configuration, but that
caused problems when an empty map snuck its way in as well. Now we
always use maps, with a const empty map for the common case.

See sass/sass#2744
  • Loading branch information
@nex3
nex3 committed Sep 27, 2019
1 parent 7bfbba0 commit 75305a1
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 15 deletions.
26 changes: 19 additions & 7 deletions lib/src/visitor/async_evaluate.dart
View file
Expand Up @@ -253,7 +253,11 @@ class _EvaluateVisitor

/// A map from variable names to the values that override their `!default`
/// definitions in this module.
Map<String, _ConfiguredValue> _configuration;
///
/// If this is empty, that indicates that the current module is not confiured.
/// Note that it may be unmodifiable when empty, in which case [Map.remove]
/// must not be called.
var _configuration = const <String, _ConfiguredValue>{};

/// Creates a new visitor.
///
Expand Down Expand Up @@ -521,9 +525,11 @@ class _EvaluateVisitor
{Uri baseUrl,
Map<String, _ConfiguredValue> configuration,
bool namesInErrors = false}) async {
configuration ??= const {};

var builtInModule = _builtInModules[url];
if (builtInModule != null) {
if (configuration != null || _configuration != null) {
if (configuration.isNotEmpty || _configuration.isNotEmpty) {
throw _exception(
namesInErrors
? "Built-in module $url can't be configured."
Expand Down Expand Up @@ -583,11 +589,12 @@ class _EvaluateVisitor
Future<Module> _execute(AsyncImporter importer, Stylesheet stylesheet,
{Map<String, _ConfiguredValue> configuration,
bool namesInErrors = false}) async {
configuration ??= const {};
var url = stylesheet.span.sourceUrl;

var alreadyLoaded = _modules[url];
if (alreadyLoaded != null) {
if (configuration != null || _configuration != null) {
if (configuration.isNotEmpty || _configuration.isNotEmpty) {
throw _exception(namesInErrors
? "${p.prettyUri(url)} was already loaded, so it can't be "
"configured using \"with\"."
Expand Down Expand Up @@ -630,7 +637,7 @@ class _EvaluateVisitor
_atRootExcludingStyleRule = false;
_inKeyframes = false;

if (configuration != null) _configuration = Map.of(configuration);
if (configuration.isNotEmpty) _configuration = Map.of(configuration);

await visitStylesheet(stylesheet);
css = _outOfOrderImports == null
Expand All @@ -651,7 +658,7 @@ class _EvaluateVisitor
_atRootExcludingStyleRule = oldAtRootExcludingStyleRule;
_inKeyframes = oldInKeyframes;

if (configuration != null && _configuration.isNotEmpty) {
if (configuration.isNotEmpty && _configuration.isNotEmpty) {
throw _exception(
namesInErrors
? "\$${_configuration.keys.first} was not declared with "
Expand Down Expand Up @@ -1201,7 +1208,7 @@ class _EvaluateVisitor
// configuration variable is used by removing it even when the underlying
// map is wrapped.
var oldConfiguration = _configuration;
if (_configuration != null) {
if (_configuration.isNotEmpty) {
if (node.prefix != null) {
_configuration = UnprefixedMapView(_configuration, node.prefix);
}
Expand Down Expand Up @@ -1720,7 +1727,12 @@ class _EvaluateVisitor
Future<Value> visitVariableDeclaration(VariableDeclaration node) async {
if (node.isGuarded) {
if (node.namespace == null && _environment.atRoot) {
var override = _configuration?.remove(node.name);
// Explicitly check whether [_configuration] is empty because if it is,
// it may be a constant map which doesn't support `remove()`.
//
// See also dart-lang/sdk#38540.
var override =
_configuration.isEmpty ? null : _configuration.remove(node.name);
if (override != null) {
_addExceptionSpan(node, () {
_environment.setVariable(
Expand Down
28 changes: 20 additions & 8 deletions lib/src/visitor/evaluate.dart
View file
Expand Up @@ -5,7 +5,7 @@
// DO NOT EDIT. This file was generated from async_evaluate.dart.
// See tool/grind/synchronize.dart for details.
//
// Checksum: 3fc19891432af3cebdc0f36730e57cbbf672d959
// Checksum: d520e2c69342b6e9c9ce0ddf6e75ac74bdf80574
//
// ignore_for_file: unused_import

Expand Down Expand Up @@ -261,7 +261,11 @@ class _EvaluateVisitor

/// A map from variable names to the values that override their `!default`
/// definitions in this module.
Map<String, _ConfiguredValue> _configuration;
///
/// If this is empty, that indicates that the current module is not confiured.
/// Note that it may be unmodifiable when empty, in which case [Map.remove]
/// must not be called.
var _configuration = const <String, _ConfiguredValue>{};

/// Creates a new visitor.
///
Expand Down Expand Up @@ -527,9 +531,11 @@ class _EvaluateVisitor
{Uri baseUrl,
Map<String, _ConfiguredValue> configuration,
bool namesInErrors = false}) {
configuration ??= const {};

var builtInModule = _builtInModules[url];
if (builtInModule != null) {
if (configuration != null || _configuration != null) {
if (configuration.isNotEmpty || _configuration.isNotEmpty) {
throw _exception(
namesInErrors
? "Built-in module $url can't be configured."
Expand Down Expand Up @@ -589,11 +595,12 @@ class _EvaluateVisitor
Module<Callable> _execute(Importer importer, Stylesheet stylesheet,
{Map<String, _ConfiguredValue> configuration,
bool namesInErrors = false}) {
configuration ??= const {};
var url = stylesheet.span.sourceUrl;

var alreadyLoaded = _modules[url];
if (alreadyLoaded != null) {
if (configuration != null || _configuration != null) {
if (configuration.isNotEmpty || _configuration.isNotEmpty) {
throw _exception(namesInErrors
? "${p.prettyUri(url)} was already loaded, so it can't be "
"configured using \"with\"."
Expand Down Expand Up @@ -636,7 +643,7 @@ class _EvaluateVisitor
_atRootExcludingStyleRule = false;
_inKeyframes = false;

if (configuration != null) _configuration = Map.of(configuration);
if (configuration.isNotEmpty) _configuration = Map.of(configuration);

visitStylesheet(stylesheet);
css = _outOfOrderImports == null
Expand All @@ -657,7 +664,7 @@ class _EvaluateVisitor
_atRootExcludingStyleRule = oldAtRootExcludingStyleRule;
_inKeyframes = oldInKeyframes;

if (configuration != null && _configuration.isNotEmpty) {
if (configuration.isNotEmpty && _configuration.isNotEmpty) {
throw _exception(
namesInErrors
? "\$${_configuration.keys.first} was not declared with "
Expand Down Expand Up @@ -1202,7 +1209,7 @@ class _EvaluateVisitor
// configuration variable is used by removing it even when the underlying
// map is wrapped.
var oldConfiguration = _configuration;
if (_configuration != null) {
if (_configuration.isNotEmpty) {
if (node.prefix != null) {
_configuration = UnprefixedMapView(_configuration, node.prefix);
}
Expand Down Expand Up @@ -1714,7 +1721,12 @@ class _EvaluateVisitor
Value visitVariableDeclaration(VariableDeclaration node) {
if (node.isGuarded) {
if (node.namespace == null && _environment.atRoot) {
var override = _configuration?.remove(node.name);
// Explicitly check whether [_configuration] is empty because if it is,
// it may be a constant map which doesn't support `remove()`.
//
// See also dart-lang/sdk#38540.
var override =
_configuration.isEmpty ? null : _configuration.remove(node.name);
if (override != null) {
_addExceptionSpan(node, () {
_environment.setVariable(
Expand Down

0 comments on commit 75305a1

Please sign in to comment.