Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Breaking: align babel-eslint-parser & espree #11137

Merged
merged 4 commits into from Feb 20, 2020
Merged

Breaking: align babel-eslint-parser & espree #11137

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a9cff79
Chore: align babel-eslint-parser & espree
kaicataldo Feb 12, 2020
bbe2799
Start program at beginning of comment when no tokens exist
kaicataldo Feb 13, 2020
3b84c20
Import correct version of Espree for tests
kaicataldo Feb 13, 2020
e13db5d
Remove hasOwnProperty guard
kaicataldo Feb 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion eslint/babel-eslint-parser/package.json
View file
Open in desktop
Expand Up @@ -31,7 +31,6 @@
"@babel/core": "^7.2.0",
"@babel/eslint-shared-fixtures": "*",
"eslint": "^6.0.1",
"espree": "^6.0.0",
Copy link
Member Author

Choose a reason for hiding this comment

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

Thoughts on this? Seems like we should ensure we're using the version of Espree that's included with the version of ESLint we're testing against.

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 it should be a combo peer dep and dep, that way npm ls will fail (and npm install in npm 7+) when the peer dep isn't met.

Copy link
Member Author

Choose a reason for hiding this comment

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

@nicolo-ribaudo @JLHwung @existentialism Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Now that we use require.resolve to get ESLint's dep, this shouldn't be needed anymore.

"lodash.clonedeep": "^4.5.0"
}
}
26 changes: 13 additions & 13 deletions eslint/babel-eslint-parser/src/convert/convertAST.js
View file
Open in desktop
@@ -1,14 +1,10 @@
import { types as t, traverse } from "@babel/core";

