Support for @mixin, @include and @extend

pkg/csslib/lib/src/analyzer.dart

Index: pkg/csslib/lib/src/analyzer.dart
diff --git a/pkg/csslib/lib/src/analyzer.dart b/pkg/csslib/lib/src/analyzer.dart
index 9e366a5fdfcc1b4d634dfc94512d5e809a9b5989..c16d9f07c551fbbb993be7aa08107e9794c1be41 100644
--- a/pkg/csslib/lib/src/analyzer.dart
+++ b/pkg/csslib/lib/src/analyzer.dart
@@ -23,16 +23,27 @@ part of csslib.parser;
 class Analyzer {
   final List<StyleSheet> _styleSheets;
   final Messages _messages;
+  MixinDefinitions mixinDefs;
   VarDefinitions varDefs;
 
   Analyzer(this._styleSheets, this._messages);
 
   void run() {
+    mixinDefs = new MixinDefinitions(_styleSheets);

[nweiz, 2013/09/18 22:40:54]

It's not correct to locate all mixin definitions before performing any
@includes. @mixin declarations aren't hoisted; they're only visible to styles
that come after them.

[terry, 2013/10/09 03:40:33]

Yeah, I liked the idea of hoisting like a class but it's not SASS so I've undone
this.
On 2013/09/18 22:40:54, nweiz wrote:

+
+    // Any mixin cycles?
+    var mixinCycles = findAllMixinCycles();
+    for (var cycle in mixinCycles) {
+      _messages.warning("mixin cycle detected @mixin ${cycle.def.name}",

[nweiz, 2013/09/18 22:40:54]

"Mixin cycle detected for @mixin ${cycle.def.name}."

Shouldn't this produce an error rather than a warning if a mixin cycle is
detected? Also, shouldn't the error message list every mixin in the cycle, not
just one of them?

[terry, 2013/10/09 03:40:33]

Without hoisting cycle detection is removed.
On 2013/09/18 22:40:54, nweiz wrote:

+          cycle.def.span);
+      // TODO(terry): What if no mixin definition for a @include an error?

[nweiz, 2013/09/18 22:40:54]

"What if there are no mixin definitions for an @include? An error?"

+    }
+
     varDefs = new VarDefinitions(_styleSheets);
 
-    // Any cycles?
-    var cycles = findAllCycles();
-    for (var cycle in cycles) {
+    // Any var cycles?
+    var varCycles = findAllVarCycles();

[nweiz, 2013/09/18 22:40:54]

Variables should be eagerly evaluated, so cycle detection shouldn't be needed.
If a variable refers to a variable that hasn't yet been assigned, that should
just produce a reference error.

[terry, 2013/10/09 03:40:33]

Yes also removed variable hoisting.
On 2013/09/18 22:40:54, nweiz wrote:

+    for (var cycle in varCycles) {
       _messages.warning("var cycle detected var-${cycle.definedName}",
           cycle.span);
       // TODO(terry): What if no var definition for a var usage an error?
@@ -42,7 +53,28 @@ class Analyzer {
 
     // Remove any var definition from the stylesheet that has a cycle.
     _styleSheets.forEach((styleSheet) =>
-        new RemoveVarDefinitions(cycles).visitStyleSheet(styleSheet));
+        new RemoveVarDefinitions(varCycles).visitStyleSheet(styleSheet));
+
+    // Expand top-level @include.
+    _styleSheets.forEach((styleSheet) =>
+        new ExpandTopLevelIncludes(mixinDefs, _messages)
+        ..visitStyleSheet(styleSheet));

[nweiz, 2013/09/18 22:40:54]

Running separate visitors for all of these seems inefficient and less clear than
doing all the transforms in one pass. As more features get added, are you
planning to add a separate visitor with its own pass over the AST for each of
them?

[terry, 2013/10/09 03:40:33]

Yes, for now that is the mode we've taken in the dwc and csslib for ease of
disabling and enabling a feature.  We plan on combining once we have a need from
a perf point of view.  But it's a good idea to add a TODO so people realize why
we've done this.

On 2013/09/18 22:40:54, nweiz wrote:

+
+    // Expand  @include in declarations.

[nweiz, 2013/09/18 22:40:54]

Nit: extra space before "@include".

[terry, 2013/10/09 03:40:33]

Done.

+    _styleSheets.forEach((styleSheet) =>
+        new ExpandDeclarationIncludes(styleSheet, mixinDefs, _messages)
+        ..visitStyleSheet(styleSheet));
+
+    // Any missing @mixins only check if everything is good up to this point.

[nweiz, 2013/09/18 22:40:54]

I don't understand what this comment means.

[terry, 2013/10/09 03:40:33]

Functionality removed when hoisting of mixin removed - was for references to
forward undefined mixins.
On 2013/09/18 22:40:54, nweiz wrote:

+    if (_messages.messages.isEmpty) {
+      _styleSheets.forEach((styleSheet) =>
+          new IncludesMissingMixins(mixinDefs, this._messages)
+          ..visitStyleSheet(styleSheet));
+    }
+
+    // Remove all @mixin and @include
+    _styleSheets.forEach((styleSheet) =>
+      new RemoveMixinsAndIncludes()..visitStyleSheet(styleSheet));

[nweiz, 2013/09/18 22:40:54]

Style nit: indent +2 spaces.

[terry, 2013/10/09 03:40:33]

Code removed.
On 2013/09/18 22:40:54, nweiz wrote:

 
     // Expand any nested selectors using selector desendant combinator to
     // signal CSS inheritance notation.
@@ -51,7 +83,83 @@ class Analyzer {
         ..flatten(styleSheet));
   }
 
-  List<VarDefinition> findAllCycles() {
+  List<MixinDefinition> findAllMixinCycles() {
+    var cycles = [];
+
+    mixinDefs.map.values.forEach((value) {

[nweiz, 2013/09/18 22:40:54]

As John's mentioned elsewhere, use a for loop rather than [forEach] where
possible.

[terry, 2013/10/09 03:40:33]

Cycle code removed.
On 2013/09/18 22:40:54, nweiz wrote:

+      if (hasMixinCycle(value.includes)) cycles.add(value);
+    });
+
+    // Update our local list of known mixinDefinitions remove any mixins with a
+    // cycle. So the same mixin cycle isn't reported for each style sheet
+    // processed.
+    for (var cycle in cycles) {
+      mixinDefs.map.remove(cycle.def.name);
+    }
+
+    return cycles;
+  }
+
+  /**
+   * [includes] list of IncludeDirective or IncludeMixinAtDeclaration.

[nweiz, 2013/09/18 22:40:54]

If you're going to include a doc comment, it shouldn't just document a single
parameter.

Also, use complete sentences. "[includes] is a list of IncludeDirectives or
IncludeMixinAtDeclarations."

It's unclear to me what the difference is between these two constructs.

[terry, 2013/10/09 03:40:33]

Cycle code removed.
On 2013/09/18 22:40:54, nweiz wrote:

+   */
+  bool hasMixinCycle(List includes, {Set<String> visiting,
+      Set<String> visited}) {
+    if (visiting == null) visiting = new Set();
+    if (visited == null) visited = new Set();

[nweiz, 2013/09/18 22:40:54]

Explain the function of [visited] and how it differs from [visiting].

[terry, 2013/10/09 03:40:33]

cycle code removed.
On 2013/09/18 22:40:54, nweiz wrote:

+
+    bool cycleDetected = false;
+
+    var name;
+    for (var include in includes) {
+      if (include is IncludeMixinAtDeclaration) {
+        // Get to the IncludeDirective.
+        name = include.include.name;
+      } else {
+        name = include.name;
+      }
+
+      if (visiting.contains(name)) {
+        cycleDetected = true;
+        break;
+      }
+      if (!visited.contains(name)) {
+        visiting.add(name);
+        visited.add(name);
+      }
+      if (mixinDefs.map[name] != null) {
+        if (hasMixinCycle(mixinDefs.map[name].includes, visiting: visiting,
+            visited: visited)) {
+          cycleDetected = true;
+          break;
+        }
+      }
+
+
+
+/*

[Jennifer Messerly, 2013/09/18 06:44:15]

remove?

[terry, 2013/10/09 03:40:33]

entire function removed.
On 2013/09/18 06:44:15, John Messerly wrote:

+        for (var usage in mixinInclude(mixinDefs.map[name].includes)) {
+          if (hasMixinCycle(usage.name, visiting: visiting, visited: visited)) {
+            cycleDetected = true;
+            break;
+          }
+        }
+*/
+      visiting.remove(include);
+    }
+
+    return cycleDetected;
+  }
+
+  // TODO(terry): Only checks @includes in declarations not ruleset top-level.
+  Iterable<IncludeDirective> mixinInclude(List includes) =>
+      includes.where((e) => e is IncludeMixinAtDeclaration);
+
+  // =====================
+  // Var cycles functions:
+  // =====================
+
+  List<VarDefinition> findAllVarCycles() {
     var cycles = [];
 
     varDefs.map.values.forEach((value) {
@@ -511,3 +619,253 @@ class _MediaRulesReplacer extends Visitor {
     }
   }
 }
+
+class MixinDef {
+  MixinDefinition def;
+  List<IncludeDirective> includes;
+
+  MixinDef(this.def, this.includes);
+}
+
+/** Find all mixin definitions from a list of stylesheets. */
+class MixinDefinitions extends Visitor {
+  /** Map of variable name key to it's definition. */
+  final Map<String, MixinDef> map = new Map<String, MixinDef>();

[nweiz, 2013/09/18 22:40:54]

This type declaration is unnecessary. The type is obvious from the value you're
assigning.

[terry, 2013/10/09 03:40:33]

Yep but editor code completion didn't work w/o it but will remove.
On 2013/09/18 22:40:54, nweiz wrote:

+
+  var _includes = [];
+
+  MixinDefinitions(List<StyleSheet> styleSheets) {
+    for (var styleSheet in styleSheets) {
+      visitTree(styleSheet);
+    }
+  }
+
+  void visitIncludeDirective(IncludeDirective node) {
+    _includes.add(node);
+  }
+
+  void visitIncludeMixinAtDeclaration(IncludeMixinAtDeclaration node) {
+    _includes.add(node);
+  }
+
+  void visitMixinRulesetDirective(MixinRulesetDirective node) {
+    _includes = [];
+
+    super.visitMixinRulesetDirective(node);
+
+    // Replace with latest top-level mixin definition.
+    map[node.name] = new MixinDef(node, _includes);
+    _includes = [];
+  }
+
+  void visitMixinDeclarationDirective(MixinDeclarationDirective node) {
+    _includes = [];
+
+    super.visitMixinDeclarationDirective(node);
+
+    // Replace with latest mixin definition.
+    map[node.name] = new MixinDef(node, _includes);
+    _includes = [];
+  }
+}
+
+/**
+ * Expand all @include at the top-level the ruleset(s) associated with the
+ * mixin.
+ */
+class ExpandTopLevelIncludes extends Visitor {
+  StyleSheet _styleSheet;
+  final MixinDefinitions defs;
+  final Messages _messages;
+
+  ExpandTopLevelIncludes(this.defs, this._messages);
+
+  void visitStyleSheet(StyleSheet ss) {
+    _styleSheet = ss;
+    super.visitStyleSheet(ss);
+    _styleSheet = null;
+  }
+
+  void visitIncludeDirective(IncludeDirective node) {
+    if (defs.map.containsKey(node.name)) {
+      var mixinDef = defs.map[node.name].def;
+      if (mixinDef is MixinRulesetDirective) {
+        _TopLevelIncludeReplacer.replace(_styleSheet, node, mixinDef.rulesets);
+      }
+    }
+    super.visitIncludeDirective(node);
+  }
+}
+
+/** @include as a top-level with ruleset(s). */
+class _TopLevelIncludeReplacer extends Visitor {
+  final IncludeDirective _include;
+  final List<RuleSet> _newRules;
+  bool _foundAndReplaced = false;
+
+  /**
+   * Look for the [ruleSet] inside of an @media directive; if found then replace
+   * with the [newRules].  If [ruleSet] is found and replaced return true.
+   */
+  static bool replace(StyleSheet styleSheet, IncludeDirective include,

[nweiz, 2013/09/18 22:40:54]

You have three different ways of invoking a visitor in this file. For
[MixinDefinitions], you just construct it and it calls [visitStyleSheet]
internally. For [ExpandTopLevelIncludes], you construct it and then call
[visitStyleSheet] manually. And then for this class, you have a static method
that handles the construction and calling of [visitStyleSheet]. Make these
consistent with one another.

[terry, 2013/10/09 03:40:33]

Function was re-written this no longer exist.
On 2013/09/18 22:40:54, nweiz wrote:

+                      List<RuleSet>newRules) {
+    var visitor = new _TopLevelIncludeReplacer(include, newRules);
+    visitor.visitStyleSheet(styleSheet);
+    return visitor._foundAndReplaced;

[nweiz, 2013/09/18 22:40:54]

This value is never used.

[terry, 2013/10/09 03:40:33]

function re-written not an issue now.
On 2013/09/18 22:40:54, nweiz wrote:

+  }
+
+  _TopLevelIncludeReplacer(this._include, this._newRules);
+
+  visitStyleSheet(StyleSheet node) {
+    var index = node.topLevels.indexOf(_include);
+    if (index != -1) {
+      node.topLevels.insertAll(index + 1, _newRules);
+      node.topLevels.replaceRange(index, index + 1, [new NoOp()]);
+      _foundAndReplaced = true;
+    }
+    super.visitStyleSheet(node);
+  }
+
+  void visitMixinRulesetDirective(MixinRulesetDirective node) {
+    var index = node.rulesets.indexOf(_include as dynamic);

[nweiz, 2013/09/18 22:40:54]

What is "as dynamic" doing here?

[terry, 2013/10/09 03:40:33]

_include can be different either declarative vs/ ruleset @include.  Should
consider a better AST node to fix this issue.
On 2013/09/18 22:40:54, nweiz wrote:

+    if (index != -1) {
+      node.rulesets.insertAll(index + 1, _newRules);
+      node.rulesets.replaceRange(index, index + 1, [new NoOp()]);

[nweiz, 2013/09/18 22:40:54]

Why aren't you replacing the include with _newRules? It seems very weird to
replace it with a noop and then add the new rules afterwards.

That said, I still think it would be cleaner to use a single visitor for all
these replacements, and to have a more generic way for that visitor to replace a
node -- or better, non-destructively construct a new AST with different nodes.

[terry, 2013/10/09 03:40:33]

Only the resolve the @include once.

On 2013/09/18 22:40:54, nweiz wrote:

+      _foundAndReplaced = true;
+    }
+    super.visitMixinRulesetDirective(node);
+  }
+}
+
+/** Expand all @include inside of a declaration associated with a mixin. */
+class ExpandDeclarationIncludes extends Visitor {
+  final StyleSheet _styleSheet;
+  final MixinDefinitions defs;
+  final Messages _messages;
+
+  ExpandDeclarationIncludes(this._styleSheet, this.defs, this._messages);
+
+  bool _allIncludes(rulesets) =>
+      rulesets.every((rule) => rule is IncludeDirective || rule is NoOp);
+
+  void visitIncludeMixinAtDeclaration(IncludeMixinAtDeclaration node) {
+    if (defs.map.containsKey(node.include.name)) {

[nweiz, 2013/09/18 22:40:54]

Short-circuit if this is false to avoid extra nesting.

[terry, 2013/10/09 03:40:33]

Really, seems clunky to call super for the short-circuit and normal path...
On 2013/09/18 22:40:54, nweiz wrote:

+      var mixinDef = defs.map[node.include.name].def;
+      if (mixinDef is MixinDeclarationDirective) {
+        _IncludeReplacer.replace(_styleSheet, node,
+            mixinDef.declarations.declarations);
+      } else if (mixinDef is MixinRulesetDirective) {
+        // We're a list of @include(s) inside of a mixin ruleset if so then
+        // we'll convert to a list of IncludeMixinAtDeclaration(s).

[nweiz, 2013/09/18 22:40:54]

It doesn't make sense to say "We're X. If so, ...".

[terry, 2013/10/09 03:40:33]

yep, changed.
On 2013/09/18 22:40:54, nweiz wrote:

+        var orgRulesets = mixinDef.rulesets;

[nweiz, 2013/09/18 22:40:54]

What does "org" mean?

[terry, 2013/10/09 03:40:33]

original - missing 'i'  origRulesets
On 2013/09/18 22:40:54, nweiz wrote:

+        var rulesets = [];
+        if (orgRulesets.every((ruleset) => ruleset is IncludeDirective)) {
+          orgRulesets.forEach((ruleset) {
+            rulesets.add(new IncludeMixinAtDeclaration(ruleset, ruleset.span));
+          });
+          _IncludeReplacer.replace(_styleSheet, node, rulesets);
+        } else {
+          _messages.warning(
+              "Trying to use top-level ruleset in a declaration - "
+              "@include ${node.include.name}",
+              node.span);
+        }
+      }
+    }
+
+    super.visitIncludeMixinAtDeclaration(node);
+  }
+}
+
+/** @include as a top-level with ruleset(s). */
+class _IncludeReplacer extends Visitor {
+  final _include;
+  final List<Declaration> _newDeclarations;
+  bool _foundAndReplaced = false;
+
+  /**
+   * Look for the [ruleSet] inside of an @media directive; if found then replace

[nweiz, 2013/09/18 22:40:54]

"a @media"

[terry, 2013/10/09 03:40:33]

Done.

+   * with the [newRules].  If [ruleSet] is found and replaced return true.
+   */
+  static bool replace(StyleSheet ss, var include,
+      List<Declaration> newDeclarations) {
+    var visitor = new _IncludeReplacer(include, newDeclarations);
+    visitor.visitStyleSheet(ss);
+    return visitor._foundAndReplaced;

[nweiz, 2013/09/18 22:40:54]

This is also never used.

[terry, 2013/10/09 03:40:33]

Done.

+  }
+
+  _IncludeReplacer(this._include, this._newDeclarations);
+
+  void visitDeclarationGroup(DeclarationGroup node) {
+    var index = -1;
+    var i = 0;
+    for (var decl in node.declarations) {
+      if (decl == _include) {
+        index = i;
+        break;
+      } else if (decl is IncludeMixinAtDeclaration) {
+        if (decl.include == _include) {

[nweiz, 2013/09/18 22:40:54]

Use "&&" rather than nested if statements.

[terry, 2013/10/09 03:40:33]

While re-writing not an issue anymore.
On 2013/09/18 22:40:54, nweiz wrote:

+          index = i;
+          break;
+        }
+      }
+      i++;
+    }

[nweiz, 2013/09/18 22:40:54]

This i++/for/break stuff is really complicated and completely obscures your
intention. Add a utility function [indexWhere] that takes a list and a function
and returns the first index of an element in the iterable for which that
function returns true.

[terry, 2013/10/09 03:40:33]

ditto on re-write.On 2013/09/18 22:40:54, nweiz wrote:

+
+    if (index != -1) {

[nweiz, 2013/09/18 22:40:54]

Use null rather than -1 as a flag value.

[terry, 2013/10/09 03:40:33]

ditto re-wrote not an issue.
On 2013/09/18 22:40:54, nweiz wrote:

+      node.declarations.insertAll(index + 1, _newDeclarations);
+      // Change @include to NoOp so it's processed only once.
+      node.declarations.replaceRange(index, index + 1, [new NoOp()]);
+      _foundAndReplaced = true;
+    }
+    super.visitDeclarationGroup(node);
+  }
+}
+
+class IncludesMissingMixins extends Visitor {
+  final MixinDefinitions defs;
+  final Messages _messages;
+
+  IncludesMissingMixins(this.defs, this._messages);
+
+  void visitIncludeDirective(IncludeDirective node) {
+    if (!defs.map.containsKey(node.name)) {
+      // @mixin is undefined
+      _messages.warning("@mixin ${node.name} is undefined.", node.span);

[nweiz, 2013/09/18 22:40:54]

This should be an error.

[terry, 2013/10/09 03:40:33]

Class removed.
On 2013/09/18 22:40:54, nweiz wrote:

+    }
+
+    super.visitIncludeDirective(node);
+  }
+}
+
+/**
+ * Remove all @mixin and @include and any NoOp used as placeholder for @include.
+ */
+class RemoveMixinsAndIncludes extends Visitor {

[nweiz, 2013/09/18 22:40:54]

Don't the other visitors remove the mixins and includes? Why are you adding
noops if they're just going to be removed?

[terry, 2013/10/09 03:40:33]

class was eliminated when cycles removed.
On 2013/09/18 22:40:54, nweiz wrote:

+  RemoveMixinsAndIncludes();
+
+  bool _nodesToRemove(node) =>

[nweiz, 2013/09/18 22:40:54]

This name sounds like a variable, not a function. Maybe something like
[_shouldRemoveNode].

+      node is IncludeDirective || node is MixinDefinition || node is NoOp;
+
+  void visitStyleSheet(StyleSheet ss) {
+    var index = ss.topLevels.length;
+    while (--index >= 0) {

[nweiz, 2013/09/18 22:40:54]

It's never clearer to use "--variable" in an expression than to manually expand
it. Don't force your reader to remember the distinction between "x++" and "++x".

This particular case would also be much clearer if you just used
[List.removeWhere].

+      if (_nodesToRemove(ss.topLevels[index])) {
+        ss.topLevels.removeAt(index);
+      }
+    }
+
+    super.visitStyleSheet(ss);
+  }
+
+  void visitDeclarationGroup(DeclarationGroup node) {
+    var index = node.declarations.length;
+    while (--index >= 0) {
+      if (_nodesToRemove(node.declarations[index])) {
+        node.declarations.removeAt(index);
+      }
+    }
+
+    super.visitDeclarationGroup(node);
+  }
+}