Support for @mixin, @include and @extend

pkg/csslib/lib/parser.dart

Index: pkg/csslib/lib/parser.dart
diff --git a/pkg/csslib/lib/parser.dart b/pkg/csslib/lib/parser.dart
index 9f7b3ff1de6992ca3436d664e45a148d9afb8a5d..e050350c55f98d5f35c5d2317f08b5d7998bfded 100644
--- a/pkg/csslib/lib/parser.dart
+++ b/pkg/csslib/lib/parser.dart
@@ -429,17 +429,23 @@ class Parser {
     }
   }
 
-  //  Directive grammar:
-  //
-  //  import:             '@import' [string | URI] media_list?
-  //  media:              '@media' media_query_list '{' ruleset '}'
-  //  page:               '@page' [':' IDENT]? '{' declarations '}'
-  //  stylet:             '@stylet' IDENT '{' ruleset '}'
-  //  media_query_list:   IDENT [',' IDENT]
-  //  keyframes:          '@-webkit-keyframes ...' (see grammar below).
-  //  font_face:          '@font-face' '{' declarations '}'
-  //  namespace:          '@namespace name url("xmlns")
-  //  host:               '@host '{' ruleset '}'
+  /**
+   * Directive grammar:
+   *
+   *  import:             '@import' [string | URI] media_list?
+   *  media:              '@media' media_query_list '{' ruleset '}'
+   *  page:               '@page' [':' IDENT]? '{' declarations '}'
+   *  stylet:             '@stylet' IDENT '{' ruleset '}'
+   *  media_query_list:   IDENT [',' IDENT]
+   *  keyframes:          '@-webkit-keyframes ...' (see grammar below).
+   *  font_face:          '@font-face' '{' declarations '}'
+   *  namespace:          '@namespace name url("xmlns")
+   *  host:               '@host '{' ruleset '}'
+   *  mixin:              '@mixin name [(args,...)] '{' declarations/ruleset '}'
+   *  include:            '@include name [(@arg,@arg1)]
+   *                      '@include name [(@arg...)]
+   *  content             '@content'
+   */
   processDirective() {

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

Why is this (as well as many other productions) public?

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

The parser should be a hidden class (_Parser).
On 2013/09/18 22:40:54, nweiz wrote:

     int start = _peekToken.start;
 
@@ -748,9 +754,129 @@ class Parser {
 
         return new NamespaceDirective(prefix != null ? prefix.name : '',
             namespaceUri, _makeSpan(start));
+
+      case TokenKind.DIRECTIVE_MIXIN:
+        /* Stylet grammar:

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

"Stylet" -> "Mixin"

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

Done.

+         *
+         *  @mixin IDENT [(args,...)] '{'
+         *    ruleset | properties

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

This should be "[ruleset | property | directive]*", or if you want to be more
correct "[ruleset | property | directive] (';' [ruleset | property |
directive])*" with the caveat that the semicolon isn't necessary if the
preceding production has a brace-delimited block.

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

Done.

+         *  '}'
+         */
+        _next();
+
+        var name;
+        if (_peekIdentifier()) {

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

is identifier optional? it doesn't appear so from the grammar comment above

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

It's not optional, it's required - changed.

On 2013/09/18 06:44:15, John Messerly wrote:

+          name = identifier();
+        }

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

TODO here about parsing the args?

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

Done.

+
+        _eat(TokenKind.LBRACE);
+
+        List<TreeNode> productions = [];
+        List<TreeNode> declarations = [];
+        var mixinDirective;
+
+        start = _peekToken.start;
+        while (!_maybeEat(TokenKind.END_OF_FILE)) {
+          var directive = processDirective();

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

is it possible to reuse any existing parser code here?

from the grammar snippet I was expecting something a bit simpler.

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

Was able to simplify a bit but this is really handling special cases for mixins
both declarations and top-level stuff which are very different.  The parsing of
the decl vs ruleset is of course re-used code.

On 2013/09/18 06:44:15, John Messerly wrote:

+          if (directive != null) {
+            productions.add(directive);
+            _maybeEat(TokenKind.SEMICOLON);

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

Why are you using [_maybeEat] here? Any number of successive semicolons should
be allowed between children of a directive, not just one. Also, the following is
invalid (due to the lack of a semicolon between the two sub-directives), where
it looks like it would be allowed by this code: "@mixin foo {@some-directive
@another-directive}".

The algorithm that Sass uses for this sort of parsing looks like this:

    while consume(';') or last child has child nodes:
      last child <- process block child

where a block child could be empty.

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

Okay, I fixed it.

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

+            continue;
+          }
+          var selectorGroup = _nestedSelector();
+          if (selectorGroup != null) {
+            // Nested selector so process as a ruleset.
+            var ruleset = processRuleSet(selectorGroup);
+            if (ruleset == null) {
+              mixinDirective = new MixinRulesetDirective(name.name, [], false,
+                  productions, _makeSpan(start));
+              break;

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

Why are you breaking here? Mixins can contain more than one ruleset, or rulesets
mixed in with other types of children.

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

Was able to eliminate the whole nestedSelector chunk of code - no longer
relevant (plus not used anyway).
On 2013/09/18 22:40:54, nweiz wrote:

+            }
+            productions.add(ruleset);
+          }
+
+          var declGroup = processDeclarations(checkBrace: false);
+
+          var decls = [];
+          if (declGroup.declarations.any((decl) {
+            return decl is Declaration && decl is! IncludeMixinAtDeclaration;
+            })) {

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

Style nit: if you're going to write this if statement like this, it should be
indented as so:

    if (declGroup.declarations.any((decl) {
          return decl is Declaration && decl is! IncludeMixinAtDeclaration;
        })) {

The baseline indentation for a continued expression is +4 spaces, and the body
of the closure should be +2 spaces in addition to that.

That said, it's probably cleaner and more readable if you move the expression
into a variable rather than using it inline in the if statement.

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

Ok, changed the indentation.
On 2013/09/18 22:40:54, nweiz wrote:

+            var newDecls = [];
+            productions.forEach((include) {
+              // If declGroup has items that are declarations then we assume
+              // this mixin is a declaration mixin not a top-level mixin.

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

This isn't a distinction that should exist.

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

Warnings are returned if a mixin has both decls and rulesets e.g.,

@mixin a {
  foo { color: red; }
  background-color: blue;
}

@include a;

gives a warning instead of magically losing the background-color.

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

+              if (include is IncludeDirective) {
+                newDecls.add(new IncludeMixinAtDeclaration(include, include.span));

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

long line

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

Done.

+              } else {
+                print("Error mixing of top-level vs declarations mixins");
+              }
+            });
+            declGroup.declarations.insertAll(0, newDecls);
+            productions = [];
+          } else {
+            // Declarations are just @includes make it a list of productions
+            // no a declaration group.

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

If I interpreted this sentence correctly, it should be "Declarations are just
@includes. Make it a list of productions, not a declaration group."

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

Done.

+            declGroup.declarations.forEach((decl) {

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

I would probably just use a normal for-in loop here

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

Done.

+              productions.add(decl is IncludeMixinAtDeclaration ?
+                  decl.include : decl);
+            });
+            declGroup.declarations.clear();
+          }
+
+          if (declGroup.declarations.isNotEmpty) {
+            if (productions.isEmpty) {
+              mixinDirective = new MixinDeclarationDirective(name.name, [],
+                  false, declGroup, _makeSpan(start));
+              break;
+            } else {
+              declGroup.declarations.forEach((decl) {
+                if (decl is IncludeMixinAtDeclaration) {

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

this looks similar to:

   productions.add(decl is IncludeMixinAtDeclaration ? decl.include : decl);

... above, maybe use the same style in both places?

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

Done.

+                  productions.add(decl.include);
+                } else {
+                  productions.add(decl);
+                }
+              });
+            }
+          } else {
+            mixinDirective = new MixinRulesetDirective(name.name, [], false,
+                productions, _makeSpan(start));
+            break;
+          }
+        }
+
+        _eat(TokenKind.RBRACE);
+
+        return mixinDirective;
+
+      case TokenKind.DIRECTIVE_INCLUDE:
+        return processInclude( _makeSpan(start));
+
+      case TokenKind.DIRECTIVE_CONTENT:
+        // TODO(terry): TBD

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

issue warning about not implemented feature?

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

Done.

     }
   }
 
