Eagerly initialize Environment._globalModules (#952)

We had been lazily initializing this to be more efficient when no global modules were used, but this meant that the object wasn't shared with closures created for mixins and functions that were created when it was still `null`, so later imported forwards weren't visible to those members.
sass · Feb 13, 2020 · 6d38824 · 6d38824
1 parent 0a2142d
commit 6d38824
Show file tree

Hide file tree

Showing 3 changed files with 18 additions and 39 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,6 +6,9 @@
 * Don't throw errors if the exact same member is loaded or forwarded from
   multiple modules at the same time.
 
+* Fix a bug where imported forwarded members weren't visible in mixins and
+  functions that were defined before the `@import`.
+
 ## 1.25.2
 
 * Fix a bug where, under extremely rare circumstances, a valid variable could

diff --git a/lib/src/async_environment.dart b/lib/src/async_environment.dart
@@ -40,15 +40,11 @@ class AsyncEnvironment {
   final Map<String, AstNode> _namespaceNodes;
 
   /// The namespaceless modules used in the current scope.
-  ///
-  /// This is `null` if there are no namespaceless modules.
-  Set<Module> _globalModules;
+  final Set<Module> _globalModules;
 
   /// A map from modules in [_globalModules] to the nodes whose spans
   /// indicate where those modules were originally loaded.
-  ///
-  /// This is `null` if there are no namespaceless modules.
-  Map<Module, AstNode> _globalModuleNodes;
+  final Map<Module, AstNode> _globalModuleNodes;
 
   /// The modules forwarded by this module.
   ///
@@ -153,8 +149,8 @@ class AsyncEnvironment {
   AsyncEnvironment({bool sourceMap = false})
       : _modules = {},
         _namespaceNodes = {},
-        _globalModules = null,
-        _globalModuleNodes = null,
+        _globalModules = {},
+        _globalModuleNodes = {},
         _forwardedModules = null,
         _forwardedModuleNodes = null,
         _nestedForwardedModules = null,
@@ -216,8 +212,8 @@ class AsyncEnvironment {
   AsyncEnvironment forImport() => AsyncEnvironment._(
       {},
       {},
-      null,
-      null,
+      {},
+      {},
       null,
       null,
       null,
@@ -240,8 +236,6 @@ class AsyncEnvironment {
   /// with the same name as a variable defined in this environment.
   void addModule(Module module, AstNode nodeWithSpan, {String namespace}) {
     if (namespace == null) {
-      _globalModules ??= {};
-      _globalModuleNodes ??= {};
       _globalModules.add(module);
       _globalModuleNodes[module] = nodeWithSpan;
       _allModules.add(module);
@@ -337,8 +331,6 @@ class AsyncEnvironment {
       var forwarded = module._environment._forwardedModules;
       if (forwarded == null) return;
 
-      _globalModules ??= {};
-      _globalModuleNodes ??= {};
       _forwardedModules ??= [];
       _forwardedModuleNodes ??= {};
 
@@ -487,8 +479,6 @@ class AsyncEnvironment {
   /// required, since some nodes need to do real work to manufacture a source
   /// span.
   AstNode _getVariableNodeFromGlobalModule(String name) {
-    if (_globalModules == null) return null;
-
     // We don't need to worry about multiple modules defining the same variable,
     // because that's already been checked by [getVariable].
     for (var module in _globalModules) {
@@ -558,7 +548,7 @@ class AsyncEnvironment {
 
       // If this module doesn't already contain a variable named [name], try
       // setting it in a global module.
-      if (!_variables.first.containsKey(name) && _globalModules != null) {
+      if (!_variables.first.containsKey(name)) {
         var moduleWithName = _fromOneModule(name, "variable",
             (module) => module.variables.containsKey(name) ? module : null);
         if (moduleWithName != null) {
@@ -861,8 +851,6 @@ class AsyncEnvironment {
       }
     }
 
-    if (_globalModules == null) return null;
-
     T value;
     Object identity;
     for (var module in _globalModules) {

diff --git a/lib/src/environment.dart b/lib/src/environment.dart
@@ -5,7 +5,7 @@
 // DO NOT EDIT. This file was generated from async_environment.dart.
 // See tool/grind/synchronize.dart for details.
 //
-// Checksum: db31838dbc5c44989803274acb581263e98b488d
+// Checksum: df5ee8bde1eec6e47c1d025041921aba01637696
 //
 // ignore_for_file: unused_import
 
@@ -46,15 +46,11 @@ class Environment {
   final Map<String, AstNode> _namespaceNodes;
 
   /// The namespaceless modules used in the current scope.
-  ///
-  /// This is `null` if there are no namespaceless modules.
-  Set<Module<Callable>> _globalModules;
+  final Set<Module<Callable>> _globalModules;
 
   /// A map from modules in [_globalModules] to the nodes whose spans
   /// indicate where those modules were originally loaded.
-  ///
-  /// This is `null` if there are no namespaceless modules.
-  Map<Module<Callable>, AstNode> _globalModuleNodes;
+  final Map<Module<Callable>, AstNode> _globalModuleNodes;
 
   /// The modules forwarded by this module.
   ///
@@ -159,8 +155,8 @@ class Environment {
   Environment({bool sourceMap = false})
       : _modules = {},
         _namespaceNodes = {},
-        _globalModules = null,
-        _globalModuleNodes = null,
+        _globalModules = {},
+        _globalModuleNodes = {},
         _forwardedModules = null,
         _forwardedModuleNodes = null,
         _nestedForwardedModules = null,
@@ -222,8 +218,8 @@ class Environment {
   Environment forImport() => Environment._(
       {},
       {},
-      null,
-      null,
+      {},
+      {},
       null,
       null,
       null,
@@ -247,8 +243,6 @@ class Environment {
   void addModule(Module<Callable> module, AstNode nodeWithSpan,
       {String namespace}) {
     if (namespace == null) {
-      _globalModules ??= {};
-      _globalModuleNodes ??= {};
       _globalModules.add(module);
       _globalModuleNodes[module] = nodeWithSpan;
       _allModules.add(module);
@@ -344,8 +338,6 @@ class Environment {
       var forwarded = module._environment._forwardedModules;
       if (forwarded == null) return;
 
-      _globalModules ??= {};
-      _globalModuleNodes ??= {};
       _forwardedModules ??= [];
       _forwardedModuleNodes ??= {};
 
@@ -494,8 +486,6 @@ class Environment {
   /// required, since some nodes need to do real work to manufacture a source
   /// span.
   AstNode _getVariableNodeFromGlobalModule(String name) {
-    if (_globalModules == null) return null;
-
     // We don't need to worry about multiple modules defining the same variable,
     // because that's already been checked by [getVariable].
     for (var module in _globalModules) {
@@ -565,7 +555,7 @@ class Environment {
 
       // If this module doesn't already contain a variable named [name], try
       // setting it in a global module.
-      if (!_variables.first.containsKey(name) && _globalModules != null) {
+      if (!_variables.first.containsKey(name)) {
         var moduleWithName = _fromOneModule(name, "variable",
             (module) => module.variables.containsKey(name) ? module : null);
         if (moduleWithName != null) {
@@ -867,8 +857,6 @@ class Environment {
       }
     }
 
-    if (_globalModules == null) return null;
-
     T value;
     Object identity;
     for (var module in _globalModules) {