Support solium-enable comment directive

CHANGELOG.md

@@ -2,6 +2,7 @@
 
 ## 1.2.3 ()
 - Added support for `solium-disable-previous-line` comment directive.
+- Added support for `solium-enable` comment directive. See [configuring with comments](https://ethlint.readthedocs.io/en/latest/user-guide.html#configuring-with-comments). This feature currently has a limitation which has been documented in [Known Issues](https://ethlint.readthedocs.io/en/latest/known-issues.html).
 
 ## 1.2.2 (2019-01-13)
 - Added support for parsing function declarations inside Inline Assembly blocks.

docs/known-issues.rst

@@ -2,12 +2,22 @@
 Known Issues
 ############
 
-While Solium is being actively maintained, a few major issues are still lurking around and we thought it best to make you aware of them so you don't spend time discovering them instead. We're working on resolving these issues so please be patient.
+While Solium is being actively maintained, a few major issues are still lurking around and we thought it best to make you aware of them so you don't spend time discovering them instead.
 
 - Solium is currently **file-aware instead of being project-aware**. What this means is that while linting, Solium doesn't have the context of all the contracts and how they may be using the contract currently being linted. A consequence of this is that the linter currently flags a state variable as unused if it doesn't find its usage in the same contract, whereas its clearly possible that you're ``import`` ing the contract elsewhere to use that variable (See `issue <https://github.com/duaraghav8/Solium/issues/11>`_). This is a fairly critical problem and will be resolved in a future release. We believe a codebase-aware linter would be much more powerful because of its broader context.
 
 - The linter's internal parser supports Solidity ``v0.5``. This means that it supports the `calldata <https://solidity.readthedocs.io/en/v0.5.2/types.html#data-location>`_ storage location specifier, but in a non-backward-compatible manner. If you're currently using Solidity version < 0.5 and have used ``calldata`` as a name for a variable or function parameter, you might see false lint issues because ``calldata`` is treated as location and hence, the variable name is seen as ``null``. Regardless of whether you use Solium or not, it is a good idea to rename all such variables to keep your code compatible with Solidity 0.5.
 
-If you discover any other pain points while using Solium, we encourage you to open up an issue.
+- There is a limitation when using the ``solium-enable`` comment directive: You cannot disable all rules (using ``// solium-disable`` for example) and then enable a select few (using ``// solium-enable rule1, rule2`` for example). The enabling part doesn't work and rules remain disabled even after using the ``enable`` directive. This is due to how the linter internally represents disabling **all** rules.
 
-Or if you don't feel like going through the formality of detailing the error(s), tap us with your problems on our `Gitter Channel <https://gitter.im/Solium-linter/Lobby#>`_.
+In the below example, the ``security/no-throw`` rule will **not** be enabled on the ``throw;`` statement, against the expectations.
+
+.. code-block:: javascript
+
+    contract Foo {
+        // solium-disable
+        function b11d() {
+            // solium-enable security/no-throw
+            throw;
+        }
+    }

docs/user-guide.rst

@@ -167,6 +167,28 @@ If you only use the directive, Solium disables all rules for the marked code. If
 		...
 	}
 
+- Disable linting over a section of code with ``solium-enable`` directive
+
+.. code-block:: javascript
+
+	/* solium-disable */
+
+	contract Foo {
+		...
+	}
+
+	/* solium-enable */
+
+	contract Bar {
+		...
+	}
+
+	// solium-disable security/no-throw, indentation
+	contract Baz {
+		throw;
+		// solium-enable security/no-throw
+	}
+
 
 .. index:: automatic code formatting
 

lib/comment-directive-parser.js

@@ -1,5 +1,5 @@
 /**
- * @fileoverview Analyzes the given comment token stream for solium configurtion directives
+ * @fileoverview Analyzes the given comment token stream for configuration directives
  * @author Raghav Dua <[email protected]>
  */
 
@@ -9,11 +9,6 @@
 const astUtils = require("./utils/ast-utils");
 
 /**
- * Parser can currently detect configuration to
- * - Disable linting on the next line
- * - Disable linting on the current line
- * - Disable linting on entire file
- * (all rules or specific ones (comma-separated))
  * NOTE: Constructor & Public functions of this class should have argument validations
  */
 class CommentDirectiveParser {
@@ -38,7 +33,7 @@ class CommentDirectiveParser {
         // Need not ensure that line > this.lastLine. This func's purpose is only to determine whether
         // given rule is disabled on given line, regardless of whether line is within bounds or not.
         if (typeof ruleName !== "string" || ruleName.length < 1) {
-            throw new Error("Rule name should be a non-empty string string.");
+            throw new Error("Rule name should be a non-empty string.");
         }
 
         if (!Number.isInteger(line) || line < 1) {
@@ -75,9 +70,45 @@ class CommentDirectiveParser {
         this.lineConfigurations[line] = (this.lineConfigurations[line] || []).concat(rules);
     }
 
+    _removeRulesFromLineConfig(line, rules) {
+        if (rules === this.ALL_RULES) {
+            // delete disabled-rule configuration for this line, since all rules
+            // are to be enabled.
+            delete this.lineConfigurations[line];
+            return;
+        }
+
+        /**
+         * A special case arises when the user has disabled all rules
+         * and then wants to enable a select few. When all rules are disabled,
+         * the configuration for a line is set to `all`. This means there is no
+         * array of rules to delete the enabled ones from.
+         *
+         * Right now, this edge case is not accounted for. In the below example,
+         * `throw;` will not be reported, which goes against expectations.
+         *
+         * contract Foo {
+         *   // solium-disable
+         *   function b11d() {
+         *     // solium-enable security/no-throw
+         *     throw;
+         *   }
+         * }
+         *
+         * TODO: Account for this edge case.
+         */
+        if (this.lineConfigurations[line] === this.ALL_RULES) {
+            return;
+        }
+
+        // Delete rules from line's disabled-rule configuration list
+        this.lineConfigurations[line] = this.lineConfigurations[line].filter(lc => { return !rules.includes(lc); });
+    }
+
     _constructLineConfigurationFromComment(token) {
-        const SD = "solium-disable", SDL = `${SD}-line`,
-            SDNL = `${SD}-next-line`, SDPL = `${SD}-previous-line`, text = this._cleanCommentText(token.text);
+        const text = this._cleanCommentText(token.text);
+        const S = "solium", SD = `${S}-disable`, SDL = `${SD}-line`,
+            SDNL = `${SD}-next-line`, SDPL = `${SD}-previous-line`, SE = `${S}-enable`;
 
         // Important that we check for SDNL, SDPL & SDL first. If they exist, then includes() will return true for SD anyway.
         if (text.includes(SDL)) {
@@ -95,7 +126,7 @@ class CommentDirectiveParser {
             return this._addRulesToLineConfig(targetLine, this._parseRuleNames(text, SDPL));
         }
 
-        // Notice how we use getEndingLine() for SDNL & SD to ensure that in case of block
+        // Notice how we use getEndingLine() for below directives to ensure that in case of a block
         // comment directive spanning over multiple lines, the disabling starts after the comment ends.
         // (SDL should disable on line on which the comment starts, so getLine() is used for it)
         if (text.includes(SDNL)) {
@@ -106,9 +137,22 @@ class CommentDirectiveParser {
             const currLine = astUtils.getEndingLine(token), rulesToDisable = this._parseRuleNames(text, SD);
             const objContext = this;
 
-            // Set desired configuration for all lines below the SD comment directive
+            // Disable rules for all lines below the SD comment directive
+            this._toEndOfFile(
+                currLine + 1,
+                lineNum => { objContext._addRulesToLineConfig(lineNum, rulesToDisable); }
+            );
+        }
+
+        if (text.includes(SE)) {
+            const currLine = astUtils.getEndingLine(token), rulesToEnable = this._parseRuleNames(text, SE);
+            const objContext = this;
+
+            // Remove disabled-rule entries for all lines below the SE comment directive
             this._toEndOfFile(
-                currLine + 1, lineNum => { objContext._addRulesToLineConfig(lineNum, rulesToDisable); });
+                currLine + 1,
+                lineNum => { objContext._removeRulesFromLineConfig(lineNum, rulesToEnable); }
+            );
         }
 
         // If none of the above branches were executed, then the current comment is not a solium directive.

test/lib/comment-directive-parser.js

@@ -60,6 +60,7 @@ describe("comment-directive-parser", () => {
         cdp.isRuleEnabledOnLine.should.be.type("function");
         cdp._constructLineConfigurations.should.be.type("function");
         cdp._addRulesToLineConfig.should.be.type("function");
+        cdp._removeRulesFromLineConfig.should.type("function");
         cdp._constructLineConfigurationFromComment.should.be.type("function");
         cdp._cleanCommentText.should.be.type("function");
         cdp._parseRuleNames.should.be.type("function");
@@ -75,7 +76,7 @@ describe("comment-directive-parser", () => {
 
         badRuleNames.forEach(r => {
             cdp.isRuleEnabledOnLine.bind(
-                cdp, r, 1).should.throw("Rule name should be a non-empty string string.");
+                cdp, r, 1).should.throw("Rule name should be a non-empty string.");
         });
 
         badLineNumbers.forEach(l => {
@@ -130,7 +131,10 @@ describe("comment-directive-parser", () => {
             ["/* solium-disable-line foorule*/", " solium-disable-line foorule"],
             ["//   solium-disable-previous-line   ", "   solium-disable-previous-line   "],
             ["/*   solium-disable-previous-line\t*/", "   solium-disable-previous-line\t"],
-            ["/* solium-disable-previous-line security/no-throw, quotes */", " solium-disable-previous-line security/no-throw, quotes "]
+            ["/* solium-disable-previous-line security/no-throw, quotes */", " solium-disable-previous-line security/no-throw, quotes "],
+            ["//   solium-enable   ", "   solium-enable   "],
+            ["/*   solium-enable\t*/", "   solium-enable\t"],
+            ["/* solium-enable security/no-throw, quotes */", " solium-enable security/no-throw, quotes "]
         ];
 
         texts.forEach(([dirty, clean]) => {
@@ -155,7 +159,9 @@ describe("comment-directive-parser", () => {
             ["  \tsolium-disable-line  pragma-on-top,\t\tindentation", "pragma-on-top,indentation", "solium-disable-line"],
             ["  \tsolium-disable-line foorule", "foorule", "solium-disable-line"],
             ["\t\tsolium-disable-previous-line  \t ", "all", "solium-disable-previous-line"],
-            [" solium-disable-previous-line security/no-throw, quotes  \t ", "security/no-throw,quotes", "solium-disable-previous-line"]
+            [" solium-disable-previous-line security/no-throw, quotes  \t ", "security/no-throw,quotes", "solium-disable-previous-line"],
+            ["\t\tsolium-enable  \t ", "all", "solium-enable"],
+            [" solium-enable security/no-throw, quotes  \t ", "security/no-throw,quotes", "solium-enable"]
         ];
 
         ruleCodes.forEach(([text, expectedOutput, p2r]) => {

test/lib/solium.js

@@ -2191,4 +2191,117 @@ describe("Solium.lint() comment directives", () => {
         done();
     });
 
+    it("should respect solium-enable comment directive", done => {
+        const config = {
+            "rules": {
+                "no-empty-blocks": "error",
+                "security/no-throw": "error",
+                "mixedcase": "error"
+            }
+        };
+
+        const snippets = [
+            {
+                code: `
+                // solium-disable
+                function BAFF() {
+                    if (true) {}
+                    throw;
+                }
+                // solium-enable
+                function BAFF() {
+                    if (true) {}
+                    throw;
+                }
+                `,
+                expectedIssues: 3
+            },
+            {
+                code: `
+                // solium-disable mixedcase, security/no-throw
+                function BAFF() {
+                    if (true) {}
+                    throw;
+                }
+                // solium-enable mixedcase, security/no-throw
+                function BAFF() {
+                    if (true) {}
+                    throw;
+                }
+                `,
+                expectedIssues: 4
+            },
+            {
+                code: `
+                // solium-disable no-empty-blocks, security/no-throw
+                function BAFF() {
+                    if (true) {}
+                    throw;
+                }
+                // solium-enable security/no-throw
+                function BAFF() {
+                    if (true) {}
+                    throw;
+                }
+                `,
+                expectedIssues: 3
+            },
+            {
+                code: `
+                // solium-disable no-empty-blocks
+                function BAFF() {
+                    if (true) {}
+                    throw;
+                }
+                // solium-enable security/no-throw, no-empty-blocks, quotes
+                function BAFF() {
+                    if (true) {}
+                    throw;
+                }
+                `,
+                expectedIssues: 5
+            },
+            {
+                code: `
+                // solium-disable no-empty-blocks, mixedcase, security/no-throw
+                function BAFF() {
+                    if (true) {}
+                    throw;
+                }
+                // solium-enable
+                function BAFF() {
+                    if (true) {}
+                    throw;
+                }
+                `,
+                expectedIssues: 3
+            },
+
+            // This is the edge case not yet handled.
+            // Its expectedIssues value should be changed from 0 to 3 once it is handled.
+            {
+                code: `
+                // solium-disable
+                function BAFF() {
+                    if (true) {}
+                    throw;
+                }
+                // solium-enable no-empty-blocks, mixedcase, security/no-throw
+                function BAFF() {
+                    if (true) {}
+                    throw;
+                }
+                `,
+                expectedIssues: 0
+            }
+        ];
+
+        snippets.forEach(({code, expectedIssues}) => {
+            let errors = Solium.lint(wrappers.toContract(code), config);
+            errors.should.have.size(expectedIssues);
+        });
+
+        done();
+    });
+
 });