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.

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

Update: Improve parser integrations (fixes #8392) #8755

Merged
merged 26 commits into from
Dec 20, 2017
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
a8fa8dd
rewrite traverser
mysticatea Jun 17, 2017
6859007
add supports scope and visitorKeys of custorm parsers
mysticatea Jun 17, 2017
fbd7552
add tests
mysticatea Jun 17, 2017
1ed2730
Merge branch 'master' into improve-parser-integrations
mysticatea Aug 10, 2017
2b60557
scope → scopeManager in the result of `parser.parseForESLint`
mysticatea Aug 10, 2017
071be3a
keys → visitorKeys
mysticatea Aug 10, 2017
4d65435
move some functions to `create()` in no-unmodified-loop-condition
mysticatea Aug 10, 2017
b6542d3
check `this.sourceCode.ast`
mysticatea Aug 10, 2017
47a91ca
fix SourceCode parameter to one object
mysticatea Sep 8, 2017
caae710
Merge remote-tracking branch 'origin/master' into improve-parser-inte…
mysticatea Sep 8, 2017
61d14be
update for review
mysticatea Sep 8, 2017
9c56cfe
Merge branch 'master' into improve-parser-integrations-fixed-conflicts
not-an-aardvark Sep 13, 2017
335d792
Merge branch 'master' into improve-parser-integrations
mysticatea Nov 16, 2017
6bf95ef
update Traverser with eslint-visitor-keys
mysticatea Nov 17, 2017
351722a
tweak tests with eslint-visitor-keys
mysticatea Nov 17, 2017
a489ddc
add `eslintVisitorKeys` and `eslintScopeManager` to parserOptions
mysticatea Nov 17, 2017
0c3d626
update docs
mysticatea Nov 22, 2017
00fc689
fix list style
mysticatea Nov 22, 2017
f5d7be4
ensure `sourceCode.scopeManager` is set
mysticatea Nov 22, 2017
3757df3
Docs: update ScopeManager docs
mysticatea Dec 7, 2017
c1a7d9e
Merge branch 'master' into improve-parser-integrations
mysticatea Dec 7, 2017
530b082
update for review
mysticatea Dec 14, 2017
6bed81e
fix some links to escope docs
mysticatea Dec 14, 2017
a2e4a83
update for review
mysticatea Dec 18, 2017
c5ceadb
Merge remote-tracking branch 'origin/master' into improve-parser-inte…
mysticatea Dec 18, 2017
de2c9b0
upgrade eslint-visitor-keys to 1.0.0
mysticatea Dec 18, 2017
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
63 changes: 38 additions & 25 deletions lib/linter.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ const MAX_AUTOFIX_PASSES = 10;
* @property {ASTNode} ast The ESTree AST Program node.
* @property {Object} services An object containing additional services related
* to the parser.
* @property {ScopeManager|null} scope The scope of this AST.
* @property {Object|null} visitorKeys The visitor keys to traverse this AST.
Copy link
Member

Choose a reason for hiding this comment

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

Are these properties intended to be used outside of linter? If not, can we clarify that they're private (e.g. with linter._scope and linter._visitorKeys)?

Copy link
Member Author

Choose a reason for hiding this comment

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

linter does not have those properties.
This is typedef of the result of parser.parseForESLint method, so this is public.

*/

//------------------------------------------------------------------------------
Expand Down Expand Up @@ -763,9 +765,7 @@ class Linter extends EventEmitter {
*/
verify(textOrSourceCode, config, filenameOrOptions, saveState) {
const text = (typeof textOrSourceCode === "string") ? textOrSourceCode : null;
let ast,
parseResult,
allowInlineConfig;
let allowInlineConfig;

// evaluate arguments
if (typeof filenameOrOptions === "object") {
Expand Down Expand Up @@ -805,7 +805,7 @@ class Linter extends EventEmitter {
return this.messages;
}

parseResult = parse(
const parseResult = parse(
stripUnicodeBOM(text).replace(astUtils.SHEBANG_MATCHER, (match, captured) => `//${captured}`),
config,
this.currentFilename,
Expand All @@ -814,27 +814,35 @@ class Linter extends EventEmitter {

// if this result is from a parseForESLint() method, normalize
if (parseResult && parseResult.ast) {
ast = parseResult.ast;
} else {
ast = parseResult;
parseResult = null;
}

if (ast) {
this.sourceCode = new SourceCode(text, ast);
// Save all returned values of `parse()` because this source code object can be reused.
// This ensures the same result when reused.
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 confused about this comment -- it seems to imply that ASTs are cached, but I don't see them cached anywhere. Is there something I'm missing?

Copy link
Member Author

Choose a reason for hiding this comment

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

The first argument of linter#verify can be a SourceCode instance. In that case, parsing is skipped and the SourceCode instance is reused.

this.sourceCode = new SourceCode(
text,
parseResult.ast,
parseResult.services,
parseResult.scope,
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to call the property scopeManager instead of scope?

Copy link
Member

Choose a reason for hiding this comment

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

Before this PR is merged, we should add documentation for the new parser APIs. (However, maybe we should wait until we decide on the APIs before adding documentation.)

parseResult.visitorKeys
);
} else if (parseResult) {
this.sourceCode = new SourceCode(
text,
parseResult
);
} else {
this.sourceCode = null;
}

} else {
this.sourceCode = textOrSourceCode;
ast = this.sourceCode.ast;
}

// if espree failed to parse the file, there's no sense in setting up rules
if (ast) {
if (this.sourceCode) {
Copy link
Member

Choose a reason for hiding this comment

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

Should this include a check for this.sourceCode.ast to match the previous behaviour?


// parse global comments and modify config
if (allowInlineConfig !== false) {
config = modifyConfigsFromComments(this.currentFilename, ast, config, this);
config = modifyConfigsFromComments(this.currentFilename, this.sourceCode.ast, config, this);
}

// ensure that severities are normalized in the config
Expand Down Expand Up @@ -865,7 +873,7 @@ class Linter extends EventEmitter {
key, this, severity, options,
config.settings, config.parserOptions, config.parser,
ruleCreator.meta,
(parseResult && parseResult.services ? parseResult.services : {})
this.sourceCode.parserServices || {}
);

const rule = ruleCreator.create ? ruleCreator.create(ruleContext)
Expand All @@ -890,21 +898,25 @@ class Linter extends EventEmitter {

const ecmaFeatures = this.currentConfig.parserOptions.ecmaFeatures || {};
const ecmaVersion = this.currentConfig.parserOptions.ecmaVersion || 5;
const sourceType = this.currentConfig.parserOptions.sourceType || "script";
Copy link
Member

Choose a reason for hiding this comment

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

Can this be this.sourceCode.ast.sourceType instead? That will make it correct for custom parsers that don't default to "script".

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, that seems like a bug to me.

Anyway, it can be addressed in a different issue.


// gather scope data that may be needed by the rules
this.scopeManager = eslintScope.analyze(ast, {
ignoreEval: true,
nodejsScope: ecmaFeatures.globalReturn,
impliedStrict: ecmaFeatures.impliedStrict,
ecmaVersion,
sourceType: this.currentConfig.parserOptions.sourceType || "script",
fallback: Traverser.getKeys
});
this.scopeManager =
this.sourceCode.scopeManager ||
eslintScope.analyze(this.sourceCode.ast, {
ignoreEval: true,
nodejsScope: ecmaFeatures.globalReturn,
impliedStrict: ecmaFeatures.impliedStrict,
ecmaVersion,
sourceType,
fallback: Traverser.getKeys,
childVisitorKeys: this.sourceCode.visitorKeys || Traverser.DEFAULT_VISITOR_KEYS
});

this.currentScopes = this.scopeManager.scopes;

// augment global scope with declared global variables
addDeclaredGlobals(ast, this.currentScopes[0], this.currentConfig, this.environments);
addDeclaredGlobals(this.sourceCode.ast, this.currentScopes[0], this.currentConfig, this.environments);

let eventGenerator = new NodeEventGenerator(this);

Expand All @@ -916,7 +928,8 @@ class Linter extends EventEmitter {
* automatically be informed that this type of node has been found
* and react accordingly.
*/
this.traverser.traverse(ast, {
this.traverser.traverse(this.sourceCode.ast, {
keys: this.sourceCode.visitorKeys,
enter(node, parent) {
node.parent = parent;
eventGenerator.enterNode(node);
Expand Down
15 changes: 9 additions & 6 deletions lib/rules/no-unmodified-loop-condition.js
Original file line number Diff line number Diff line change
Expand Up @@ -111,13 +111,14 @@ const isInLoop = {
*
* @param {ASTNode} root - A node to check.
* This node is one of BinaryExpression or ConditionalExpression.
* @param {SourceCode} sourceCode - The source code object of this context.
* @returns {boolean} `true` if the node is dynamic.
*/
function hasDynamicExpressions(root) {
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: Could these functions be moved into the create method so they can reference sourceCode from a closure?

function hasDynamicExpressions(root, sourceCode) {
let retv = false;
const traverser = new Traverser();

traverser.traverse(root, {
Traverser.traverse(root, {
keys: sourceCode.visitorKeys,
enter(node) {
if (DYNAMIC_PATTERN.test(node.type)) {
retv = true;
Expand All @@ -135,9 +136,10 @@ function hasDynamicExpressions(root) {
* Creates the loop condition information from a given reference.
*
* @param {eslint-scope.Reference} reference - A reference to create.
* @param {SourceCode} sourceCode - The source code object of this context.
* @returns {LoopConditionInfo|null} Created loop condition info, or null.
*/
function toLoopCondition(reference) {
function toLoopCondition(reference, sourceCode) {
if (reference.init) {
return null;
}
Expand Down Expand Up @@ -170,7 +172,7 @@ function toLoopCondition(reference) {
if (GROUP_PATTERN.test(node.type)) {

// If this expression is dynamic, no need to check.
if (hasDynamicExpressions(node)) {
if (hasDynamicExpressions(node, sourceCode)) {
break;
} else {
group = node;
Expand Down Expand Up @@ -254,6 +256,7 @@ module.exports = {
},

create(context) {
const sourceCode = context.getSourceCode();
let groupMap = null;

/**
Expand Down Expand Up @@ -319,7 +322,7 @@ module.exports = {
// Gets references that exist in loop conditions.
const conditions = variable
.references
.map(toLoopCondition)
.map(reference => toLoopCondition(reference, sourceCode))
.filter(Boolean);

if (conditions.length === 0) {
Expand Down