Update no-shadow-restricted-names to handle optional catch

[kesne]

What is the purpose of this pull request? (put an "X" next to 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:

Tell us about your environment

• ESLint Version: 4
• Node Version: 6
• npm Version: 3

What parser (default, Babel-ESLint, etc.) are you using? babel-eslint

Please show your full configuration:
{ "rules": { "no-shadow-restricted-names": "error" }, "parser": "babel-eslint" }

What did you do? Please include the actual source code causing the issue.
Just guarded the access of name on id, which is null when optional catch binding syntax is enabled.

What did you expect to happen?
Eslint runs.

What actually happened? Please include the actual, raw output from ESLint.
Eslint failes to run.

What changes did you make? (Give an overview)
Just a simple change to guard on id existing so that property access on null isn't performed.

Is there anything you'd like reviewers to focus on?
Should be a simple change.

[jsf-clabot]

All committers have signed the CLA.

[platinumazure]

On the one hand, we don't generally support ECMAScript proposals that are below Stage 4 and generally prefer to wait until they reach Stage 4 before really supporting them. On the other hand, this seems like a simple and entirely backward-compatible change.

At the very least, we would need a test for this change, and that would require creating a fixture representing the parse output for a parser that can handle the optional catch binding. But, with that said, I know that there might be some on the team who would say that we should wait to accept this until the proposal reaches stage 4. So I'm not going to insist on you creating a test yet, in case the team decides not to accept this feature (and then hopefully you won't have wasted any time).

@eslint/eslint-team Thoughts?

[ljharb]

In general, this is just a bugfix - eslint rules always need to be coded defensively to work with multiple parsers, and in master, the rule mistakenly assumes that node.id is always non-nullish. This change fixes that broken assumption.

A test case, of course, would be great, but since eslint's parser refuses to follow the ecosystem and implement things prior to stage 4, it seems worth merging this fix now, and waiting until later to ad the test case.

[mysticatea]

As following ESTree spec (except experimental section), the id should not be null. ESLint is using the spec. If a parser creates AST which does not satisfy the spec, it's a bug in the parser.

In my understand, the ESTree spec should have backward compatible in their philosophy: https://github.com/estree/estree#philosophy

[alberto]

@mysticatea when you say

id should not be null

you are referring to the param in the case of a catch clause, right?

[mysticatea]

@alberto Partially yes. Because the id is the 1st parameter of checkForViolation function, so all of VariableDeclarator#id, the elements of Function#params, and CatchClause#param.

I remember the long issue about nullable FunctionDeclaration#id. It was accepted eventually, so probably CatchClause#param is same, but not yet.

[alberto]

It's in the experimental section: https://github.com/estree/estree/blob/master/experimental/optional-catch-binding.md

[mysticatea]

I know that breaking changes can happen in the experimental section of ESTree. For example, the spec of rest/spread properties was changed drastically after espree/babel-eslint implement it. I can't recommend following the experimental section.

[alberto]

Yes, I agree, I didn't mean to imply we should follow that. I was just sharing my findings after checking this.

[ljharb]

So if FunctionDeclaration can have a null id, then why wouldn't we want this rule to be coded defensively? An extra truthiness check is a pretty low cost.

[not-an-aardvark]

A FunctionDeclaration with a null id is valid in general, so it's not a case of coding defensively.

export default function() {}

[ljharb]

What I mean is, a.b.c is not coding defensively, because a and b might be null or undefined. Coding defensively would require a && a.b && a.b.c or similar.

(this applies to any value in JS, regardless of what theoretically should be there wrt an AST node)

[not-an-aardvark]

The general issue is that sometimes doing things "defensively" would result in either (a) a rule with a lot of cases to handle, or (b) a rule that behaves strangely anyway.

Rules typically depend on a variety of invariants of an AST. For example, no-shadow-restricted-names depends on the invariant that all CatchClause nodes have a parameter. Some rules depend on more complicated invariants; for example, prefer-const relies on the invariant that a lexically-declared variable is only declared once in a given scope. All of these invariants are sound according to the behavior specified in ECMA262. For example, catch blocks are currently required to have a parameter, and declaring a lexical variable twice (like let x; let x;) is an early error.

When a parser creates an AST for a program that uses a syntactic language extension/proposal, the AST generally violates one or more of the invariants implied by ECMA262. This is unavoidable, because handling new syntax generally requires new AST structures/nodes that wouldn't have otherwise been possible. However, this means that when a rule receives the new AST, the rule's behavior is effectively undefined, because it might be relying on an invariant which doesn't hold with the experimental AST.

In order to work correctly with experimental ASTs, a rule essentially needs to rely on fewer invariants, to reduce the risk of the rule getting broken by any given syntax extension. Unfortunately, this isn't always possible in general, because rules can't understand the semantics of arbitrary syntax extensions.

