Merge pull request #945 from sass/fix-bugs

Fix a number of defects related to imported environments

Author: nex3

CHANGELOG.md

@@ -1,6 +1,7 @@
-## 1.25.1-test.1
+## 1.25.2
 
-* No user-visible changes.
+* Fix a bug where, under extremely rare circumstances, a valid variable could
+  become unassigned.
 
 ## 1.25.0
 

lib/src/async_environment.dart

@@ -209,11 +209,11 @@ class AsyncEnvironment {
       _mixins.toList(),
       _content);
 
-  /// Returns a new global environment.
+  /// Returns a new environment to use for an imported file.
   ///
-  /// The returned environment shares this environment's global variables,
-  /// functions, and mixins, but not its modules.
-  AsyncEnvironment global() => AsyncEnvironment._(
+  /// The returned environment shares this environment's variables, functions,
+  /// and mixins, but not its modules.
+  AsyncEnvironment forImport() => AsyncEnvironment._(
       {},
       {},
       null,
@@ -419,7 +419,12 @@ class AsyncEnvironment {
     }
 
     index = _variableIndex(name);
-    if (index == null) return _getVariableFromGlobalModule(name);
+    if (index == null) {
+      // There isn't a real variable defined as this index, but it will cause
+      // [getVariable] to short-circuit and get to this function faster next
+      // time the variable is accessed.
+      return _getVariableFromGlobalModule(name);
+    }
 
     _lastVariableName = name;
     _lastVariableIndex = index;
@@ -476,12 +481,6 @@ class AsyncEnvironment {
   /// required, since some nodes need to do real work to manufacture a source
   /// span.
   AstNode _getVariableNodeFromGlobalModule(String name) {
-    // There isn't a real variable defined as this index, but it will cause
-    // [getVariable] to short-circuit and get to this function faster next time
-    // the variable is accessed.
-    _lastVariableName = name;
-    _lastVariableIndex = 0;
-
     if (_globalModules == null) return null;
 
     // We don't need to worry about multiple modules defining the same variable,
@@ -811,6 +810,20 @@ class AsyncEnvironment {
         forwarded: _forwardedModules);
   }
 
+  /// Returns a module with the same members and upstream modules as [this], but
+  /// an empty stylesheet and extender.
+  ///
+  /// This is used when resolving imports, since they need to inject forwarded
+  /// members into the current scope. It's the only situation in which a nested
+  /// environment can become a module.
+  Module toDummyModule() {
+    return _EnvironmentModule(
+        this,
+        CssStylesheet(const [], SourceFile.decoded(const []).span(0)),
+        Extender.empty,
+        forwarded: _forwardedModules);
+  }
+
   /// Returns the module with the given [namespace], or throws a
   /// [SassScriptException] if none exists.
   Module _getModule(String namespace) {

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: b497eab76eb15ba7bfc4f1cecf71ff9f9c1fb2a5
+// Checksum: b5212ffc7c50a8e7e436b25c7c16eb2996da2def
 //
 // ignore_for_file: unused_import
 
@@ -215,11 +215,11 @@ class Environment {
       _mixins.toList(),
       _content);
 
-  /// Returns a new global environment.
+  /// Returns a new environment to use for an imported file.
   ///
-  /// The returned environment shares this environment's global variables,
-  /// functions, and mixins, but not its modules.
-  Environment global() => Environment._(
+  /// The returned environment shares this environment's variables, functions,
+  /// and mixins, but not its modules.
+  Environment forImport() => Environment._(
       {},
       {},
       null,
@@ -426,7 +426,12 @@ class Environment {
     }
 
     index = _variableIndex(name);
-    if (index == null) return _getVariableFromGlobalModule(name);
+    if (index == null) {
+      // There isn't a real variable defined as this index, but it will cause
+      // [getVariable] to short-circuit and get to this function faster next
+      // time the variable is accessed.
+      return _getVariableFromGlobalModule(name);
+    }
 
     _lastVariableName = name;
     _lastVariableIndex = index;
@@ -483,12 +488,6 @@ class Environment {
   /// required, since some nodes need to do real work to manufacture a source
   /// span.
   AstNode _getVariableNodeFromGlobalModule(String name) {
-    // There isn't a real variable defined as this index, but it will cause
-    // [getVariable] to short-circuit and get to this function faster next time
-    // the variable is accessed.
-    _lastVariableName = name;
-    _lastVariableIndex = 0;
-
     if (_globalModules == null) return null;
 
     // We don't need to worry about multiple modules defining the same variable,
@@ -816,6 +815,20 @@ class Environment {
         forwarded: _forwardedModules);
   }
 
+  /// Returns a module with the same members and upstream modules as [this], but
+  /// an empty stylesheet and extender.
+  ///
+  /// This is used when resolving imports, since they need to inject forwarded
+  /// members into the current scope. It's the only situation in which a nested
+  /// environment can become a module.
+  Module<Callable> toDummyModule() {
+    return _EnvironmentModule(
+        this,
+        CssStylesheet(const [], SourceFile.decoded(const []).span(0)),
+        Extender.empty,
+        forwarded: _forwardedModules);
+  }
+
   /// Returns the module with the given [namespace], or throws a
   /// [SassScriptException] if none exists.
   Module<Callable> _getModule(String namespace) {

lib/src/visitor/async_evaluate.dart

@@ -1383,7 +1383,7 @@ class _EvaluateVisitor
       }
 
       List<ModifiableCssNode> children;
-      var environment = _environment.global();
+      var environment = _environment.forImport();
       await _withEnvironment(environment, () async {
         var oldImporter = _importer;
         var oldStylesheet = _stylesheet;
@@ -1420,8 +1420,7 @@ class _EvaluateVisitor
       // Create a dummy module with empty CSS and no extensions to make forwarded
       // members available in the current import context and to combine all the
       // CSS from modules used by [stylesheet].
-      var module = environment.toModule(
-          CssStylesheet(const [], stylesheet.span), Extender.empty);
+      var module = environment.toDummyModule();
       _environment.importForwards(module);
 
       if (module.transitivelyContainsCss) {
@@ -2803,7 +2802,8 @@ class _EvaluateVisitor
     if (!_sourceMap) return null;
     if (expression is VariableExpression) {
       return _environment.getVariableNode(expression.name,
-          namespace: expression.namespace);
+              namespace: expression.namespace) ??
+          expression;
     } else {
       return expression;
     }

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: eb095e782e2983223945d189caadc649b081a676
+// Checksum: 0ad7cedc5ee1d05297c4a8d321d492854742e8ab
 //
 // ignore_for_file: unused_import
 
@@ -1380,7 +1380,7 @@ class _EvaluateVisitor
       }
 
       List<ModifiableCssNode> children;
-      var environment = _environment.global();
+      var environment = _environment.forImport();
       _withEnvironment(environment, () {
         var oldImporter = _importer;
         var oldStylesheet = _stylesheet;
@@ -1417,8 +1417,7 @@ class _EvaluateVisitor
       // Create a dummy module with empty CSS and no extensions to make forwarded
       // members available in the current import context and to combine all the
       // CSS from modules used by [stylesheet].
-      var module = environment.toModule(
-          CssStylesheet(const [], stylesheet.span), Extender.empty);
+      var module = environment.toDummyModule();
       _environment.importForwards(module);
 
       if (module.transitivelyContainsCss) {
@@ -2778,7 +2777,8 @@ class _EvaluateVisitor
     if (!_sourceMap) return null;
     if (expression is VariableExpression) {
       return _environment.getVariableNode(expression.name,
-          namespace: expression.namespace);
+              namespace: expression.namespace) ??
+          expression;
     } else {
       return expression;
     }

pubspec.yaml

@@ -1,5 +1,5 @@
 name: sass
-version: 1.25.1-test.1
+version: 1.25.2-dev
 description: A Sass implementation in Dart.
 author: Sass Team
 homepage: https://github.com/sass/dart-sass

test/cli/shared.dart

@@ -538,4 +538,33 @@ void sharedTests(
       });
     });
   });
+
+  test("doesn't unassign variables", () async {
+    // This is a regression test for one of the strangest errors I've ever
+    // encountered. Every bit of what's going on was necessary to reproduce it,
+    // *including* running with source maps enabled, which is why it's here and
+    // not in sass-spec.
+    await d.file("input.scss", "a {@import 'downstream'}").create();
+    await d.file("_downstream.scss", r"""
+      @import 'midstream';
+
+      $b: $c;
+      @mixin d($_) {}
+      @include d($b);
+      e {f: $b}
+    """).create();
+    await d.file("_midstream.scss", "@forward 'upstream'").create();
+    await d.file("_upstream.scss", r"$c: g").create();
+
+    var sass = await runSass(["input.scss", "output.css"]);
+    await sass.shouldExit(0);
+
+    await d.file("output.css", equalsIgnoringWhitespace("""
+      a e {
+        f: g;
+      }
+
+      /*# sourceMappingURL=output.css.map */
+    """)).validate();
+  });
 }