Repository: eslint/eslint
Pull request #9363 (2017-09-27 20:27:58)

Chore: remove sourceCode property from Linter (refs #9161)
<!--
    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)**

[x] Other, please explain:

<!--
    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)**

This removes the `sourceCode` property from `Linter` instances, as part of the ongoing effort in #9161 to reduce undocumented API surface area.

The `sourceCode` property was used because `Linter` has a `getSourceCode` method which points to the `SourceCode` instance from the last `verify` call. This is unfortunate, because it prevents `Linter#verify` from being side-effect-free as described in #9161. However, we can't remove it right now, because it's documented [here](https://eslint.org/docs/developer-guide/nodejs-api#linterverify). Eventually I think we should deprecate and remove that, but that is a separate issue.

**Is there anything you'd like reviewers to focus on?**

Nothing in particular


eslintbot: LGTM

Patch:
diff --git a/README.md b/README.md
index 9ebf50389..2f0f1648e 100644
--- a/README.md
+++ b/README.md
@@ -171,7 +171,6 @@ ESLint follows [semantic versioning](http://semver.org). However, due to the nat
 * Major release (likely to break your lint build)
     * `eslint:recommended` is updated.
     * A new option to an existing rule that results in ESLint reporting more errors by default.
-    * An existing rule is removed.
     * An existing formatter is removed.
     * Part of the public API is removed or changed in an incompatible way.
 
diff --git a/docs/developer-guide/architecture.md b/docs/developer-guide/architecture.md
index 06403c20f..2c7ae4419 100644
--- a/docs/developer-guide/architecture.md
+++ b/docs/developer-guide/architecture.md
@@ -4,7 +4,10 @@ At a high level, there are a few key parts to ESLint:
 
 * `bin/eslint.js` - this is the file that actually gets executed with the command line utility. It's a dumb wrapper that does nothing more than bootstrap ESLint, passing the command line arguments to `cli`. This is intentionally small so as not to require heavy testing.
 * `lib/cli.js` - this is the heart of the ESLint CLI. It takes an array of arguments and then uses `eslint` to execute the commands. By keeping this as a separate utility, it allows others to effectively call ESLint from within another Node.js program as if it were done on the command line. The main call is `cli.execute()`. This is also the part that does all the file reading, directory traversing, input, and output.
-* `lib/eslint.js` - this is the core `eslint` object that does code verifying based on configuration options. This file does no file I/O and does not interact with the `console` at all. For other Node.js programs that have JavaScript text to verify, they would be able to use this interface directly.
+* `lib/linter.js` - this is the core Linter class that does code verifying based on configuration options. This file does no file I/O and does not interact with the `console` at all. For other Node.js programs that have JavaScript text to verify, they would be able to use this interface directly.
+* `lib/api.js` - this exposes an object that contains Linter, CLIEngine, RuleTester, and SourceCode.
+* `lib/testers/rule-tester.js` - this is a wrapper around Mocha, so that rules can be unit tested. This class lets us write consistently formatted tests for each rule that is implemented and be confident that each of the rules work. The RuleTester interface was modeled after Mocha and works with Mocha's global testing methods. RuleTester can also be modified to work with other testing frameworks.
+* `lib/util/source-code.js` - this contains a SourceCode class that is used to represent the parsed source code. It takes in source code and the Program node of the AST representing the code.
 
 ## The `cli` object
 
diff --git a/lib/formatters/html-template-page.html b/lib/formatters/html-template-page.html
index 887ca40e8..4016576fa 100644
--- a/lib/formatters/html-template-page.html
+++ b/lib/formatters/html-template-page.html
@@ -1,5 +1,7 @@
 <!DOCTYPE html>
+<html>
     <head>
+        <meta charset="UTF-8">
         <title>ESLint Report</title>
         <style>
             body {
diff --git a/lib/linter.js b/lib/linter.js
index a8fa58b9d..595ce77b8 100755
--- a/lib/linter.js
+++ b/lib/linter.js
@@ -9,8 +9,7 @@
 // Requirements
 //------------------------------------------------------------------------------
 
-const EventEmitter = require("events").EventEmitter,
-    eslintScope = require("eslint-scope"),
+const eslintScope = require("eslint-scope"),
     levn = require("levn"),
     lodash = require("lodash"),
     blankScriptAST = require("../conf/blank-script.json"),
@@ -20,6 +19,7 @@ const EventEmitter = require("events").EventEmitter,
     validator = require("./config/config-validator"),
     Environments = require("./config/environments"),
     applyDisableDirectives = require("./util/apply-disable-directives"),
+    createEmitter = require("./util/safe-emitter"),
     NodeEventGenerator = require("./util/node-event-generator"),
     SourceCode = require("./util/source-code"),
     Traverser = require("./util/traverser"),
@@ -697,6 +697,8 @@ const BASE_TRAVERSAL_CONTEXT = Object.freeze(
     )
 );
 
+const lastSourceCodes = new WeakMap();
+
 //------------------------------------------------------------------------------
 // Public Interface
 //------------------------------------------------------------------------------
@@ -708,7 +710,7 @@ const BASE_TRAVERSAL_CONTEXT = Object.freeze(
 module.exports = class Linter {
 
     constructor() {
-        this.sourceCode = null;
+        lastSourceCodes.set(this, null);
         this.version = pkg.version;
 
         this.rules = new Rules();
@@ -754,11 +756,11 @@ module.exports = class Linter {
         const filename = typeof providedFilename === "string" ? providedFilename : "<input>";
 
         if (typeof textOrSourceCode === "string") {
-            this.sourceCode = null;
+            lastSourceCodes.set(this, null);
             text = textOrSourceCode;
         } else {
-            this.sourceCode = textOrSourceCode;
-            text = this.sourceCode.text;
+            lastSourceCodes.set(this, textOrSourceCode);
+            text = textOrSourceCode.text;
         }
 
         // search and apply "eslint-env *".
@@ -777,13 +779,13 @@ module.exports = class Linter {
         // process initial config to make it safe to extend
         config = prepareConfig(config, this.environments);
 
-        if (this.sourceCode) {
+        if (lastSourceCodes.get(this)) {
             parserServices = {};
         } else {
 
             // there's no input, just exit here
             if (text.trim().length === 0) {
-                this.sourceCode = new SourceCode(text, blankScriptAST);
+                lastSourceCodes.set(this, new SourceCode(text, blankScriptAST));
                 return [];
             }
 
@@ -799,11 +801,11 @@ module.exports = class Linter {
             }
 
             parserServices = parseResult.services;
-            this.sourceCode = new SourceCode(text, parseResult.ast);
+            lastSourceCodes.set(this, new SourceCode(text, parseResult.ast));
         }
 
         const problems = [];
-        const sourceCode = this.sourceCode;
+        const sourceCode = lastSourceCodes.get(this);
         let disableDirectives;
 
         // parse global comments and modify config
@@ -817,7 +819,7 @@ module.exports = class Linter {
             disableDirectives = [];
         }
 
-        const emitter = new EventEmitter().setMaxListeners(Infinity);
+        const emitter = createEmitter();
         const traverser = new Traverser();
         const ecmaFeatures = config.parserOptions.ecmaFeatures || {};
         const ecmaVersion = config.parserOptions.ecmaVersion || 5;
@@ -860,7 +862,7 @@ module.exports = class Linter {
                      */
                     _linter: {
                         report() {},
-                        on: emitter.on.bind(emitter)
+                        on: emitter.on
                     }
                 }
             )
@@ -1005,7 +1007,7 @@ module.exports = class Linter {
      * @returns {SourceCode} The SourceCode object.
      */
     getSourceCode() {
-        return this.sourceCode;
+        return lastSourceCodes.get(this);
     }
 
     /**
diff --git a/lib/util/node-event-generator.js b/lib/util/node-event-generator.js
index 568a3b7fe..05b343ae9 100644
--- a/lib/util/node-event-generator.js
+++ b/lib/util/node-event-generator.js
@@ -194,7 +194,7 @@ const parseSelector = lodash.memoize(rawSelector => {
  *
  * ```ts
  * interface EventGenerator {
- *     emitter: EventEmitter;
+ *     emitter: SafeEmitter;
  *     enterNode(node: ASTNode): void;
  *     leaveNode(node: ASTNode): void;
  * }
@@ -203,8 +203,10 @@ const parseSelector = lodash.memoize(rawSelector => {
 class NodeEventGenerator {
 
     /**
-    * @param {EventEmitter} emitter - An event emitter which is the destination of events. This emitter must already
+    * @param {SafeEmitter} emitter
+    * An SafeEmitter which is the destination of events. This emitter must already
     * have registered listeners for all of the events that it needs to listen for.
+    * (See lib/util/safe-emitter.js for more details on `SafeEmitter`.)
     * @returns {NodeEventGenerator} new instance
     */
     constructor(emitter) {
@@ -215,23 +217,7 @@ class NodeEventGenerator {
         this.anyTypeEnterSelectors = [];
         this.anyTypeExitSelectors = [];
 
-        const eventNames = typeof emitter.eventNames === "function"
-
-            // Use the built-in eventNames() function if available (Node 6+)
-            ? emitter.eventNames()
-
-            /*
-             * Otherwise, use the private _events property.
-             * Using a private property isn't ideal here, but this seems to
-             * be the best way to get a list of event names without overriding
-             * addEventListener, which would hurt performance. This property
-             * is widely used and unlikely to be removed in a future version
-             * (see https://github.com/nodejs/node/issues/1817). Also, future
-             * node versions will have eventNames() anyway.
-             */
-            : Object.keys(emitter._events); // eslint-disable-line no-underscore-dangle
-
-        eventNames.forEach(rawSelector => {
+        emitter.eventNames().forEach(rawSelector => {
             const selector = parseSelector(rawSelector);
 
             if (selector.listenerTypes) {
diff --git a/lib/util/safe-emitter.js b/lib/util/safe-emitter.js
new file mode 100644
index 000000000..242cbe429
--- /dev/null
+++ b/lib/util/safe-emitter.js
@@ -0,0 +1,54 @@
+/**
+ * @fileoverview A variant of EventEmitter which does not give listeners information about each other
+ * @author Teddy Katz
+ */
+
+"use strict";
+
+//------------------------------------------------------------------------------
+// Typedefs
+//------------------------------------------------------------------------------
+
+/**
+ * An object describing an AST selector
+ * @typedef {Object} SafeEmitter
+ * @property {function(eventName: string, listenerFunc: Function): void} on Adds a listener for a given event name
+ * @property {function(eventName: string, arg1?: any, arg2?: any, arg3?: any)} emit Emits an event with a given name.
+ * This calls all the listeners that were listening for that name, with `arg1`, `arg2`, and `arg3` as arguments.
+ * @property {function(): string[]} eventNames Gets the list of event names that have registered listeners.
+ */
+
+/**
+ * Creates an object which can listen for and emit events.
+ * This is similar to the EventEmitter API in Node's standard library, but it has a few differences.
+ * The goal is to allow multiple modules to attach arbitrary listeners to the same emitter, without
+ * letting the modules know about each other at all.
+ * 1. It has no special keys like `error` and `newListener`, which would allow modules to detect when
+ * another module throws an error or registers a listener.
+ * 2. It calls listener functions without any `this` value. (`EventEmitter` calls listeners with a
+ * `this` value of the emitter instance, which would give listeners access to other listeners.)
+ * 3. Events can be emitted with at most 3 arguments. (For example: when using `emitter.emit('foo', a, b, c)`,
+ * the arguments `a`, `b`, and `c` will be passed to the listener functions.)
+ * @returns {SafeEmitter} An emitter
+ */
+module.exports = () => {
+    const listeners = Object.create(null);
+
+    return Object.freeze({
+        on(eventName, listener) {
+            if (eventName in listeners) {
+                listeners[eventName].push(listener);
+            } else {
+                listeners[eventName] = [listener];
+            }
+        },
+        emit(eventName, a, b, c) {
+            if (eventName in listeners) {
+                listeners[eventName].forEach(listener => listener(a, b, c));
+            }
+        },
+        eventNames() {
+            return Object.keys(listeners);
+        }
+    });
+};


Test patch:
diff --git a/tests/lib/code-path-analysis/code-path-analyzer.js b/tests/lib/code-path-analysis/code-path-analyzer.js
index 82384b157..e9878d667 100644
--- a/tests/lib/code-path-analysis/code-path-analyzer.js
+++ b/tests/lib/code-path-analysis/code-path-analyzer.js
@@ -10,11 +10,11 @@
 //------------------------------------------------------------------------------
 
 const assert = require("assert"),
-    EventEmitter = require("events").EventEmitter,
     fs = require("fs"),
     path = require("path"),
     Linter = require("../../../lib/linter"),
     EventGeneratorTester = require("../../../tools/internal-testers/event-generator-tester"),
+    createEmitter = require("../../../lib/util/safe-emitter"),
     debug = require("../../../lib/code-path-analysis/debug-helpers"),
     CodePath = require("../../../lib/code-path-analysis/code-path"),
     CodePathAnalyzer = require("../../../lib/code-path-analysis/code-path-analyzer"),
@@ -55,7 +55,7 @@ function getExpectedDotArrows(source) {
 
 describe("CodePathAnalyzer", () => {
     EventGeneratorTester.testEventGeneratorInterface(
-        new CodePathAnalyzer(new NodeEventGenerator(new EventEmitter()))
+        new CodePathAnalyzer(new NodeEventGenerator(createEmitter()))
     );
 
     describe("interface of code paths", () => {
diff --git a/tests/lib/linter.js b/tests/lib/linter.js
index eda0bd571..a6352c0f9 100644
--- a/tests/lib/linter.js
+++ b/tests/lib/linter.js
@@ -100,6 +100,23 @@ describe("Linter", () => {
                 linter.verify(code, config, filename, true);
             }, "Intentional error.");
         });
+
+        it("does not call rule listeners with a `this` value", () => {
+            const spy = sandbox.spy();
+
+            linter.defineRule("checker", () => ({ Program: spy }));
+            linter.verify("foo", { rules: { checker: "error" } });
+            assert(spy.calledOnce);
+            assert.strictEqual(spy.firstCall.thisValue, void 0);
+        });
+
+        it("does not allow listeners to use special EventEmitter values", () => {
+            const spy = sandbox.spy();
+
+            linter.defineRule("checker", () => ({ newListener: spy }));
+            linter.verify("foo", { rules: { checker: "error", "no-undef": "error" } });
+            assert(spy.notCalled);
+        });
     });
 
     describe("context.getSourceLines()", () => {
diff --git a/tests/lib/util/node-event-generator.js b/tests/lib/util/node-event-generator.js
index b59fabce7..67b6fa53a 100644
--- a/tests/lib/util/node-event-generator.js
+++ b/tests/lib/util/node-event-generator.js
@@ -9,11 +9,11 @@
 //------------------------------------------------------------------------------
 
 const assert = require("assert"),
-    EventEmitter = require("events").EventEmitter,
     sinon = require("sinon"),
     espree = require("espree"),
     estraverse = require("estraverse"),
     EventGeneratorTester = require("../../../tools/internal-testers/event-generator-tester"),
+    createEmitter = require("../../../lib/util/safe-emitter"),
     NodeEventGenerator = require("../../../lib/util/node-event-generator");
 
 //------------------------------------------------------------------------------
@@ -30,16 +30,16 @@ const ESPREE_CONFIG = {
 
 describe("NodeEventGenerator", () => {
     EventGeneratorTester.testEventGeneratorInterface(
-        new NodeEventGenerator(new EventEmitter())
+        new NodeEventGenerator(createEmitter())
     );
 
     describe("entering a single AST node", () => {
         let emitter, generator;
 
         beforeEach(() => {
-            emitter = new EventEmitter();
+            emitter = Object.create(createEmitter(), { emit: { value: sinon.spy() } });
+
             ["Foo", "Bar", "Foo > Bar", "Foo:exit"].forEach(selector => emitter.on(selector, () => {}));
-            emitter.emit = sinon.spy(emitter.emit);
             generator = new NodeEventGenerator(emitter);
         });
 
@@ -82,13 +82,15 @@ describe("NodeEventGenerator", () => {
          */
         function getEmissions(ast, possibleQueries) {
             const emissions = [];
-            const emitter = new EventEmitter();
+            const emitter = Object.create(createEmitter(), {
+                emit: {
+                    value: (selector, node) => emissions.push([selector, node])
+                }
+            });
 
             possibleQueries.forEach(query => emitter.on(query, () => {}));
             const generator = new NodeEventGenerator(emitter);
 
-            emitter.emit = (selector, node) => emissions.push([selector, node]);
-
             estraverse.traverse(ast, {
                 enter(node, parent) {
                     node.parent = parent;
@@ -308,7 +310,7 @@ describe("NodeEventGenerator", () => {
 
     describe("parsing an invalid selector", () => {
         it("throws a useful error", () => {
-            const emitter = new EventEmitter();
+            const emitter = createEmitter();
 
             emitter.on("Foo >", () => {});
             assert.throws(
diff --git a/tests/lib/util/safe-emitter.js b/tests/lib/util/safe-emitter.js
new file mode 100644
index 000000000..7b127ea89
--- /dev/null
+++ b/tests/lib/util/safe-emitter.js
@@ -0,0 +1,43 @@
+/**
+ * @fileoverview Tests for safe-emitter
+ * @author Teddy Katz
+ */
+
+"use strict";
+
+const createEmitter = require("../../../lib/util/safe-emitter");
+const assert = require("chai").assert;
+
+describe("safe-emitter", () => {
+    describe("emit() and on()", () => {
+        it("allows listeners to be registered calls them when emitted", () => {
+            const emitter = createEmitter();
+            const colors = [];
+
+            emitter.on("foo", () => colors.push("red"));
+            emitter.on("foo", () => colors.push("blue"));
+            emitter.on("bar", () => colors.push("green"));
+
+            emitter.emit("foo");
+            assert.deepEqual(colors, ["red", "blue"]);
+
+            emitter.on("bar", color => colors.push(color));
+            emitter.emit("bar", "yellow");
+
+            assert.deepEqual(colors, ["red", "blue", "green", "yellow"]);
+        });
+
+        it("calls listeners with no `this` value", () => {
+            const emitter = createEmitter();
+            let called = false;
+
+            emitter.on("foo", function() {
+                assert.strictEqual(this, void 0); // eslint-disable-line no-invalid-this
+                called = true;
+            });
+
+            emitter.emit("foo");
+            assert(called);
+        });
+    });
+});