Going back to the prefer-const example, what should prefer-const do if a program does contain a duplicate lexical declaration, under some syntax extension that allows this to happen? There isn't really a good answer to this question, because the behavior of prefer-const only makes sense when applied to languages where a lexical variable gets declared exactly once.

In theory, the rule could run as normal and report some errors, but those errors might be incorrect if the syntax extension comes with special semantics that the rule doesn't necessary. Alternatively, we could update the rule to be "defensive" in that case and avoid reporting errors, but there could easily be another similar case that we don't account for. We would end up effectively supporting an arbitrary combination of syntax extensions, because we would be adding special logic for those extensions to ensure the rule doesn't crash. At some point, the end result of doing everything defensively would be to avoid reporting a problem for any AST that violates an invariant, which is equivalent to turning off the rule whenever custom syntax is used.

I think this is the main reason that we're hesitant to add extra logic to rules to make syntax extensions work, even in simple cases like this. By accepting these fixes, we would effectively be supporting this particular syntax extension and giving an implicit guarantee that the rule won't crash when it encounters this syntax extension. But we don't actually want to make that guarantee because we have no way to ensure that rules will work correctly in general for an arbitrary syntax extension, and we can't realistically support all arbitrary syntax extensions that a user might be using.

[kesne]

@not-an-aardvark I understand your hesitation, but that seems like a really hard line to draw to disallow the addition of 6 characters which allow eslint to not break on a stage 3 syntax feature.

[ljharb]

@not-an-aardvark I agree that when invariants are violated, it might not be safe to continue. However, in an eslint rule, that means that the entire process crashes - what would be truly safe is to bail out of the rule entirely when an invariant is violated, and keep going - but that's not how eslint currently works. Thus, defensive coding in rules is necessary.

Either way, by allowing custom parsers, eslint has effectively guaranteed that it can never rely on any invariants - if you want eslint to consider them invariant, then eslint needs to check the parser's AST nodes for compliance before invoking rule code, so as to provide a helpful error message.

[platinumazure]

Personally, I find it strange that we're willing to do a lot of gymnastics around node.typeAnnotation for Flow and TypeScript in core rules (which are not even ECMAScript), but that we're not willing to do the same for Stage 3 ECMAScript proposals that are simple, likely to go in, and don't require a lot of change in the ESLint rules.

I really hope we can lay out a cohesive policy around this in the near future and put something in our README to that effect.

[not-an-aardvark]

Either way, by allowing custom parsers, eslint has effectively guaranteed that it can never rely on any invariants - if you want eslint to consider them invariant, then eslint needs to check the parser's AST nodes for compliance before invoking rule code, so as to provide a helpful error message.

We "rely" on the invariants in that the only supported use case is running the rules on a compliant AST. For convenience, users also have the ability to run the rules on non-compliant ASTs, and the rules will sometimes work. But we can't make guarantees about whether the rules will work correctly in those cases, because the rules won't be familiar with the semantics of the non-compliant AST.

[ljharb]

@not-an-aardvark that doesn't mean that for usability, you can't ensure the rules work.

In this case, optional catch bindings is already shipping in browsers, and may achieve stage 4 as early as this month. Why not adapt eslint to "not crash hard on it" right now?

It's pretty user-hostile already to not support pre-stage-4 features; however by not accepting this bug fix now, all you're doing is making more work for eslint within the next few weeks or months.

[not-an-aardvark]

@platinumazure I agree that we should handle these cases more consistently. Given that our current policy is to only support stage-4 features, I generally think we should stop making exceptions to that. I'd be open to a discussion about changing our policy to support stage 3 features, in which case we could accept changes like this. In either case, I think we should make a policy and stick to it, rather than making repeated one-off exceptions.

@ljharb

that doesn't mean that for usability, you can't ensure the rules work.

When dealing with arbitrary syntax extensions, we can't ensure that rules work. Rules can't know the intended semantics of arbitrary syntax extensions.

You seem to be arguing that we should make rules work with this particular syntax extension, because it's at stage 3. Currently, our policy is to support only stage 4 features, so we don't distinguish between stage 3 features and any other syntax extension. I think it would be reasonable for us to consider change this policy, but I don't think we should make one-off exceptions to it.

[ljharb]

Like object spread, and jsx, and flow, and typescript?

[ljharb]

The current policy is to make one-off exceptions. Let's stick to that.

[not-an-aardvark]

To clarify, by "making one-off exceptions" I mean making changes to fix use-cases that we don't officially support. We officially support JSX and object spread, despite the fact that they aren't stage 4 features, so there isn't any ambiguity about whether we should accept a particular change relating to those features.

If we're going to support optional catch bindings, I think we should document that policy and make sure it's enforced across all rules. That would avoid debates over whether any particular fix is appropriate, and it would also make things easier for rule authors because the set of ASTs that they need to support is clearly defined.

[ljharb]

Makes sense; what's the likelihood of deciding, in this PR, to support optional catch bindings across all rules, even if the default parser doesn't yet support it?

