Repository: eslint/eslint (js)

Issue:
Store information for files with lint warnings and errors in cache
**The version of ESLint you are using.**
4.15.0

**The problem you want to solve.**
Make cache feature (and its corresponding documentation) more useful and intuitive. Currently, the cache feature does not seem as useful or intuitive as it might be in that only information for files that pass linting with no warnings or errors is saved in the cache. I wonder if the cache feature could offer even more of a performance benefit if the cache were to store information for files with lint warnings or errors as well such that they don't have to run through a linting process if they haven't been changed since they were last linted with the cache feature enabled?

**Your take on the correct solution to problem.**
Perhaps we can extend the usage of the cache such that information for files with lint warnings or errors is also saved to the cache? @IanVS, following up on [this comment](https://github.com/eslint/eslint/issues/2998#issuecomment-363171051)

Pull request:
Update: Cache files that failed linting (fixes #9948)
<!--
    ESLint adheres to the [JS Foundation Code of Conduct](https://js.foundation/community/code-of-conduct).
-->

**What is the purpose of this pull request? (put an "X" next to item)**

[ ] Documentation update
[ ] Bug fix ([template](https://raw.githubusercontent.com/eslint/eslint/master/templates/bug-report.md))
[ ] New rule ([template](https://raw.githubusercontent.com/eslint/eslint/master/templates/rule-proposal.md))
[ ] Changes an existing rule ([template](https://raw.githubusercontent.com/eslint/eslint/master/templates/rule-change-proposal.md))
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain: Updates `--cache` to store messages for files that failed linting, as well as those which succeeded in linting.

Fixes #9948.

<!--
    If the item you've checked above has a template, please paste the template questions below and answer them. (If this pull request is addressing an issue, you can just paste a link to the issue here instead.)
-->

<!--
    Please ensure your pull request is ready:

    - Read the pull request guide (https://eslint.org/docs/developer-guide/contributing/pull-requests)
    - Include tests for this change
    - Update documentation for this change (if appropriate)
-->

<!--
    The following is required for all pull requests:
-->

**What changes did you make? (Give an overview)**

Building on #10562, this changes the cache logic to cache files that failed linting as well as files that passed linting. The rationale is that, as long as the file contents or ESLint config (including ESLint version) haven't changed, we can assume the lint results will be the same even in the failure case.

**Is there anything you'd like reviewers to focus on?**

Is there sufficient test coverage?

Patch:
diff --git a/Makefile.js b/Makefile.js
index b99296156..9c48620ef 100644
--- a/Makefile.js
+++ b/Makefile.js
@@ -68,7 +68,7 @@ const NODE = "node ", // intentional extra space
     BUILD_DIR = "./build/",
     DOCS_DIR = "../eslint.github.io/docs",
     SITE_DIR = "../eslint.github.io/",
-    PERF_TMP_DIR = path.join(os.tmpdir(), "eslint", "performance"),
+    PERF_TMP_DIR = path.join(TEMP_DIR, "eslint", "performance"),
 
     // Utilities - intentional extra space at the end of each string
     MOCHA = `${NODE_MODULES}mocha/bin/_mocha `,
@@ -660,10 +660,10 @@ target.gensite = function(prereleaseVersion) {
 
         if (test("-f", fullPath)) {
 
-            rm("-r", fullPath);
+            rm("-rf", fullPath);
 
             if (filePath.indexOf(".md") >= 0 && test("-f", htmlFullPath)) {
-                rm("-r", htmlFullPath);
+                rm("-rf", htmlFullPath);
             }
         }
     });
@@ -801,7 +801,7 @@ target.gensite = function(prereleaseVersion) {
 
     // 12. Delete temporary directory
     echo("> Removing the temporary directory (Step 12)");
-    rm("-r", TEMP_DIR);
+    rm("-rf", TEMP_DIR);
 
     // 13. Update demos, but only for non-prereleases
     if (!prereleaseVersion) {
@@ -834,7 +834,7 @@ target.browserify = function() {
     cp("-r", "lib/*", TEMP_DIR);
 
     // 3. delete the load-rules.js file
-    rm(`${TEMP_DIR}load-rules.js`);
+    rm("-rf", `${TEMP_DIR}load-rules.js`);
 
     // 4. create new load-rule.js with hardcoded requires
     generateRulesIndex(TEMP_DIR);
@@ -849,7 +849,7 @@ target.browserify = function() {
     cat("./node_modules/babel-polyfill/dist/polyfill.js", `${TEMP_DIR}espree.js`, `${BUILD_DIR}eslint.js`).to(`${BUILD_DIR}eslint.js`);
 
     // 8. remove temp directory
-    rm("-r", TEMP_DIR);
+    rm("-rf", TEMP_DIR);
 };
 
 target.checkRuleFiles = function() {
diff --git a/lib/cli-engine.js b/lib/cli-engine.js
index 352a9ca3c..8442de2de 100644
--- a/lib/cli-engine.js
+++ b/lib/cli-engine.js
@@ -540,23 +540,14 @@ class CLIEngine {
 
         if (options.cache) {
             results.forEach(result => {
-                if (result.messages.length) {
-
-                    /*
-                     * if a file contains errors or warnings we don't want to
-                     * store the file in the cache so we can guarantee that
-                     * next execution will also operate on this file
-                     */
-                    lintResultCache.removeEntry(result.filePath);
-                } else {
-
-                    /*
-                     * since the file passed we store the result here
-                     * TODO: it might not be necessary to store the results list in the cache,
-                     * since it should always be 0 errors/warnings
-                     */
-                    lintResultCache.setCachedLintResults(result.filePath, result);
-                }
+
+                /*
+                 * Store the lint result in the LintResultCache.
+                 * NOTE: The LintResultCache will remove the file source and any
+                 * other properties that are difficult to serialize, and will
+                 * hydrate those properties back in on future lint runs.
+                 */
+                lintResultCache.setCachedLintResults(result.filePath, result);
             });
 
             // persist the cache to disk
diff --git a/lib/linter.js b/lib/linter.js
index faf7654cd..a6b5a5408 100644
--- a/lib/linter.js
+++ b/lib/linter.js
@@ -465,11 +465,10 @@ function resolveParserOptions(parserName, providedOptions, enabledEnvironments)
  * @returns {Object} The resolved globals object
  */
 function resolveGlobals(providedGlobals, enabledEnvironments) {
-    return Object.assign.apply(
-        null,
-        [{}]
-            .concat(enabledEnvironments.filter(env => env.globals).map(env => env.globals))
-            .concat(providedGlobals)
+    return Object.assign(
+        {},
+        ...enabledEnvironments.filter(env => env.globals).map(env => env.globals),
+        providedGlobals
     );
 }
 
diff --git a/lib/util/lint-result-cache.js b/lib/util/lint-result-cache.js
index a8f1c775f..29b432bd5 100644
--- a/lib/util/lint-result-cache.js
+++ b/lib/util/lint-result-cache.js
@@ -9,6 +9,7 @@
 //-----------------------------------------------------------------------------
 
 const assert = require("assert"),
+    fs = require("fs"),
     fileEntryCache = require("file-entry-cache"),
     hash = require("./hash"),
     pkg = require("../../package.json"),
@@ -89,21 +90,46 @@ class LintResultCache {
             return null;
         }
 
+        // If source is present but null, need to reread the file from the filesystem.
+        if (fileDescriptor.meta.results && fileDescriptor.meta.results.source === null) {
+            fileDescriptor.meta.results.source = fs.readFileSync(filePath, "utf-8");
+        }
+
         return fileDescriptor.meta.results;
     }
 
     /**
      * Set the cached lint results for a given file path, after removing any
      * information that will be both unnecessary and difficult to serialize.
+     * Avoids caching results with an "output" property (meaning fixes were
+     * applied), to prevent potentially incorrect results if fixes are not
+     * written to disk.
      * @param {string} filePath The file for which to set lint results.
      * @param {Object} result The lint result to be set for the file.
      * @returns {void}
      */
     setCachedLintResults(filePath, result) {
+        if (result && result.hasOwnProperty("output")) {
+            return;
+        }
+
         const fileDescriptor = this.fileEntryCache.getFileDescriptor(filePath);
 
-        if (!fileDescriptor.notFound) {
-            fileDescriptor.meta.results = result;
+        if (fileDescriptor && !fileDescriptor.notFound) {
+
+            // Serialize the result, except that we want to remove the file source if present.
+            const resultToSerialize = Object.assign({}, result);
+
+            /*
+             * Set result.source to null.
+             * In `getCachedLintResults`, if source is explicitly null, we will
+             * read the file from the filesystem to set the value again.
+             */
+            if (resultToSerialize.hasOwnProperty("source")) {
+                resultToSerialize.source = null;
+            }
+
+            fileDescriptor.meta.results = resultToSerialize;
             fileDescriptor.meta.hashOfConfig = hashOfConfigFor(this.configHelper, result.filePath);
         }
     }
@@ -115,15 +141,6 @@ class LintResultCache {
     reconcile() {
         this.fileEntryCache.reconcile();
     }
-
-    /**
-     * Remove a file entry from this lint result cache.
-     * @param {string} filePath The file path to be removed from the cache.
-     * @returns {void}
-     */
-    removeEntry(filePath) {
-        this.fileEntryCache.removeEntry(filePath);
-    }
 }
 
 module.exports = LintResultCache;
diff --git a/package.json b/package.json
index 846620061..253762553 100644
--- a/package.json
+++ b/package.json
@@ -51,7 +51,7 @@
     "functional-red-black-tree": "^1.0.1",
     "glob": "^7.1.2",
     "globals": "^11.7.0",
-    "ignore": "^3.3.3",
+    "ignore": "^4.0.2",
     "imurmurhash": "^0.1.4",
     "inquirer": "^5.2.0",
     "is-resolvable": "^1.1.0",
@@ -89,7 +89,7 @@
     "dateformat": "^3.0.3",
     "ejs": "^2.6.1",
     "eslint-plugin-eslint-plugin": "^1.2.0",
-    "eslint-plugin-node": "^6.0.1",
+    "eslint-plugin-node": "^7.0.1",
     "eslint-plugin-rulesdir": "^0.1.0",
     "eslint-release": "^0.11.1",
     "eslint-rule-composer": "^0.3.0",
diff --git a/packages/eslint-config-eslint/package.json b/packages/eslint-config-eslint/package.json
index 8267a3c50..e6e0bfe4e 100644
--- a/packages/eslint-config-eslint/package.json
+++ b/packages/eslint-config-eslint/package.json
@@ -23,7 +23,7 @@
     "js-yaml": "^3.5.1"
   },
   "peerDependencies": {
-    "eslint-plugin-node": "^6.0.1"
+    "eslint-plugin-node": "^6.0.1 || ^7.0.1"
   },
   "keywords": [
     "eslintconfig",


Test patch:
diff --git a/tests/fixtures/lint-result-cache/.eslintrc.json b/tests/fixtures/lint-result-cache/.eslintrc.json
index 6e5817ef6..8036a75af 100644
--- a/tests/fixtures/lint-result-cache/.eslintrc.json
+++ b/tests/fixtures/lint-result-cache/.eslintrc.json
@@ -1,5 +1,6 @@
 {
     "rules": {
+        "no-dupe-args": "error",
         "semi": ["error", "always"]
     },
     "root": true
diff --git a/tests/fixtures/lint-result-cache/test-with-errors.js b/tests/fixtures/lint-result-cache/test-with-errors.js
index 7f97cd8a0..38790aabe 100644
--- a/tests/fixtures/lint-result-cache/test-with-errors.js
+++ b/tests/fixtures/lint-result-cache/test-with-errors.js
@@ -1,1 +1,2 @@
 console.log("Hello, world!")
+function foo(a, a) {}
diff --git a/tests/lib/cli-engine.js b/tests/lib/cli-engine.js
index c4e0ab054..7643a241d 100644
--- a/tests/lib/cli-engine.js
+++ b/tests/lib/cli-engine.js
@@ -2132,7 +2132,7 @@ describe("CLIEngine", () => {
                 assert.isFalse(shell.test("-f", cacheFile), "the cache for eslint was deleted since last run did not used the cache");
             });
 
-            it("should not store in the cache a file that failed the test", () => {
+            it("should store in the cache a file that failed the test", () => {
 
                 const cacheFile = getFixturePath(".eslintcache");
 
@@ -2163,7 +2163,7 @@ describe("CLIEngine", () => {
 
                 assert.isTrue(typeof cache[goodFile] === "object", "the entry for the good file is in the cache");
 
-                assert.isTrue(typeof cache[badFile] === "undefined", "the entry for the bad file is not in the cache");
+                assert.isTrue(typeof cache[badFile] === "object", "the entry for the bad file is in the cache");
 
                 const cachedResult = engine.executeOnFiles([badFile, goodFile]);
 
@@ -2378,7 +2378,7 @@ describe("CLIEngine", () => {
 
                     assert.isTrue(typeof cache[goodFile] === "object", "the entry for the good file is in the cache");
 
-                    assert.isTrue(typeof cache[badFile] === "undefined", "the entry for the bad file is not in the cache");
+                    assert.isTrue(typeof cache[badFile] === "object", "the entry for the bad file is in the cache");
 
                     const cachedResult = engine.executeOnFiles([badFile, goodFile]);
 
diff --git a/tests/lib/util/lint-result-cache.js b/tests/lib/util/lint-result-cache.js
index c895918b3..3bb6382c7 100644
--- a/tests/lib/util/lint-result-cache.js
+++ b/tests/lib/util/lint-result-cache.js
@@ -28,7 +28,8 @@ describe("LintResultCache", () => {
         hashStub,
         sandbox,
         fakeConfigHelper,
-        fakeErrorResults;
+        fakeErrorResults,
+        fakeErrorResultsAutofix;
 
     before(() => {
         sandbox = sinon.sandbox.create();
@@ -39,14 +40,23 @@ describe("LintResultCache", () => {
 
         hashStub = sandbox.stub();
 
-        // Get lint results for test fixtures with and without errors
+        let shouldFix = false;
+
+        // Get lint results for test fixtures
         const cliEngine = new CLIEngine({
             cache: false,
-            globInputPaths: false
+            ignore: false,
+            globInputPaths: false,
+            fix: () => shouldFix
         });
 
+        // Get results without autofixing...
         fakeErrorResults = cliEngine.executeOnFiles([path.join(fixturePath, "test-with-errors.js")]).results[0];
 
+        // ...and with autofixing
+        shouldFix = true;
+        fakeErrorResultsAutofix = cliEngine.executeOnFiles([path.join(fixturePath, "test-with-errors.js")]).results[0];
+
         // Set up LintResultCache with fake fileEntryCache module
         LintResultCache = proxyquire("../../../lib/util/lint-result-cache", {
             "file-entry-cache": fileEntryCacheStubs,
@@ -105,7 +115,10 @@ describe("LintResultCache", () => {
         beforeEach(() => {
             cacheEntry = {
                 meta: {
-                    results: fakeErrorResults,
+
+                    // Serialized results will have null source
+                    results: Object.assign({}, fakeErrorResults, { source: null }),
+
                     hashOfConfig
                 }
             };
@@ -170,6 +183,7 @@ describe("LintResultCache", () => {
                 const result = lintResultsCache.getCachedLintResults(filePath);
 
                 assert.deepStrictEqual(result, fakeErrorResults);
+                assert.ok(result.source, "source property should be hydrated from filesystem");
             });
         });
     });
@@ -211,6 +225,15 @@ describe("LintResultCache", () => {
             lintResultsCache = new LintResultCache(cacheFileLocation, fakeConfigHelper);
         });
 
+        describe("When lint result has output property", () => {
+            it("does not modify file entry", () => {
+                lintResultsCache.setCachedLintResults(filePath, fakeErrorResultsAutofix);
+
+                assert.notProperty(cacheEntry.meta, "results");
+                assert.notProperty(cacheEntry.meta, "hashOfConfig");
+            });
+        });
+
         describe("When file is not found on filesystem", () => {
             beforeEach(() => {
                 cacheEntry.notFound = true;
@@ -225,53 +248,58 @@ describe("LintResultCache", () => {
         });
 
         describe("When file is found on filesystem", () => {
-            it("stores results and hash of config in file entry", () => {
+            beforeEach(() => {
                 lintResultsCache.setCachedLintResults(filePath, fakeErrorResults);
+            });
 
-                assert.deepStrictEqual(cacheEntry.meta.results, fakeErrorResults);
+            it("stores hash of config in file entry", () => {
                 assert.strictEqual(cacheEntry.meta.hashOfConfig, hashOfConfig);
             });
-        });
-    });
 
-    describe("reconcile", () => {
-        let reconcileStub,
-            lintResultsCache;
+            it("stores results (except source) in file entry", () => {
+                const expectedCachedResults = Object.assign(
+                    {},
+                    fakeErrorResults,
+                    { source: null }
+                );
 
-        before(() => {
-            reconcileStub = sandbox.stub();
-
-            fileEntryCacheStubs.create = () => ({
-                reconcile: reconcileStub
+                assert.deepStrictEqual(cacheEntry.meta.results, expectedCachedResults);
             });
         });
 
-        after(() => {
-            delete fileEntryCacheStubs.create;
-        });
+        describe("When file is found and empty", () => {
+            beforeEach(() => {
+                lintResultsCache.setCachedLintResults(
+                    filePath,
+                    Object.assign({}, fakeErrorResults, { source: "" })
+                );
+            });
 
-        beforeEach(() => {
-            lintResultsCache = new LintResultCache(cacheFileLocation, fakeConfigHelper);
-        });
+            it("stores hash of config in file entry", () => {
+                assert.strictEqual(cacheEntry.meta.hashOfConfig, hashOfConfig);
+            });
 
-        it("calls reconcile on the underlying cache", () => {
-            lintResultsCache.reconcile();
+            it("stores results (except source) in file entry", () => {
+                const expectedCachedResults = Object.assign(
+                    {},
+                    fakeErrorResults,
+                    { source: null }
+                );
 
-            assert.isTrue(reconcileStub.calledOnce);
+                assert.deepStrictEqual(cacheEntry.meta.results, expectedCachedResults);
+            });
         });
     });
 
-    describe("removeEntry", () => {
-        const filePath = path.join(fixturePath, "test-with-errors.js");
-
-        let removeEntryStub,
+    describe("reconcile", () => {
+        let reconcileStub,
             lintResultsCache;
 
         before(() => {
-            removeEntryStub = sandbox.stub();
+            reconcileStub = sandbox.stub();
 
             fileEntryCacheStubs.create = () => ({
-                removeEntry: removeEntryStub
+                reconcile: reconcileStub
             });
         });
 
@@ -283,11 +311,10 @@ describe("LintResultCache", () => {
             lintResultsCache = new LintResultCache(cacheFileLocation, fakeConfigHelper);
         });
 
-        it("calls removeEntry on the underlying cache", () => {
-            lintResultsCache.removeEntry(filePath);
+        it("calls reconcile on the underlying cache", () => {
+            lintResultsCache.reconcile();
 
-            assert.isTrue(removeEntryStub.calledOnce);
-            assert.isTrue(removeEntryStub.alwaysCalledWith(filePath));
+            assert.isTrue(reconcileStub.calledOnce);
         });
     });
 });