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

Clarify policy on accepting support for new language features #9804

Closed
ilyavolodin opened this issue Jan 4, 2018 · 29 comments
Closed

Clarify policy on accepting support for new language features #9804

ilyavolodin opened this issue Jan 4, 2018 · 29 comments
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features documentation Relates to ESLint's documentation enhancement This change enhances an existing feature of ESLint needs bikeshedding Minor details about this change need to be discussed

Comments

@ilyavolodin
Copy link
Member

ilyavolodin commented Jan 4, 2018

At last TSC meeting we said we will create an issue to discuss current policy on accepting support for new language features.

Our current approach is that we will only accept new ECMA features that reach Stage 4. There has been some exception to this rule in the past. We accepted Object Rest/Spread when it was at Stage 2, but it was put behind experimental prefix. We also accepted features that are not part of ECMA Script spec at all, like JSX, JSDoc, etc. While we haven't accepted any changes to support decorators, we have modified our rules to not fail when decorators are present.

There has been multiple PRs and issues in the past proposing support to new features that we've rejected, however, we have no clear policy that specify what we will accept and what we will not. I think this topic needs clarification so contributors don't waste their time coding support for new feature just to see their PR closed.

@ilyavolodin ilyavolodin added core Relates to ESLint's core APIs and features documentation Relates to ESLint's documentation enhancement This change enhances an existing feature of ESLint needs bikeshedding Minor details about this change need to be discussed labels Jan 4, 2018
@ljharb
Copy link
Sponsor Contributor

ljharb commented Jan 19, 2018