+  IncludeDirective processInclude(Span span, [bool eatSemiColon = true]) {

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

for bools it's kind of nice to use named arguments like:

{bool eatSemicolon: true}

... a tad more readable at the call site

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

Done.

+    /* Stylet grammar:

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

"Stylet" -> "Include"

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

Done.

+    *
+     *  @include IDENT [(args,...)];
+     */
+    _next();
+
+    var name;
+    if (_peekIdentifier()) {

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

As in John's comment for @mixin, this shouldn't be optional.

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

Right fixed and handled parameters too.
On 2013/09/18 22:40:54, nweiz wrote:

+      name = identifier();
+    }
+
+    if (eatSemiColon) {
+      _maybeEat(TokenKind.SEMICOLON);
+    }
+
+    // TODO(terry): Handle parameters to mixin.
+
+    return new IncludeDirective(name.name, [], span);
+  }
+
   RuleSet processRuleSet([SelectorGroup selectorGroup]) {
     if (selectorGroup == null) {
       selectorGroup = processSelectorGroup();
@@ -809,7 +935,7 @@ class Parser {
     }
   }
 
-  processDeclarations({bool checkBrace: true}) {
+  DeclarationGroup processDeclarations({bool checkBrace: true}) {
     int start = _peekToken.start;
 
     if (checkBrace) _eat(TokenKind.LBRACE);
@@ -1411,6 +1537,11 @@ class Parser {
       Expressions exprs = processExpr();
 
       decl = new VarDefinition(definedName, exprs, _makeSpan(start));
+    } else if (_peekToken.kind == TokenKind.DIRECTIVE_INCLUDE) {
+      // @include mixinName in the declaration area.
+      var span = _makeSpan(start);
+      var include = processInclude(span, false);
+      decl = new IncludeMixinAtDeclaration(include, span);
     }
 
     return decl;