Skip to content
This repository has been archived by the owner on Aug 18, 2021. It is now read-only.

Prevent parseForESLint() behavior from changing after parse() is called #559

Merged
merged 6 commits into from
Dec 25, 2017
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions lib/analyze-scope.js
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,10 @@ class Referencer extends OriginalReferencer {
}

module.exports = function(ast, parserOptions) {
if (OriginalReferencer._babelEslintPatched) {
return escope.analyze(ast, parserOptions);
}

const options = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think escope.analyze cannot understand parserOptions. It needs this options.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but just passing in that options object causes some additional errors. I think this needs to use the same option-translation logic that parse-with-patch uses.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like the monkeypatching in parse-with-patch relies on a global eslintOptions object, which isn't being set properly here because the parse-with-patch function isn't being called. To fix it, I think it would be better to pass the options down directly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm investigating it as well. But I have to leave while an hour.

optimistic: false,
directive: false,
Expand Down
6 changes: 1 addition & 5 deletions lib/index.js
Original file line number Diff line number Diff line change
@@ -1,18 +1,14 @@
"use strict";

let patched = false;

exports.parse = function(code, options) {
patched = true;
return require("./parse-with-patch")(code, options);
};

exports.parseForESLint = function(code, options) {
if (!patched && options.eslintVisitorKeys && options.eslintScopeManager) {
if (options.eslintVisitorKeys && options.eslintScopeManager) {
return require("./parse-with-scope")(code, options);
}

patched = true;
return { ast: require("./parse-with-patch")(code, options) };
};

Expand Down
2 changes: 2 additions & 0 deletions lib/parse-with-patch.js
Original file line number Diff line number Diff line change
Expand Up @@ -352,6 +352,8 @@ function monkeypatch(modules) {
this.close(node);
}
};

referencer._babelEslintPatched = true;
}

module.exports = function(code, options) {
Expand Down
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,10 @@
"devDependencies": {
"babel-eslint": "^8.0.0",
"dedent": "^0.7.0",
"eslint": "^4.12.1",
"eslint": "^4.14.0",
"eslint-config-babel": "^7.0.1",
"eslint-plugin-flowtype": "^2.30.3",
"eslint-plugin-import": "^2.8.0",
"eslint-plugin-prettier": "^2.1.2",
"espree": "^3.4.0",
"husky": "^0.14.0",
Expand Down
27 changes: 27 additions & 0 deletions test/babel-eslint.js
Original file line number Diff line number Diff line change
Expand Up @@ -539,4 +539,31 @@ describe("Public API", () => {
babelEslint.parseNoPatch("foo", {})
);
});

/*
* This test ensures that the enhanced referencer does not get used if eslint-scope has already been
* monkeypatched, because this causes some correctness issues. For example, if the enhanced referencer
* is used after the original referencer is monkeypatched, type annotation references are counted twice.
*/
it("does not visit type annotations multiple times after monkeypatching and calling parseForESLint()", () => {
assertImplementsAST(
espree.parse("foo", { sourceType: "module" }),
babelEslint.parse("foo", {})
);
const parseResult = babelEslint.parseForESLint(
"type Foo = {}; function x(): Foo {}",
{
eslintVisitorKeys: true,
eslintScopeManager: true,
}
);
assert(parseResult.visitorKeys);
assert(parseResult.scopeManager);

const fooVariable = parseResult.scopeManager.getDeclaredVariables(
parseResult.ast.body[0]
)[0];

assert.strictEqual(fooVariable.references.length, 1);
});
});
11 changes: 11 additions & 0 deletions test/fixtures/eslint-plugin-import/.eslintrc.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
root: true

# babel-eslint
parser: ../../../lib/index.js

# use eslint-plugin-import
plugins:
- import
rules:
import/no-named-as-default: error
no-unused-vars: error
1 change: 1 addition & 0 deletions test/fixtures/eslint-plugin-import/a.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default function foo() { }
1 change: 1 addition & 0 deletions test/fixtures/eslint-plugin-import/b.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
import foo from './a.js';
4 changes: 4 additions & 0 deletions test/fixtures/eslint-plugin-import/c.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
// @flow
type Foo = {};

const FlowTypeButton = ({ }: Foo) => { };
14 changes: 14 additions & 0 deletions test/z_eslint-plugin-import.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
"use strict";

const eslint = require("eslint");

describe("https://github.com/babel/babel-eslint/issues/558", () => {
it("don't crash with eslint-plugin-import", () => {
const engine = new eslint.CLIEngine({ ignore: false });
engine.executeOnFiles([
"test/fixtures/eslint-plugin-import/a.js",
"test/fixtures/eslint-plugin-import/b.js",
"test/fixtures/eslint-plugin-import/c.js",
]);
});
});
Loading