Repository: shellscape/postcss-less (js)

Issue:
Incorrect value `rule.raws.semicolon`
<!--
  Please note: This template is not optional. Please fill in all fields and
  questions otherwise the issue may be closed. Please provide actual technical
  information about errors, if an error has occurred.
-->

* Node Version: `7.10.0`
* NPM Version: `4.2.0`
* postcss-less Version: `0.16.1`

If you have a large amount of code to share which demonstrates the problem you're experiencing, please provide a link to your
repository rather than pasting code. Otherwise, please paste relevant short snippets below.

### LESS

```less
a { .mixin; .mixin2; }
```

### JavaScript

```js
import postCssLess from 'postcss-less';
const less = `a { .mixin; .mixin2; }`;
const root = postCssLess.parse(less);
```

### Expected Behavior

`root.nodes[0].raws.semicolon` should be `true`.
`root.nodes[0].raws.after` should be ` `.

### Actual Behavior

`root.nodes[0].raws.semicolon` is `false`.
`root.nodes[0].raws.after` is `;  `.

### How can we reproduce the behavior?

Use example in `javascript` section.

Issue was created based on https://github.com/stylelint/stylelint/pull/2547.
Information about https://github.com/stylelint/stylelint/pull/2547#issuecomment-299712329.
Information about expected behavior based on `postcss` docs http://api.postcss.org/Node.html#raws


Pull request description:
Fix semicolons with Mixin, Extend Rules
**Which issue #** if any, does this resolve? fixes #70

<!-- PRs must be accompanied by related tests -->

Please check one:
- [x] New tests created for this change
- [x] Tests updated for this change

This PR:
- [ ] Adds new API
- [ ] Extends existing API, backwards-compatible
- [x] Introduces a breaking change
- [x] Fixes a bug

---

Commits in this PR will be part of a 1.0.0 release.

@evilebottnawi this should fix #70 and correct the `semicolon` and `after` raws properties. Please give this PR a test and let me know what you find.


Patch:
diff --git a/.editorconfig b/.editorconfig
index 0bd0fd3..8a09d85 100644
--- a/.editorconfig
+++ b/.editorconfig
@@ -1,21 +1,15 @@
-# EditorConfig helps developers define and maintain consistent
-# coding styles between different editors and IDEs
-# http://editorconfig.org
-
 root = true
 
 [*]