For the record, the appropriate time for everyone - browsers/implementations, babel, tooling like eslint - to implement proposals is at stage 3 (that's the intention of the staging process).

The risk of syntax changes, specifically, in stage 3 is remarkably low, and is even lower after a browser has shipped an implementation (and the risk is not zero in stage 4, either).

My suggestion is that stage 3 syntax proposals that are shipping in at least one browser (or node), should be considered stable enough to implement. Similarly, stage 3 API proposals that are shipping in at least one unflagged browser (to account for web compatibility risk) should be eligible for new rules, or modifications to existing rules, to support.

This, imo, has the best combination of "support stage 3 proposals" without adding any significant risk of breakage.

@not-an-aardvark
Copy link
Member

A few thoughts:

  • acorn, the parser behind espree, only supports stage 4 features. If we decide to start supporting stage features, we would need to implement everything independently in espree, and I doubt the team has time to do that. (In theory, we could accept changes to rules to support stage 3 features without supporting them in the default parser.)

  • The risk of syntax changes, specifically, in stage 3 is remarkably low

    From what I've seen, this doesn't seem to be the case:

    • The syntax for object rest/spread has been modified several times since reaching stage 3, in tc39/proposal-object-rest-spread@f152f9c and tc39/proposal-object-rest-spread@72deb7d.
    • Separately from the proposal's status in TC39, the AST specification for object rest/spread has also changed since it reached stage 3 (see estree/estree@996deb6). In combination with the "Experimental" prefix in espree, this means that there are now 3 different AST node types for the same thing being emitted by different versions of various parsers (ExperimenalRestProperty, RestProperty, and RestElement). This makes for error-prone ESLint rules if they forget to account for all the cases.
    • Rules generally are based on assumptions about the semantics of a feature, in addition to the syntax. As a result, semantic changes could theoretically invalidate some assumptions that a rule makes, and this would either require a rule deprecation or a breaking change in ESLint to fix. (The latter is a problem because ESLint doesn't do major releases very often, so a behavior might be broken for most of the lifetime of a proposal.)

    The general problem here is that if a proposal changes, we will still have people using old versions of babel/babel-eslint/espree, and so we will end up needing to support multiple behaviors simultaneously at least until the next major ESLint release. This makes I think it would be better to externalize feature support into dedicated plugins, which could make breaking changes more easily and quickly without inconveniencing the rest of ESLint's users.

@ljharb
Copy link
Sponsor Contributor

ljharb commented Jan 19, 2018

eslint can choose to add arbitrary acorn plugins to achieve stage 3 support, fwiw.

@not-an-aardvark
Copy link
Member

eslint can choose to add arbitrary acorn plugins to achieve stage 3 support, fwiw.

Yes, but that requires us to implement them, and I don't think we have the manpower for that right now.

@ljharb
Copy link
Sponsor Contributor

ljharb commented Jan 19, 2018

I can quite guarantee you that if eslint officially promises to add the support if but a syntax plugin existed, a mob of people would build one quite rapidly, myself included :-)

@mysticatea
Copy link
Member

mysticatea commented Jan 19, 2018

Acorn has moved to acornjs organization and it looks having official plugins for stage-3 syntaxes. Probably we can use those for parsing.

If eslint-scope needs updates, I can work on it.

My thought seems similar to @ljharb.

  • We can support stage 3 features which are enabled by default in a browser (stable) at least. (Rest/Spread Properties in V8 was fixed before it's available in Chrome stable.)
    • Use ecmaFeatures.experimental***: true.
    • If ecmaVersion is not enough to support an enabled experimental syntax, throw an error. For example, if experimentalPrivateClassMethod is true and ecmaVersion is 5 then throw.
    • We don't need Experimental prefix in AST node type because of consistent -- not all syntactic feature introduces new node type (E.g. Optional Catch Binding).
  • If a breaking change happens in experimental syntax (either TC39 or ESTree), we delay following it until the next major release if increasing errors. Our policy doesn't have two or more major version within a half year. We must describe the change in the release note of the major release. EDIT: we can follow it in the next minor version. We add note "The AST of experimental syntax can be changed in minor version" into our semver policy.
  • If a breaking change happens in stable syntax, the behavior is according to ecmaVersion, as same as current.
  • EDIT: We accept PRs to avoid crashing by type annotations of Flow and TypeScript. ESTree has typeAnnotation/returnType field in their extensions: https://github.com/estree/estree/blob/master/extensions/type-annotations.md
  • EDIT: We don't accept PRs for stage 0-2 features and stage 3 features which aren't enabled by default in any browser. We recommend babel-eslint and eslint-plugin-babel for the purpose.
  • EDIT: We don't accept PRs for non-javascript syntactic features except type annotations.
  • Maybe we should have own AST spec to reduce breakage risk, but maybe we shouldn't have own AST spec in order to use acorn's official plugins. 🤔

@ljharb
Copy link
Sponsor Contributor

ljharb commented Jan 19, 2018

Another alternative is supporting stage 3 proposals with a feature flag - then, if there’s a syntax or semantics change, a different feature flag could be used for the new behavior, and all the outdated ones could be removed in the next major (or, once it’s stage 4, all of them for that proposal would be removed in favor of the ecmaVersion). (I realize feature flags were intentionally removed, but it might make sense here)

@j-f1
Copy link
Contributor

j-f1 commented Jan 19, 2018

You could also say that semver guidelines don’t apply to experimental features — i.e. if a feature makes breaking changes, those changes will affect the next minor version of ESLint. Because the feature is opt-in and the syntax itself is subject to change, it would seem to make sense to be able to quickly adapt to changes.

@not-an-aardvark
Copy link
Member

I would note that the "experimental" status of these language features usually doesn't seem to stop people from using them in production anyway. I suspect that a lot of our users would rather not have their build broken despite relying on experimental features.

That said, having the semver guidelines not apply to experimental features would make things better, in my opinion.

@not-an-aardvark
Copy link
Member

Acorn has moved to acornjs organization and it looks having official plugins for stage-3 syntaxes. Probably we can use those for parsing.

It seems like most of those plugins are released under the AGPL, so I don't think we would be able to use them unless the license is changed.

@mysticatea
Copy link
Member

Sounds good to me if we add "AST of experimental feature can be changed in minor" into our semver policy. I updated my comment #9804 (comment).

I didn't realize the license matter. Maybe does it affect closed source web services which are using ESLint?

@ljharb
Copy link
Sponsor Contributor

ljharb commented Jan 21, 2018

I'm quite sure that if the sole barrier to adoption of any pre-stage-4 feature in eslint by default is the lack of a properly licensed acorn plugin, the appropriate call to action will summon one from the wider JS community :-)

@nzakas
Copy link
Member

nzakas commented Jan 21, 2018

No one has yet mentioned why we (maybe me, initially) decided to limit our support to stage 4 features. Here is some of the rationale, for historical purposes:

  1. In the beginning, we only had ecmaFeatures, no ecmaVersion. I thought this would solve the problem of having ongoing development to ECMAScript that could easily be reflected in ESLint. As it turned out, it ended up being a big pain point for ESLint users because the list of ecmaFeatures was so long. People found that they kept missing important features. Most just wanted to turn on everything up to a specific version. So, we got rid of almost all ecmaFeatures and switched to ecmaVersion to make life easier for everyone.
  2. We added support for a couple of experimental features that were not stage 4 specifically to help people who were using React. experimentalRestSpread and jsx, specifically, were added because these were the only things preventing people from using ESLint's native parser instead of babel-eslint. At the time, babel-eslint was quite a bit slower than our native parser, so we the goal here was to allow React users to use ESLint without babel-eslint in order to improve their performance. We had no other place to stick them besides ecmaFeatures.
  3. We added globalReturn to deal with the NodeJS/CommonJS problem of the extra top-level scope. Without this, the scope calculations we did were inaccurate. (Illegal return statement #1158)
  4. We decided not to add any more features to ecmaFeatures to avoid the problem mentioned in point 1.
  5. We decided against adding experimental (including stage 3) features because we didn't want to get caught in a race to implement every new experimental feature like Babel does. There's just not enough time or devs available to be constantly updating everything. It's much easier for us to wait until things have finalized, at which point the features might have already ended up in Acorn, and if they haven't, we can submit them ourselves with a final implementation. This helps mitigate some of the work we have to do as a team.

The fundamental reason that we now don't support stage 3 features is because we don't know if they will end up in the next edition of ECMAScript or not. Since we are limited to the ecmaVersion property for setting this (as we don't want to keep adding new ecmaFeatures), we need to be certain that a given feature will be definitively represented in a given edition of ECMAScript before adding it. We want everyone ecmaVersion setting to be 100% accurate 100% of the time and, ideally, never change what it means. To put that in concrete terms: if you set ecmaVersion: 2018 in your config, that should always mean the same thing. We don't want to change what ecmaVersion: 2018 means because that could break someone's build.

There have been requests in the past to create something like a "latest" setting for ecmaVersion that would allow us to include everything that has been finalized up to the last publishing of Espree, but the details always got too difficult and we ended up deciding against it (#7528).

So the two real barriers to implementing stage 3 features right now are:

  1. We don't have a config setting to house them under. Because we don't want to add new ecmaFeatures and we can't definitively say which ecmaVersion they will end up under, we just don't have anywhere to put them.
  2. We want ecmaVersion values to always mean the same thing without introducing breaking changes, we can't really start implementing ECMAScript versions piece-by-piece.

Our users rely on ecmaVersion pretty heavily, so unlike browsers that can just implement new features as they come along without much hassle, we don't have that ability,

@ljharb
Copy link
Sponsor Contributor

ljharb commented Jan 21, 2018

One difference is that now there's schema validation, and, the stage process means that in a given calendar year (between publications of an edition of the specification) there's a lot less churn then there used to be in stage 3 proposals.

Some potential options:

  • ecmaVersions that do not yet exist should be schema errors, if they're not already
  • if, say, eslint had some sort of flag to enable the "dotAll" flag for regular expressions (which is now stage 4 and will be in ES2018), when the 2018 ecmaVersion was added in a semver-minor, the dotAll flag could be made to be invalid when paired with an ecmaVersion >= 2018
  • the dotAll flag reached stage 3 in March 2017, when 2016 was the latest ecmaVersion, but if eslint waited to add dotAll until it shipped in a browser (July 2017), then the dotAll flag could have been made invalid with any ecmaVersion < 2017 - which would mean that 2017 is the only ecmaVersion the flag is available for. (Alternatively, it could simply have no effect < 2017 (or >= 2018), but failing the schema check seems safer)

These, paired with "proposal flags can break in a semver-minor", seems like it would address all the issues except "where do the flags go"?


For that bikeshed, I'd suggest in parserOptions, ecmaProposals: ["dotAll", "async-await", "something-new"] - that way there's no false signal to worry about (ie, there's no way for the config to ask not to parse it, only to parse it).

@mysticatea
Copy link
Member

mysticatea commented Jan 22, 2018

@nzakas Thank you for the description. I thought that we can relax this policy because of few reasons:

  • Acorn now has official stage-3 syntax plugins, so the implementation cost is minimum if we can use those.
  • I don't think we must add the support of stage 3 syntactic features (which are enabled by default in a browser at least) by ourselves immediately, but I think we can accept PRs from the community.
  • I think that adding ecmaFeatures.experimental*** is not annoying for users. Those are independent from ecmaVersion and most users just don't use those. However,
    • we can get more time to update core rules in order to work with new syntactic features. I think we need more time to update core rules than add parsing.
    • supporting stage 3 syntactic features (which are enabled by default in a browser at least) will be useful for the developers on the browser (browser extensions, Electron, and Node.js).
  • When an ecmaFeatures.experimental*** gets stage 4 (...or when the official publication of the spec), we can move it to ecmaVersion in safe.

@nzakas
Copy link
Member

nzakas commented Jan 22, 2018

I thought the purpose of this issue was just to document what we do and don't support, not to propose changing it?

@mysticatea

I think that adding ecmaFeatures.experimental*** is not annoying for users. Those are independent from ecmaVersion and most users just don't use those.

When an ecmaFeatures.experimental*** gets stage 4 (...or when the official publication of the spec), we can move it to ecmaVersion in safe.

This is exactly what I wanted to avoid. One of our guiding principles has been, for a while, that we don't want people to have to update their configs all that frequently. If we add an experimental*** flag and then later remove it, then we are forcing people to update their configs in order to get the same behavior (and if we just leave the flag, then we end up with confusing configs). I'm not a fan of that, nor the overhead of constantly adding/removing ecmaFeatures options (this also goes for @ljharb's proposal for ecmaProposals).

Again, we did all of this for ES6 features, it turned into a lot of confusion for everyone. I'd really rather not go through that again.

As long as babel-eslint exists, I'm just not convinced that there's any pressing need to jump to implement stage 3 proposals. People who really need or want to use the latest stuff are already using Babel, so having them use babel-eslint isn't a huge leap.

But again, I was under the impression that this issue was just to document the current approach to supporting ECMAScript versions and features. It seems like we should be focusing on that and if someone wants to propose a change, that should be a separate issue that lays out the use cases and requirements.

@not-an-aardvark
Copy link
Member

not-an-aardvark commented Jan 22, 2018

Going back to discussion about clarifying the current policy:

The conversation that started this issue is in #9613, about adding a null check to a rule to avoid a runtime crash after encountering a missing catch binding (currently a stage 3 proposal). I originally added it to the TSC agenda because it seems like we've been inconsistent about whether we accept this type of change.

Previous cases that I can find (I might be missing some):

# Nonstandard feature Description of change Accepted?
#6723 Type annotations Prevents crash in space-infix-ops Yes
#6945 Type annotations Prevents false positive in object-curly-spacing Yes
#7417 Async iterators Prevents false positive in semi No
#7445 Type annotations Prevents false positive in array-bracket-spacing Yes
#7708 Type declarations Prevents false positive in no-duplicate-imports No
#8111 Type annotations Prevents false positive in keyword-spacing Yes
#8341 Type annotations Prevents false positive in space-infix-ops Yes
#8349 Type annotations Prevents false positive in space-before-function-paren Yes
#8958 Class properties Prevents false negative in semi No
#9027 Type annotations Prevents false positive in indent Yes
#9458 Type annotations Prevents false positive in object-curly-newline Yes
#9636 BigInt Prevents false positive in valid-typeof No

This seems inconsistent, and as a result I'm not sure what our official policy is. Ideally, we can clarify what type of changes we will accept so that there isn't any ambiguity in the future.

@ljharb
Copy link
Sponsor Contributor

ljharb commented Jan 22, 2018

@not-an-aardvark also, #7230 was not accepted (prefer object spread over Object.assign({}, …))

@not-an-aardvark
Copy link
Member

I didn't include #7230 because object spread is a feature that we explicitly do support with the experimentalObjectRestSpread option, whereas all of the other examples relate to syntax that comes from external parsers. #7230 also proposed adding new features/options targeted at experimental syntax, which IMO is a different consideration than making the existing code work correctly with experimental syntax.

@mysticatea
Copy link
Member

I'm sorry for I have gone too far.

My thoughts on this issue are:

  • We can accept PRs to avoid crashing by stage 3 syntactic features which are enabled by default in a browser (stable) at least.
    I think that this border is reasonable for stability because:
    • Stage 3 means invitation of implementations, but there are proposals which retired from stage 3.
    • Rest/Spread Properties spec was changed in stage 3 after V8 implemented it. But V8 fixed the feature before it's available in Chrome stable.
    • We have many rules, so we need time to update the rules in order to work with new syntactic features.
  • We accept PRs to avoid crashing by type annotations of Flow and TypeScript because ESTree has typeAnnotation/returnType field in their extensions.

@nzakas
Copy link
Member

nzakas commented Jan 25, 2018

I agree with @mysticatea's last comment. We have a history of updating rules to avoid crashing on nonstandard and experimental AST nodes when they are well-defined and obvious.

I'd say we can clarify our support by saying something like:

ESLint's parser only officially supports the latest final ECMAScript standard. We will, however, make changes to core rules if they crash on experimental features or popular language extensions like TypeScript and Flow.

@j-f1
Copy link
Contributor

j-f1 commented Jan 25, 2018

s/TypeScript and Flow/TypeScript, Flow, and JSX/?

@not-an-aardvark
Copy link
Member

not-an-aardvark commented Jan 25, 2018

Would this policy only extend to crashes, or would we also fix false positives resulting from these language features?

edit: In other words, are we committing to making the rules work correctly for experimental syntax, or are we just committing to having the rules not crash and allowing your linting build to continue (even if they report a bunch of incorrect errors)?

@ilyavolodin
Copy link
Member Author

My suggestion would be to accept fixes that stop rules from breaking (as in not being able to complete linting process on the file that uses Stage 3 features), but not the fixes that decrease/increase number of errors. Also, I would want to specify that this rule should only apply to any Stage 3 features that don't introduce new ESTree nodes (or rather, non-Stage 4 AST Nodes should not be used anywhere in the code).

@j-f1
Copy link
Contributor

j-f1 commented Jan 25, 2018

non-Stage 4 AST Nodes should not be used anywhere in the code

What about JSX and type annotations? They’re already referenced in the codebase.

@not-an-aardvark
Copy link
Member

not-an-aardvark commented Jan 25, 2018

My understanding has been that we have full support for JSX, and treat it the same way as stage 4 features. However, I have the same question as @j-f1 about type annotations.

Ideally, I think it would be nice if the policy allowed changes like #9027 (fixes a false positive when using type annotations), because the bug would otherwise have made the indent rule very inconvenient to use with custom syntax.

@ilyavolodin
Copy link
Member Author

My assumption is that this policy only applies to ECMAScript features. Anything outside of the ECMA spec is treated differently on a case-by-case bases (which includes type annotations, JSX, JSDoc, etc.).

@nzakas
Copy link
Member

nzakas commented Jan 26, 2018

Agree with @ilyavolodin. So how about 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.

@platinumazure
Copy link
Member

I would also like to propose adding an issue label like "experimental", which would allow us to review at a glance what proposals are or were for experimental features.

I'm okay with bikeshedding on whether we should have a label just for rejected experimental change requests (which would allow us to easily find those if the syntax hits Stage 4 and reopen them, if we wanted); or if we should have a general "experimental" label and a further label to indicate a decision to reject an issue/PR, which combined with "experimental" shows why we closed the issue. Either way is fine by me. I just think we should be labeling those issues in some way.

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Dec 8, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Dec 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features documentation Relates to ESLint's documentation enhancement This change enhances an existing feature of ESLint needs bikeshedding Minor details about this change need to be discussed
Projects
None yet
Development

No branches or pull requests

7 participants