Fix: ecmaVersion defaults to 5 and allows "latest" in RuleTester

[mdjermanovic]

Prerequisites checklist

• I have read the contributing guidelines.

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

I noticed that this test is failing in #14653:

eslint/tests/lib/rule-tester/rule-tester.js

Lines 632 to 641 in 353ddf9

	it("should throw an error if there's a parsing error and output doesn't match", () => {
	assert.throws(() => {
	ruleTester.run("no-eval", require("../../fixtures/testers/rule-tester/no-eval"), {
	valid: [],
	invalid: [
	{ code: "eval(`foo`)", output: "eval(`foo`);", errors: [{}] }
	]
	});
	}, /fatal parsing error/iu);
	});

The test is unrelated to the PR. It's failing because:

• Test expects a parsing error on ES6 code (template string) when ecmaVersion isn't specified.
• PR uses Espree v8.
• Espree v8 has new default for ecmaVersion (latest, currently 13)
• RuleTester always wraps parsers to add some checks directly on AST.
• Linter gets a parser that isn't Espree (it's wrapped Espree) so it doesn't set ecmaVersion: 5
• The ES6 code doesn't produce a parsing error, because it's parsed with the latest ecmaVersion (new Espree's default).

This reveals some problems:

• RuleTester doesn't support ecmaVersion: "latest"
• If ecmaVersion isn't specified, rules that use context.parserOptions will get ecmaVersion: 5 in runtime, but ecmaVersion: undefined when run through RuleTester.
• With Espree v8, all tests that don't have ecmaVersion specified will be parsed with the latest ecmaVersion.

What changes did you make? (Give an overview)

Updated RuleTester to perform normalizations from #14622

Is there anything you'd like reviewers to focus on?

• Is this a good solution? An alternative is to add some identification key to the wrapper object and read that key in Linter.
• This doesn't work well with tests that have /*eslint-env*/ comments in the code because parserOptions in configurations have precedence over parserOptions in environments. For example, a test with code: "/*eslint-env es6*/ let x;" will produce a parsing error after this change.
• Should we consider reverting Update: ecmaVersion defaults to 5, and allows "latest" #14622?

[mdjermanovic]

@nzakas, @aladdin-add this is related to #14622.

[aladdin-add]

Should we consider reverting #14622?

I'm 👍 to this - I found the easiest way to get it is to add a wrapper to espree, to modify its defaults. something like:

const espree = require("espree");

espree.parse = function(){
  // TODO: change its defaults
}

module.exports = espree;

then we can replace all the require("espree") to it in the codebase. so , we don't need to change the linter, ruleTester(and maybe some more). thoughts?

[mdjermanovic]

Another alternative is to also check parserName in this condition:

eslint/lib/linter/linter.js

Lines 440 to 456 in 353ddf9

	function normalizeEcmaVersion(parser, ecmaVersion) {
	if (parser === espree) {
	if (ecmaVersion === void 0) {
	return DEFAULT_ECMA_VERSION;
	}
	if (ecmaVersion === "latest") {
	return espree.latestEcmaVersion;
	}
	}
	/*
	* Calculate ECMAScript edition number from official year version starting with
	* ES2015, which corresponds with ES6 (or a difference of 2009).
	*/
	return ecmaVersion >= 2015 ? ecmaVersion - 2009 : ecmaVersion;
	}

if (parser === espree) -> if (parser === espree || parserName === espreePath)

Linter would get espreePath with require.resolve("espree").

This will work with RuleTester because RuleTester sets the absolute path of the original parser as parser name for the wrapper object.

[aladdin-add]

LGTM!

[aladdin-add]

it can be removed when eslint has the same defaults with espree?

[aladdin-add]

Is this a good solution? An alternative is to add some identification key to the wrapper object and read that key in Linter.

@mdjermanovic I've pushed a commit aladdin-add@f701736, it avoids the duplicate Linter's espree-specific normalizations. thoughts?

[mdjermanovic]

Is this a good solution? An alternative is to add some identification key to the wrapper object and read that key in Linter.

@mdjermanovic I've pushed a commit aladdin-add@f701736, it avoids the duplicate Linter's espree-specific normalizations. thoughts?

Yes, now I think something like that would be better than duplicating, which is error-prone by itself, and RuleTester, unlike Linter, doesn't have the final parserOptions after merging environments.

For example, if we add the following test case here, it fails on this branch but works well with aladdin-add@f701736:

{
    code: "/*eslint-env es6*/ let x;",
    errors: [{ messageId: "ecmaVersionMessage", data: { type: "number", ecmaVersion: "6" } }]
}

@nzakas thoughts about this?

[mdjermanovic]

We decided to revert 831f6b3 for now, and also keep ecmaVersion: 5 default in Espree v8.

We'll revisit "latest" next week.

I'll keep this open for the case there's some code that might be useful for a "latest" PR.

[nzakas]

Should we close this now?

[mdjermanovic]

Yes, closing in favor of #14720