-# Don't modify any files by default (opt-in below)
-trim_trailing_whitespace = false
-insert_final_newline = false
-
-[{lib/,test/,./*}]
-# Change these settings to your own preference
 indent_style = space
-indent_size = 2
-
-# We recommend you to keep these unchanged
+indent_size = 4
 end_of_line = lf
 charset = utf-8
 trim_trailing_whitespace = true
 insert_final_newline = true
+
+[*.{json,yml}]
+indent_size = 2
+
+[*.md]
+trim_trailing_whitespace = false
diff --git a/README.md b/README.md
index 19ec306..9844a61 100644
--- a/README.md
+++ b/README.md
@@ -57,8 +57,11 @@ If you need to support inline comments, please use a [custom PostCSSLess stringi
 
 [PostCSS Rule Node](https://github.com/postcss/postcss/blob/master/docs/api.md#rule-node)
 
-### rule.ruleWithoutBody
-Determines whether or not a rule has a body, or content.
+### rule.empty
+
+Determines whether or not a rule contains declarations.
+
+_Note: Previously `ruleWithoutBody`. This is a breaking change from v0.16.0 to v1.0.0._
 
 ```js
 import postCssLess from 'postcss-less';
@@ -70,44 +73,81 @@ const less = `
 `;
 const root = postCssLess.parse(less);
 
-root.first.nodes[0].ruleWithoutBody // => true for &:extend
-root.first.nodes[1].ruleWithoutBody // => true for calling of mixin
+root.first.nodes[0].empty // => true for &:extend
+root.first.nodes[1].empty // => true for calling of mixin
 ```
-### rule.nodes
 
-An `Array` of child nodes.
+### rule.extend
+
+Determines whether or not a rule is [nested](http://lesscss.org/features/#extend-feature-extend-inside-ruleset).
 
-**Note** that rules without body don't have this property.
+_Note: Previously `extendRule`. This is a breaking change from v0.16.0 to v1.0.0._
 
 ```js
 import postCssLess from 'postcss-less';
 const less = `
     .class2 {
         &:extend(.class1);
-        .mixin-name(@param1, @param2);
     }
 `;
 const root = postCssLess.parse(less);
 
-root.first.nodes[0].nodes // => undefined for &:extend
-root.first.nodes[1].nodes // => undefined for mixin calling
+root.first.nodes[0].extend // => true
+```
+
+### rule.important
+
+Determines whether or not a rule is marked as [important](http://lesscss.org/features/#mixins-feature-the-important-keyword).
+
+_Note: This is a breaking change from v0.16.0 to v1.0.0._
+
+```js
+import postCssLess from 'postcss-less';
+const less = `
+    .class {
+        .mixin !important;
+    }
+`;
+const root = postCssLess.parse(less);
+
+root.first.nodes[0].important // => true
+root.first.nodes[0].selector // => '.mixin'
+```
+
+### rule.mixin
+
+Determines whether or not a rule is a [mixin](http://lesscss.org/features/#mixins-feature).
+
+```js
+import postCssLess from 'postcss-less';
+const less = `
+    .class2 {
+        .mixin-name;
+    }
+`;
+const root = postCssLess.parse(less);
+
+root.first.nodes[0].mixin // => true
 ```
 
-### rule.extendRule
+### rule.nodes
 
-Determines whether or not a rule is nested (eg.
- [extended](http://lesscss.org/features/#extend-feature-extend-inside-ruleset)).
+An `Array` of child nodes.
+
+**Note** that `nodes` is `undefined` for rules that don't contain declarations.
 
 ```js
 import postCssLess from 'postcss-less';
 const less = `
     .class2 {
         &:extend(.class1);
+        .mixin-name(@param1, @param2);
     }
 `;
 const root = postCssLess.parse(less);
 
-root.first.nodes[0].extendRule // => true
+root.first.nodes[0].nodes // => undefined for &:extend
+root.first.nodes[1].nodes // => undefined for mixin
 ```
 
 ## Comment Node
diff --git a/lib/less-parser.js b/lib/less-parser.js
index 06cea01..9125131 100755
--- a/lib/less-parser.js
+++ b/lib/less-parser.js
@@ -9,9 +9,6 @@ import lessTokenizer from './less-tokenize';
 const blockCommentEndPattern = /\*\/$/;
 
 export default class LessParser extends Parser {
-    tokenize () {
-        this.tokens = lessTokenizer(this.input);
-    }
 
     atrule (token) {
         if (token[1] === '@import') {
@@ -21,6 +18,118 @@ export default class LessParser extends Parser {
         }
     }
 
+    comment (token) {
+        const node = new Comment();
+        const content = token[1];
+        const text = content.slice(2).replace(blockCommentEndPattern, '');
+
+        this.init(node, token[2], token[3]);
+        node.source.end = {
+            line: token[4],
+            column: token[5]
+        };
+
+        node.raws.content = content;
+        node.raws.begin = content[0] + content[1];
+        node.inline = token[6] === 'inline';
+        node.block = !node.inline;
+
+        if (/^\s*$/.test(text)) {
+            node.text = '';
+            node.raws.left = text;
+            node.raws.right = '';
+        } else {
+            const match = text.match(/^(\s*)([^]*[^\s])(\s*)$/);
+
+            node.text = match[2];
+
+            // Add extra spaces to generate a comment in a common style /*[space][text][space]*/
+            node.raws.left = match[1] || ' ';
+            node.raws.right = match[3] || ' ';
+        }
+    }
+
+    /**
+     * @description Create a Declaration
+     * @param options {{start: number}}
+     */
+    createDeclaration (options) {
+        this.decl(this.tokens.slice(options.start, this.pos + 1));
+    }
+
+    /**
+     * @description Create a Rule node
+     * @param options {{start: number, params: Array}}
+     */
+    createRule (options) {
+
+        const semi = this.tokens[this.pos][0] === ';';
+        const end = this.pos + (options.empty && semi ? 2 : 1);
+        const tokens = this.tokens.slice(options.start, end);
+        const node = this.rule(tokens);
+
+        /**
+         * By default in PostCSS `Rule.params` is `undefined`.
+         * To preserve compability with PostCSS:
+         *  - Don't set empty params for a Rule.
+         *  - Set params for a Rule only if it can be a mixin or &:extend rule.
+         */
+        if (options.params[0] && (options.mixin || options.extend)) {
+            this.raw(node, 'params', options.params);
+        }
+
+        if (options.empty) {
+            // if it's an empty mixin or extend, it must have a semicolon
+            // (that's the only way we get to this point)
+            if (semi) {
+                node.raws.semicolon = this.semicolon = true;
+                node.selector = node.selector.replace(/;$/, '');
+            }
+
+            if (options.extend) {
+                node.extend = true;
+            }
+
+            if (options.mixin) {
+                node.mixin = true;
+            }
+
+            /**
+             * @description Mark mixin without declarations.
+             * @type {boolean}
+             */
+            node.empty = true;
+
+            // eslint-disable-next-line
+            delete this.current.nodes;
+
+            if (node.selector.indexOf('!important') >= 0) {
+                node.important = true;
+                node.selector = node.selector.replace(/\s!important/, '');
+            }
+
+            // rules don't have trailing semicolons in vanilla css, so they get
+            // added to this.spaces by the parser loop, so don't step back.
+            if (!semi) {
+                this.pos--;
+            }
+
+            this.end(this.tokens[this.pos]);
+        }
+    }
+
+    end (token) {
+        const node = this.current;
+
+        // if a Rule contains other Rules (mixins, extends) and those have
+        // semicolons, assert that the parent Rule has a semicolon
+        if (node.nodes && node.nodes.length && node.last.raws.semicolon && !node.last.nodes) {
+            this.semicolon = true;
+        }
+
+        super.end(token);
+    }
+
     import (token) {
         /* eslint complexity: 0 */
         let last = false;
@@ -97,130 +206,6 @@ export default class LessParser extends Parser {
         }
     }
 