[not-an-aardvark]

I'm not sure. I can add it to the TSC agenda though.

TSC Summary: This PR adds a null-check to a rule that would prevent it from crashing when it encounters code with optional catch bindings, a stage 3 proposal. There is some uncertainty about our policy on accepting changes like this. Officially, we only support stage 4 features, but in the past we've accepted patches to avoid crashes with syntax extensions like Flow, TypeScript, etc.

TSC Questions:

• What is our policy for accepting patches related to custom syntax in rules?
  • Should it make a difference if the patch is very simple?
  • Should it make a difference if the custom syntax is a stage 3 feature and/or a very popular syntax extension?
• Should we support optional catch bindings in general?

[mysticatea]

In my understanding about Flow/TypeScript, ESTree has typeAnnotation in extensions section: https://github.com/estree/estree/blob/master/extensions/type-annotations.md, it's the reason that we have accepted PRs about type annotations.

[alberto]

Regarding the second question. Given our current policy for unfinished proposals, I'd also like to see a clear summary of the arguments (at least) in favor of including this one. Is the only one that "it's simple to implement"?

[not-an-aardvark]

The TSC discussed this issue in today's meeting but did not reach a consensus. We plan to create a new issue to discuss the policy, and come back to it in the next meeting.

[wmertens]

I encountered this crash simply by forgetting to add the parameter to catch, triggering annoying notifications in my editor.

The amount of red tape displayed in this issue is really disheartening - the code is crashing due to an upstream feature/bug that is not going to go away any time soon. Why not patch it now and open another PR with the reverse patch and the discussion?

[mysticatea]

@wmertens Because this is not a problem of ESLint core in the current spec. There is a problem in a custom parser to generate invalid AST. If people use non-javascript syntax, ESLint doesn't guarantee that core rules work fine. So eslint-plugin-babel, eslint-plugin-typescript, and something like have same rules as core rules to work on non-javascript syntax. I expect people to use such way to solve this for right now.

[wmertens]

Still, there is a difference between generating useful linting output, and outright crashing of the linter.

If you say "ESLint doesn't guarantee that core rules work fine", I expect that they would give false positives or negatives. I would not expect a crash under any circumstances.

Perhaps each rule should be run in a try/catch (with err param 😉) so that the other rules can work if one crashes?

[ljharb]

In general, that would be useful - when a file crashes an eslint rule (core or otherwise), it'd be great to get a pointer to the code that it broke on, instead of to the place in the rule code that it broke.

[mysticatea]

Perhaps each rule should be run in a try/catch (with err param 😉) so that the other rules can work if one crashes?

Fair point.
It's worth to investigate. Though I guess that it's not easy than the first impression because ESLint is running every rule in parallel with using EventEmitter.

[platinumazure]

In general, that would be useful - when a file crashes an eslint rule (core or otherwise), it'd be great to get a pointer to the code that it broke on, instead of to the place in the rule code that it broke.

Agreed- see #9742 where we can hopefully get that done next release.

[not-an-aardvark]

Removing the TSC agenda label because this is being discussed in #9804.

[platinumazure]

Okay, coming back to this issue: It seems our new policy on experimental syntax is or will be this:

ESLint's parser only officially supports the latest final ECMAScript standard. We will make changes to core rules in order to avoid crashes on stage 3 ECMAScript syntax proposals. We may make changes to core rules to better work with language extensions (such as JSX, Flow, and TypeScript) on a case-by-case basis.

So since this is just a bugfix to avoid a crash, I think we can accept this issue.

[platinumazure]

Could we add a unit test that would fail without this change, but pass with this change? (You might need to create a parser fixture for this test-- there should be other examples available, but ping me if you get stuck.)

[ljharb]

@platinumazure either way, optional catch is now stage 4, so it’s way past time to merge this :-)

[platinumazure]

@ljharb Thanks, didn't realize this was now stage 4.

Still needs a unit test, per my review comment. I don't care if a feature is Stage 10, PRs still need tests 😀

[platinumazure]

Friendly ping @kesne: Now that we're able to accept this change, are you willing to add one or more unit tests? No rush, just wanted to make sure you saw that this is now accepted. Thanks!

[ljharb]

@platinumazure since eslint can't currently parse optional catch, do you have a suggestion for how to write a test for this?

[platinumazure]

We have other examples based on babel-eslint. Mostly they just create fixtures which export an AST matching what babel-eslint would generate. Then that fixture can be used as the parser in the RuleTester test.

[ljharb]

@platinumazure thanks, is there an easy way to generate them?

[not-an-aardvark]

@ljharb Have you tried this with the latest version of ESLint? I think the version of espree on master can parse optional catch.

It's also worth noting that #10429 fixes the same issue as this PR.

[ljharb]

In that case, let's just merge #10429 since it already addresses the issue.

[not-an-aardvark]

This issue has been solved by #10429. @kesne Thanks for the contribution regardless!