From 1063cb1b9fa231fc628d13c62e378bd794a9a1e7 Mon Sep 17 00:00:00 2001 From: Natalie Weizenbaum Date: Tue, 24 Sep 2019 10:21:19 +0100 Subject: [PATCH] Always use a non-null map for _EvaluateVisitor._configuration 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 --- lib/src/visitor/async_evaluate.dart | 26 +++++++++++++++++++------- lib/src/visitor/evaluate.dart | 28 ++++++++++++++++++++-------- 2 files changed, 39 insertions(+), 15 deletions(-) diff --git a/lib/src/visitor/async_evaluate.dart b/lib/src/visitor/async_evaluate.dart index 594d3dc45..22c184deb 100644 --- a/lib/src/visitor/async_evaluate.dart +++ b/lib/src/visitor/async_evaluate.dart @@ -253,7 +253,11 @@ class _EvaluateVisitor /// A map from variable names to the values that override their `!default` /// definitions in this module. - Map _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 {}; /// Creates a new visitor. /// @@ -521,9 +525,11 @@ class _EvaluateVisitor {Uri baseUrl, Map 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." @@ -583,11 +589,12 @@ class _EvaluateVisitor Future _execute(AsyncImporter importer, Stylesheet stylesheet, {Map 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\"." @@ -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 @@ -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 " @@ -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); } @@ -1720,7 +1727,12 @@ class _EvaluateVisitor Future 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( diff --git a/lib/src/visitor/evaluate.dart b/lib/src/visitor/evaluate.dart index b38158188..8736dd25a 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: 5c9f270ef574f9c6062421ed1866af3d07672b46 +// Checksum: 81a3519c2d9182c91da1907a5129f3810c0ecdb5 // // ignore_for_file: unused_import @@ -261,7 +261,11 @@ class _EvaluateVisitor /// A map from variable names to the values that override their `!default` /// definitions in this module. - Map _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 {}; /// Creates a new visitor. /// @@ -527,9 +531,11 @@ class _EvaluateVisitor {Uri baseUrl, Map 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." @@ -589,11 +595,12 @@ class _EvaluateVisitor Module _execute(Importer importer, Stylesheet stylesheet, {Map 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\"." @@ -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 @@ -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 " @@ -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); } @@ -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(