-    rule (tokens) {
-        tokens.pop();
-
-        const node = new Rule();
-
-        this.init(node, tokens[0][2], tokens[0][3]);
-        node.raws.between = this.spacesFromEnd(tokens);
-        this.raw(node, 'selector', tokens);
-        this.current = node;
-    }
-
-    comment (token) {
-        const node = new Comment();
-        const content = token[1];
-        const text = content.slice(2).replace(blockCommentEndPattern, '');
-
-        this.init(node, token[2], token[3]);
-        node.source.end = {
-            line: token[4],
-            column: token[5]
-        };
-
-        node.raws.content = content;
-        node.raws.begin = content[0] + content[1];
-        node.inline = token[6] === 'inline';
-        node.block = !node.inline;
-
-        if (/^\s*$/.test(text)) {
-            node.text = '';
-            node.raws.left = text;
-            node.raws.right = '';
-        } else {
-            const match = text.match(/^(\s*)([^]*[^\s])(\s*)$/);
-
-            node.text = match[2];
-
-            // Add extra spaces to generate a comment in a common style /*[space][text][space]*/
-            node.raws.left = match[1] || ' ';
-            node.raws.right = match[3] || ' ';
-        }
-    }
-
-    /**
-     * @description Create a Rule node
-     * @param options {{start: number, params: Array}}
-     */
-    createRule (options) {
-        this.rule(this.tokens.slice(options.start, this.pos + 1));
-
-        /**
-         * By default in PostCSS `Rule.params` is `undefined`. There are rules to save the compability with PostCSS:
-         *  - Don't set empty params for Rule node.
-         *  - Set params fro Rule node only if it can be a mixin or &:extend rule.
-         */
-        if (options.params[0] && (options.isMixin || options.isExtendRule)) {
-            this.raw(this.current, 'params', options.params);
-        }
-    }
-
-    /**
-     * @description Create a Declaration
-     * @param options {{start: number}}
-     */
-    createDeclaration (options) {
-        this.decl(this.tokens.slice(options.start, this.pos + 1));
-    }
-
-    /**
-     * @description Create a Rule block and close it, because this mixin doesn't have a body
-     * @param options
-     */
-    ruleWithoutBody (options) {
-        this.createRule(options);
-
-        /**
-         * @description Mark mixin without body.
-         * @type {boolean}
-         */
-        this.current.ruleWithoutBody = true;
-
-        // remove `nodes` property from rules without body
-        // eslint-disable-next-line
-        delete this.current.nodes;
-        this.current.extendRule = this.current.selector.indexOf('&:extend') >= 0;
-        this.current.important = this.current.selector.indexOf('!important') >= 0;
-
-        this.pos--;
-
-        this.end(this.tokens[this.pos]);
-    }
-
-    /**
-     * @description
-     * @param options
-     * @returns {boolean}
-     */
-    processEndOfRule (options) {
-        const {start} = options;
-
-        if (options.isExtendRule || options.isMixin) {
-            this.ruleWithoutBody(options);
-            return true;
-        }
-
-        if (options.colon) {
-            if (options.isEndOfBlock) {
-                while (this.pos > start) {
-                    const token = this.tokens[this.pos][0];
-
-                    if (token !== 'space' && token !== 'comment') {
-                        break;
-                    }
-
-                    this.pos -= 1;
-                }
-            }
-
-            this.createDeclaration({start});
-            return true;
-        }
-
-        return false;
-    }
-
     /* eslint-disable max-statements, complexity */
     other () {
         let end = false;
@@ -235,8 +220,8 @@ export default class LessParser extends Parser {
             this.spaces += this.tokens[start][1];
             return;
         }
-        const isMixin = isMixinToken(this.tokens[start]);
-        const isExtendRule = Boolean(findExtendRule(this.tokens, start));
+        const mixin = isMixinToken(this.tokens[start]);
+        const extend = Boolean(findExtendRule(this.tokens, start));
         const params = [];
 
         this.pos += 1;
@@ -252,12 +237,12 @@ export default class LessParser extends Parser {
                 brackets.push(type === '(' ? ')' : ']');
             } else if (brackets.length === 0) {
                 if (type === ';') {
-                    const foundEndOfRule = this.processEndOfRule({
+                    const foundEndOfRule = this.ruleEnd({
                         start,
                         params,
                         colon,
-                        isMixin,
-                        isExtendRule
+                        mixin,
+                        extend
                     });
 
                     if (foundEndOfRule) {
@@ -266,7 +251,7 @@ export default class LessParser extends Parser {
 
                     break;
                 } else if (type === '{') {
-                    this.createRule({start, params, isMixin});
+                    this.createRule({start, params, mixin});
                     return;
                 } else if (type === '}') {
                     this.pos -= 1;
@@ -283,7 +268,7 @@ export default class LessParser extends Parser {
             }
 
             // we don't want to add params for pseudo-selectors that utilize parens (#56)
-            if ((isExtendRule || !colon) && (brackets.length > 0 || type === 'brackets' || params[0])) {
+            if ((extend || !colon) && (brackets.length > 0 || type === 'brackets' || params[0])) {
                 params.push(token);
             }
 
@@ -301,12 +286,12 @@ export default class LessParser extends Parser {
 
         // dont process an end of rule if there's only one token and it's unknown (#64)
         if (end && this.tokens.length > 1) {
-            const foundEndOfRule = this.processEndOfRule({
+            const foundEndOfRule = this.ruleEnd({
                 start,
                 params,
                 colon,
-                isMixin,
-                isExtendRule,
+                mixin,
+                extend,
                 isEndOfBlock: true
             });
 
@@ -318,5 +303,52 @@ export default class LessParser extends Parser {
         this.unknownWord(start);
     }
 
+    rule (tokens) {
+        tokens.pop();
+
+        const node = new Rule();
+
+        this.init(node, tokens[0][2], tokens[0][3]);
+
+        node.raws.between = this.spacesFromEnd(tokens);
+
+        this.raw(node, 'selector', tokens);
+        this.current = node;
+
+        return node;
+    }
+
+    ruleEnd (options) {
+        const {start} = options;
+
+        if (options.extend || options.mixin) {
+            this.createRule(Object.assign(options, {empty: true}));
+            return true;
+        }
+
+        if (options.colon) {
+            if (options.isEndOfBlock) {
+                while (this.pos > start) {
+                    const token = this.tokens[this.pos][0];
+
+                    if (token !== 'space' && token !== 'comment') {
+                        break;
+                    }
+
+                    this.pos -= 1;
+                }
+            }
+
+            this.createDeclaration({start});
+            return true;
+        }
+
+        return false;
+    }
+
+    tokenize () {
+        this.tokens = lessTokenizer(this.input);
+    }
+
     /* eslint-enable max-statements, complexity */
 }
diff --git a/lib/less-stringifier.js b/lib/less-stringifier.js
index 4ad7a7a..f39eff9 100755
--- a/lib/less-stringifier.js
+++ b/lib/less-stringifier.js
@@ -18,12 +18,26 @@ export default class LessStringifier extends Stringifier {
         }
     }
 
+    rule (node) {
+        super.rule(node);
+
+        if (node.empty && node.raws.semicolon) {
+            if (node.important) {
+                this.builder(' !important');
+            }
+
+            if (node.raws.semicolon) {
+                this.builder(';');
+            }
+        }
+    }
+
     block (node, start) {
-        const {ruleWithoutBody} = node;
+        const {empty} = node;
         const between = this.raw(node, 'between', 'beforeOpen');
         let after = '';
 
-        if (ruleWithoutBody) {
+        if (empty) {
             this.builder(start + between, node, 'start');
         } else {
             this.builder(`${ start + between }{`, node, 'start');
@@ -40,7 +54,7 @@ export default class LessStringifier extends Stringifier {
             this.builder(after);
         }
 
-        if (!ruleWithoutBody) {
+        if (!empty) {
             this.builder('}', node, 'end');
         }
     }


Test patch:
diff --git a/test/parser/extend.spec.js b/test/parser/extend.spec.js
index 43a7536..3fa1358 100644
--- a/test/parser/extend.spec.js
+++ b/test/parser/extend.spec.js
@@ -34,8 +34,8 @@ describe('Parser', () => {
             expect(root.first.selector).to.eql('.a');
             expect(root.first.first.selector).to.eql('&:extend(.bucket tr)');
             expect(root.first.first.params).to.eql('(.bucket tr)');
-            expect(root.first.first.extendRule).to.eql(true);
-            expect(root.first.first.toString()).to.be.eql('&:extend(.bucket tr)');
+            expect(root.first.first.extend).to.eql(true);
+            expect(root.first.first.toString()).to.be.eql('&:extend(.bucket tr);');
         });
 
         it('parses :extend() after selector', () => {
@@ -82,4 +82,4 @@ describe('Parser', () => {
             expect(root.nodes[1].selector).to.eql('.some-class:extend(@{variable})');
         });
     });
-});
\ No newline at end of file
+});
diff --git a/test/parser/mixins.spec.js b/test/parser/mixins.spec.js
index 3e367f9..2ac015d 100644
--- a/test/parser/mixins.spec.js
+++ b/test/parser/mixins.spec.js
@@ -26,9 +26,9 @@ describe('Parser', () => {
                 expect(root.first.type).to.eql('rule');
                 expect(root.first.selector).to.eql('.mixin-name (#FFF)');
                 expect(root.first.params).to.eql('(#FFF)');
-                expect(root.first.ruleWithoutBody).to.eql(true);
+                expect(root.first.empty).to.eql(true);
                 expect(root.first.nodes).to.be.an('undefined');
-                expect(root.first.toString()).to.be.eql('.mixin-name (#FFF)');
+                expect(root.first.toString()).to.be.eql('.mixin-name (#FFF);');
             });
 
             it('mixin without body #2', () => {
@@ -37,7 +37,7 @@ describe('Parser', () => {
                 expect(root.first.first.type).to.eql('rule');
                 expect(root.first.first.selector).to.eql('.mixin-name');
                 expect(root.first.params).to.be.an('undefined');
-                expect(root.first.first.ruleWithoutBody).to.eql(true);
+                expect(root.first.first.empty).to.eql(true);
                 expect(root.first.first.nodes).to.be.an('undefined');
             });
         });
@@ -122,7 +122,7 @@ describe('Parser', () => {
 
                 const root = parse(code);
 
-                expect(root.first.selector).to.eql('.foo() !important');
+                expect(root.first.selector).to.eql('.foo()');
                 expect(root.first.important).to.eql(true);
             });
 
@@ -134,7 +134,7 @@ describe('Parser', () => {
                 expect(root.first.selector).to.eql('header');
                 expect(root.first.first.selector).to.eql(ruleSet);
                 expect(root.first.first.params).to.eql(params, 'Mixin rule set. Invalid params');
-                expect(root.first.first.ruleWithoutBody).to.eql(true);
+                expect(root.first.first.empty).to.eql(true);
                 expect(root.first.first.nodes).to.be.an('undefined');
             });