-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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.
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
refactor: Move JS parsing logic into JS language #18448
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for docs-eslint canceled.
|
Sorry for the delay, I plan to review this this week. |
//----------------------------------------------------------------------------- | ||
|
||
const debug = createDebug("eslint:languages:js"); | ||
const DEFAULT_ECMA_VERSION = 5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to confirm: DEFAULT_ECMA_VERSION
set to 5, for eslintrc-compat?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct. The eslintrc branch of logic will still use this file, so want to cover our bases.
// We only need to do this for ESTree-compatible ASTs | ||
if (!this.isESTree) { | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be a breaking change because this code was executed for non-estree ASTs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. Without this, several tests failed, so I thought we were skipping logic. I'll take another look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I think I understand what's happening.
Those tests use fixture parsers that return undefined
or an invalid AST, and then new SourceCode()
crashes while trying to process it.
They were passing before these changes because new SourceCode()
was inside the same try-catch
as parser.parse()
, so when this happens, it ends up being treated as a parsing error - it produces a fatal lint error for the file, but doesn't crash linting.
But now, new SourceCode()
isn't in a try-catch, so when parser returns something we can't process, it crashes the whole linting. So I think the first question is: should we wrap language.createSourceCode()
in a try-catch to not crash the whole linting when parser returns an invalid result for a single file? That would retain the current behavior, though it can be argued that the linting should crash when the parser is malfunctioning (throwing an error when the passed code isn't parsable is expected behavior; returning undefined
or an invalid AST is not expected behavior).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good call. I think it makes sense to crash in this instance because it's not actually a parsing error. The parsing succeeded but for some reason creating the SourceCode
instance failed. I'd expect this to be caught by plugin developers before they release and most end-users would not end up seeing thing. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to crash in this instance because it's not actually a parsing error.
Makes sense to me 👍
But, related to this, I think we should update the failing tests to return valid parse results instead of updating parts of the code to avoid crashing on those tests (#18448 (comment) & #18448 (comment)).
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
@christian-bromann looks like there's a problem in WebdriverIO:
|
@mdjermanovic I will take a look tomorrow. |
@mdjermanovic please re-run the tests, it should pass now. |
@christian-bromann now the test fails with:
|
@mdjermanovic all test started to run extremely slow, I will investigate as to why. |
// augment global scope with declared global variables for ESTree only | ||
if (sourceCode.isESTree) { | ||
addDeclaredGlobals( | ||
sourceCode.scopeManager.scopes[0], | ||
configuredGlobals, | ||
{ exportedVariables: commentDirectives.exportedVariables, enabledGlobals: commentDirectives.enabledGlobals } | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to #18448 (comment).
I think we should rather update the tests that are failing without this change to return a valid AST.
addDeclaredGlobals(globalScope, configGlobals, inlineGlobals); | ||
/* | ||
* For languages other than regular JavaScript, it's possible that there | ||
* is no scope analysis done, and therefore, `globalScope` might be undefined. | ||
*/ | ||
if (globalScope) { | ||
addDeclaredGlobals(globalScope, configGlobals, inlineGlobals); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also similar to #18448 (comment), and I also think we should rather update the tests to return a valid AST.
(I left a few additional suggestions on how to update the tests)
const ast = { | ||
tokens: [], | ||
comments: [], | ||
loc: {}, | ||
range: [] | ||
}; | ||
const parseSpy = { parse: sinon.fake.returns(ast) }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const ast = { | |
tokens: [], | |
comments: [], | |
loc: {}, | |
range: [] | |
}; | |
const parseSpy = { parse: sinon.fake.returns(ast) }; | |
const parseSpy = { parse: sinon.spy(espree.parse) }; |
We could use espree
here to return a valid AST.
|
||
it("should have file path passed to it", () => { | ||
const code = "/* this is code */"; | ||
const parseSpy = { parse: sinon.spy() }; | ||
const parseSpy = { parse: sinon.fake.returns(ast) }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const parseSpy = { parse: sinon.fake.returns(ast) }; | |
const parseSpy = { parse: sinon.spy(espree.parse) }; |
Same here.
const ast = { | ||
tokens: [], | ||
comments: [], | ||
loc: {}, | ||
range: [] | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const ast = { | |
tokens: [], | |
comments: [], | |
loc: {}, | |
range: [] | |
}; |
Then, this wouldn't be needed.
return { | ||
comments: [], | ||
tokens: [], | ||
type: "Root", | ||
loc: {}, | ||
range: [] | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return { | |
comments: [], | |
tokens: [], | |
type: "Root", | |
loc: {}, | |
range: [] | |
}; | |
return espree.parse(text, parserOptions); |
We could use espree
to return a valid AST here as well (just need to add const espree = require("espree");
too).
@christian-bromann any update on these wdio failures? We've not had a test pass in several days and it's becoming an issue when reviewing PRs. |
ci failing has been fixed in the main branch, can you please rebase it, thanks! |
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[x] Add something to the core
[ ] Other, please explain:
What changes did you make? (Give an overview)
This PR moves JS parsing and config handling logic into a new JS language object (
/lib/languages/js/index.js
), and includes:VFile
class (implements theFile
interface from the RFC) and tests.Language
interface). This deviates slightly from the RFC in that I've renamed "options" in all places to "languageOptions" to avoid confusion. Otherwise, I was constantly assigning back and forth between these two names.language: "@/js"
to the default config.languageOptions
validation out of the default schema and into the JS language object.languageOptionsSchema
that does not validate the keys (that is now done in the JS language object) but preserves the merge behavior.Refs #16999
Is there anything you'd like reviewers to focus on?
I think these changes are backwards-compatible, but would like another set of eyes.
languageOptionsSchema
is the part I'm most concerned about. The previous merging behavior was very JS-specific, so I tried to come up with a generic version that would make sense for other languages. Please double-check that.