function convertNodes(ast, code) {
const state = { source: code };
const astTransformVisitor = {
noScope: true,
enter(path) {
const node = path.node;

// private var to track original node type
node._babelType = node.type;
Copy link
Member

Choose a reason for hiding this comment

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

I was wondering why we had this in the first place, but a quick search on GitHub (search) shows that it's only used in snapshot tests output: it can be safely removed.

const { node } = path;

if (node.innerComments) {
delete node.innerComments;
Expand All @@ -23,7 +19,15 @@ function convertNodes(ast, code) {
}
},
exit(path) {
const node = path.node;
const { node } = path;

if (Object.hasOwnProperty.call(node, "extra")) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Seems unlikely ESLint would need this, but please let me know if I'm wrong. I also don't see this referenced anywhere in eslint-plugin-flowtype.

Copy link
Contributor

@JLHwung JLHwung Feb 19, 2020 •
edited

Choose a reason for hiding this comment

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

AFAIK node.extra is only used internally in babel-parser. It is a collection of per-node parser features, parenthesized, trailingComma, etc. So we can safely remove node.extra.

I think we can even remove the hasOwnProperty guard then.

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it, thanks!

delete node.extra;
}

if (node.loc && Object.hasOwnProperty.call(node.loc, "identifierName")) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Seems unlikely ESLint would need this, but please let me know if I'm wrong. I also don't see this referenced anywhere in eslint-plugin-flowtype.

delete node.loc.identifierName;
}

if (path.isTypeParameter()) {
node.type = "Identifier";
Expand Down Expand Up @@ -74,6 +78,7 @@ function convertNodes(ast, code) {
}
},
};
const state = { source: code };

// Monkey patch visitor keys in order to be able to traverse the estree nodes
t.VISITOR_KEYS.Property = t.VISITOR_KEYS.ObjectProperty;
Expand All @@ -95,19 +100,14 @@ function convertNodes(ast, code) {
function convertProgramNode(ast) {
ast.type = "Program";
ast.sourceType = ast.program.sourceType;
ast.directives = ast.program.directives;
ast.body = ast.program.body;
delete ast.program;
delete ast.errors;
Copy link
Member Author

Choose a reason for hiding this comment

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

Extra property that ESLint doesn't use (though we are planning to implement a top-level recoverableErrors property - you can find more info here).


if (ast.comments.length) {
const lastComment = ast.comments[ast.comments.length - 1];

if (!ast.tokens.length) {
// if no tokens, the program starts at the end of the last comment
Copy link
Member Author

Choose a reason for hiding this comment

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

This behavior doesn't match Espree's (it probably did at one point and has since changed).

ast.start = lastComment.end;
ast.loc.start.line = lastComment.loc.end.line;
ast.loc.start.column = lastComment.loc.end.column;
} else {
if (ast.tokens.length) {
const lastToken = ast.tokens[ast.tokens.length - 1];

if (lastComment.end > lastToken.end) {
Expand Down
3 changes: 1 addition & 2 deletions eslint/babel-eslint-parser/src/convert/convertComments.js
View file
Open in desktop
@@ -1,6 +1,5 @@
export default function convertComments(comments) {
for (let i = 0; i < comments.length; i++) {
const comment = comments[i];
for (const comment of comments) {
if (comment.type === "CommentBlock") {
comment.type = "Block";
} else if (comment.type === "CommentLine") {
Expand Down
40 changes: 0 additions & 40 deletions eslint/babel-eslint-parser/test/helpers/assert-implements-ast.js
View file
Open in desktop

This file was deleted.

134 changes: 85 additions & 49 deletions eslint/babel-eslint-parser/test/index.js
View file
Open in desktop
@@ -1,51 +1,85 @@
import assert from "assert";
import espree from "espree";
import path from "path";
import escope from "eslint-scope";
import unpad from "dedent";
import { parseForESLint } from "../src";
import assertImplementsAST from "./helpers/assert-implements-ast";

const babelOptions = {
const BABEL_OPTIONS = {
configFile: require.resolve(
"@babel/eslint-shared-fixtures/config/babel.config.js",
),
};
const ALLOWED_PROPERTIES = [
"importKind",
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'm assuming we want to leave importKind and exportKind since they might be useful for Flow-related ESLint rules. exportKind is already used in eslint-plugin-flowtype, so it seems like we should include both properties.

Copy link
Member

Choose a reason for hiding this comment

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

eslint-plugin-import definitely uses these.

Copy link
Member

Choose a reason for hiding this comment

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

We might want to add tests for third-party ESLint plugins in the e2e Circle CI workflow.

"exportKind",
"variance",
Copy link
Member Author

Choose a reason for hiding this comment

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

This is used in eslint-plugin-flowtype.

"typeArguments",
Copy link
Member Author

Choose a reason for hiding this comment

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

This is not used in eslint-plugin-flowtype. Should we remove this?

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 that we should keep it: if espree doesn't support flow, we don't need to align in this case.

];

function deeplyRemoveProperties(obj, props) {
for (const [k, v] of Object.entries(obj)) {
if (typeof v === "object") {
if (Array.isArray(v)) {
for (const el of v) {
if (el != null) deeplyRemoveProperties(el, props);
}
}

function parseAndAssertSame(code) {
code = unpad(code);
const esAST = espree.parse(code, {
ecmaFeatures: {
// enable JSX parsing
jsx: true,
// enable return in global scope
globalReturn: true,
// enable implied strict mode (if ecmaVersion >= 5)
impliedStrict: true,
// allow experimental object rest/spread
experimentalObjectRestSpread: true,
},
tokens: true,
loc: true,
range: true,
comment: true,
ecmaVersion: 2020,
sourceType: "module",
});
const babylonAST = parseForESLint(code, {
eslintVisitorKeys: true,
eslintScopeManager: true,
babelOptions,
}).ast;
assertImplementsAST(esAST, babylonAST);
if (props.includes(k)) delete obj[k];
else if (v != null) deeplyRemoveProperties(v, props);
continue;
}

if (props.includes(k)) delete obj[k];
}
}

describe("babylon-to-espree", () => {
describe("Babel and Espree", () => {
let espree;

function parseAndAssertSame(code) {
code = unpad(code);
const espreeAST = espree.parse(code, {
ecmaFeatures: {
// enable JSX parsing
jsx: true,
// enable return in global scope
globalReturn: true,
// enable implied strict mode (if ecmaVersion >= 5)
impliedStrict: true,
// allow experimental object rest/spread
experimentalObjectRestSpread: true,
},
tokens: true,
loc: true,
range: true,
comment: true,
ecmaVersion: 2020,
sourceType: "module",
});
const babelAST = parseForESLint(code, {
eslintVisitorKeys: true,
eslintScopeManager: true,
babelOptions: BABEL_OPTIONS,
}).ast;
deeplyRemoveProperties(babelAST, ALLOWED_PROPERTIES);
expect(babelAST).toEqual(espreeAST);
}

beforeAll(async () => {
// Use the version of Espree that is a dependency of
// the version of ESLint we are testing against.
const espreePath = require.resolve("espree", {
paths: [path.dirname(require.resolve("eslint"))],
});
espree = await import(espreePath);
});

describe("compatibility", () => {
it("should allow ast.analyze to be called without options", function() {
const esAST = parseForESLint("`test`", {
eslintScopeManager: true,
eslintVisitorKeys: true,
babelOptions,
babelOptions: BABEL_OPTIONS,
}).ast;
expect(() => {
escope.analyze(esAST);
Expand Down Expand Up @@ -244,9 +278,9 @@ describe("babylon-to-espree", () => {
const babylonAST = parseForESLint(code, {
eslintVisitorKeys: true,
eslintScopeManager: true,
babelOptions,
babelOptions: BABEL_OPTIONS,
}).ast;
assert.strictEqual(babylonAST.tokens[1].type, "Punctuator");
expect(babylonAST.tokens[1].type).toEqual("Punctuator");
});

// Espree doesn't support the nullish coalescing operator yet
Expand All @@ -255,9 +289,9 @@ describe("babylon-to-espree", () => {
const babylonAST = parseForESLint(code, {
eslintVisitorKeys: true,
eslintScopeManager: true,
babelOptions,
babelOptions: BABEL_OPTIONS,
}).ast;
assert.strictEqual(babylonAST.tokens[1].type, "Punctuator");
expect(babylonAST.tokens[1].type).toEqual("Punctuator");
});

// Espree doesn't support the pipeline operator yet
Expand All @@ -266,9 +300,9 @@ describe("babylon-to-espree", () => {
const babylonAST = parseForESLint(code, {
eslintVisitorKeys: true,
eslintScopeManager: true,
babelOptions,
babelOptions: BABEL_OPTIONS,
}).ast;
assert.strictEqual(babylonAST.tokens[1].type, "Punctuator");
expect(babylonAST.tokens[1].type).toEqual("Punctuator");
});

// Espree doesn't support private fields yet
Expand All @@ -277,19 +311,17 @@ describe("babylon-to-espree", () => {
const babylonAST = parseForESLint(code, {
eslintVisitorKeys: true,
eslintScopeManager: true,
babelOptions,
babelOptions: BABEL_OPTIONS,
}).ast;
assert.strictEqual(babylonAST.tokens[3].type, "Punctuator");
assert.strictEqual(babylonAST.tokens[3].value, "#");
expect(babylonAST.tokens[3].type).toEqual("Punctuator");
expect(babylonAST.tokens[3].value).toEqual("#");
});

// eslint-disable-next-line jest/no-disabled-tests
it.skip("empty program with line comment", () => {
it("empty program with line comment", () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is now aligned! 🎉

parseAndAssertSame("// single comment");
});

// eslint-disable-next-line jest/no-disabled-tests
it.skip("empty program with block comment", () => {
it("empty program with block comment", () => {
parseAndAssertSame(" /* multiline\n * comment\n*/");
});

Expand Down Expand Up @@ -448,19 +480,23 @@ describe("babylon-to-espree", () => {
});

it("do not allow import export everywhere", () => {
assert.throws(() => {
expect(() => {
parseAndAssertSame('function F() { import a from "a"; }');
}, /SyntaxError: 'import' and 'export' may only appear at the top level/);
}).toThrow(
new SyntaxError(
"'import' and 'export' may only appear at the top level",
),
);
});

it("return outside function", () => {
parseAndAssertSame("return;");
});

it("super outside method", () => {
assert.throws(() => {
expect(() => {
parseAndAssertSame("function F() { super(); }");
}, /SyntaxError: 'super' keyword outside a method/);
}).toThrow(new SyntaxError("'super' keyword outside a method"));
});

it("StringLiteral", () => {
Expand Down