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

`fixable` property only necessary when `meta` is present #13349

Open
brettz9 opened this issue May 23, 2020 · 23 comments
Open

`fixable` property only necessary when `meta` is present #13349

brettz9 opened this issue May 23, 2020 · 23 comments
Assignees

Comments

@brettz9
Copy link
Contributor

@brettz9 brettz9 commented May 23, 2020

What did you expect to happen?

Per the docs:

Important: Without the fixable property, ESLint does not apply fixes even if the rule implements fix functions. Omit the fixable property if the rule is not fixable.

Based on this, I would expect a rule without any meta at all to also not be fixable.

What actually happened? Please include the actual, raw output from ESLint.

A rule without meta is successfully fixed when it has a fixer.

The docs are true, however, if there is a meta object of some sort (including an empty object); in such a case, an error will be reported that fixable is necessary if the rule attempts to add a fixer. But if there is no meta object at all, fixable is not necessary for fixes to be applied for the rule.

I am not expecting breaking changes to start preventing fixers without meta; I'd just expect the docs to mention that rules still without the recommended meta property at all are currently still fixable.

@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented May 23, 2020

@brettz9 Can you fill out an issue template? It's not clear to me what the desired outcome of this issue is. Thanks!

@brettz9
Copy link
Contributor Author

@brettz9 brettz9 commented May 23, 2020

I've updated to include those two headings relevant to a doc bug.

@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented May 23, 2020

Thank you for clarifying. Agreed, and PRs to improve documentation are always welcome!

@anikethsaha
Copy link
Member

@anikethsaha anikethsaha commented May 23, 2020

I am not expecting breaking changes to start preventing fixers without meta;

agreed. but shouldn't it?

IMO, the fixer shouldnt work or throw an error if meta is not present.
cause in rule authoring docs, it is mentioned that we can omit docs but it is not mentioned about the meta. (or maybe I am missing.)

thoughts?
otherwise I think bug label can be removed then ?

@brettz9
Copy link
Contributor Author

@brettz9 brettz9 commented May 24, 2020

Re: it not mentioning meta, yeah, the Working with Rules page doesn't mention that meta can be omitted.

As far as whether it should be a breaking change to require meta.fixable always, I'd personally even be in favor of always requiring meta (specifically so as to require meta.type even for non-fixable rules--though that is not required now and would perhaps also need an "other" type; without getting too off track, in my eslint badge-maker, I felt the need to let users override the built-in type's as they are not very specific (e.g., as to whether the "problem" is for security, preventing obtrusiveness, etc); I could perhaps use docs.category for that instead, but these are not enumerated in the docs nor encouraged for third-party use).

However, I have come across quite a number of plugins using the legacy format or not using meta if they are, so I would expect people might worry about breaking too many old plugins. Might be good to mention it may become required in a future version, both to encourage meta (and meta.type) in case a breaking change is ever seen as feasible and desirable.

@und3fined-v01d
Copy link

@und3fined-v01d und3fined-v01d commented Jun 4, 2020

Hey guys I am looking to contribute to ESLint. I would like to work on this issue however based on what I read, I am not sure if this requires just a documentation fix. Can anyone appropriately guide me to work on it. I have already set up ESLint on my machine.

@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Jun 4, 2020

This is marked as accepted for a documentation update, though I do think this is unclear behavior.

@ajkovar
Copy link

@ajkovar ajkovar commented Jun 12, 2020

Hi. I'm interested in taking this one if no one minds. Seems a pretty straight forward documentation update.

ajkovar added a commit to ajkovar/eslint that referenced this issue Jun 12, 2020
If metadata property is missing, `fixable` essentially gets defaulted
to true.  Described this behavior in the docs as well as added
recommendations to include metadata/type properties
@matthenschke
Copy link

@matthenschke matthenschke commented Jun 12, 2020

Is anyone working on this issue? If not, I would not mind working on it.

@btmills
Copy link
Member

@btmills btmills commented Jun 18, 2020

I have come across quite a number of plugins using the legacy format or not using meta

@brettz9 do you happen to know of any examples that currently rely on this buggy behavior by providing fixers without including meta?

I see three options to resolve this:

  1. Fix the bug and require meta.
  2. Document the existing behavior with a warning that we'll fix it in a future major version.
  3. Document the existing behavior and live with it even though it's unintended.

Which path I prefer would depend on how much fixing the buggy behavior would break.

@brettz9
Copy link
Contributor Author

@brettz9 brettz9 commented Jun 18, 2020

Authors I've encountered have thankfully been open to updates, e.g., recently with SonarSource/eslint-plugin-sonarjs#161 . (In the cases where the lack of meta.fixable is due to using the old format where there is just a function exported and no create as well as no meta (i.e., when the old format is in use), authors might see this less as "buggy" and more as merely "deprecated" since those rules predated meta entirely, but I get your gist, esp. with the docs urging this now.)

I am waiting on one PR, Turbo87/eslint-plugin-chai-expect#99 , to introduce the new rule format, so I know at least one plugins still using the old format, but in this case, the rules do not happen to be fixable.

If you were only going to fix the case of requiring meta.fixable when create was present, I would think you might avoid a major bump (assuming the docs when the new rule format was introduced had from the beginning added mention of this need).

However, I would think no. 1 on your list is not an option for a major bump when the fixing occurs with the deprecated rule format since that format (which did not even allow meta at all despite supporting fixing) is still mentioned as supported (though deprecated) at https://eslint.org/docs/developer-guide/working-with-rules (beginning of page).

While I haven't done any exhaustive study (e.g., of https://github.com/dustinspecker/awesome-eslint/ or those on npm), I would think therefore that with no. 1 excluded, no. 2 could be chosen over no. 3 as:

  1. At least the big ones I have used or needed to use with other projects, "standard", "prettier", "promise", "import", "jsdoc", "sonarjs" and other ones of interest to me (e.g., "mocha", "mocha-cleanup", various chai ones) are pretty much all on board already.
  2. It's not too big of a hassle to just add a create method and if needed add a fixable.
  3. The blog post introducing the new rule format (at https://eslint.org/blog/2016/07/eslint-new-rule-format ) mentions "knowing that a rule is fixable ahead of time allows us to optimize the autofixing process as well as call this out in documentation" so it should be nice to be able to enforce this benefit (even if there may be some plugins or custom rules in the wild that have yet to make the change).

IMO, it would be nice to also require meta.type (and meta.docs (url and/or description) too if the latter were worded less as being ESLint-project-specific). Even for non-fixable rules, a type can be used by formatters, so that tools could more readily make use of them. Authors I have encountered have been open to adding the former or have at least not objected for the few PRs I have still waiting merging (I haven't made PRs for docs). Don't want to distract with this suggestion, but just mentioning if a breaking change is being made anyways and there were interest.

@nzakas
Copy link
Member

@nzakas nzakas commented Jun 19, 2020

Because we don't guarantee fixes to be applied, I think we can fix this behavior to work as it was originally intended. The result of using a rule with a legacy fixer is that it will only warn and not fix, which I wouldn't consider a breaking change.

The bigger question is why RuleTester isn't testing for this already. That's one of our primary ways of enforcing rule standards and I'm surprised there's not a check in there.

Here's what I would suggest as a path forward:

  1. Make an announcement about this explaining that it is a bug and we know it will affect people.
  2. Update RuleTester to check that meta.fixable is present whenever a fix is provided. Release this first without changing how rules actually work.
  3. Fix the bug with how rule fixes are being applied in a subsequent release.
@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Jul 12, 2020

If there is consensus on these steps, I could start working on RuleTester.

@btmills btmills added accepted and removed evaluating labels Jul 12, 2020
@btmills
Copy link
Member

@btmills btmills commented Jul 12, 2020

3 of the other 4 TSC members left a 👍 on @nzakas’s comment, and nobody’s objected, so I’d say you’re good to go.

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Jul 12, 2020

I think the documentation is still incorrect, even if there's meta without fixable:

Important: Without the fixable property, ESLint does not apply fixes even if the rule implements fix functions. Omit the fixable property if the rule is not fixable.

Important: Unless the rule exports the meta.fixable property, ESLint does not apply fixes even if the rule implements fix functions.

It sounds like ESLint will just not apply fixes, but it actually throws when this happens. This was an intended change (#6043), so perhaps we should update the docs to be more accurate about what will actually happen.

If we want the same behavior in the case when rule that provides fixes doesn't have meta (which is what this issue is about), then ESLint should throw in that case as well. Maybe we should add this as step 4 and implement it in the next major version?

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Jul 12, 2020

The bigger question is why RuleTester isn't testing for this already. That's one of our primary ways of enforcing rule standards and I'm surprised there's not a check in there.

It was decided to do these checks and throw in the linter (#5970), so there was no need to update RuleTester.

However, it seems that this scenario (new-format rule without meta) was missed from the start. Even prior to the change to throw, fixes were being applied for rules that don't have meta in order to support fixable rules that have the legacy-format (rules that export function).

If we still want to allow fixable rules that have the legacy format (I guess we should?), maybe we could add some info about the original format here, and then use it in the rule tester (step 2) and linter (subsequent steps).

edit: RuleTester can see the original (unnormalized) rule.

@nzakas
Copy link
Member

@nzakas nzakas commented Jul 14, 2020

If we want the same behavior in the case when rule that provides fixes doesn't have meta (which is what this issue is about), then ESLint should throw in that case as well. Maybe we should add this as step 4 and implement it in the next major version?

Nice catch! I think in this case, we should just make step 3 the final fix that throws an error and implement that in a major release. I don't see a ton of benefit to doing a silent-fail of fixes first and then throwing an error later.

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Jul 14, 2020

So, the steps would be:

  1. Make an announcement.
  2. Throw in RuleTester (might break plugin builds).
  3. Throw in Linter (might affect end users if plugin hasn't fixed the problem) - major release.

step 2 is ready in #13489. Should there be some time between step 1 and step 2?

Also to confirm: this doesn't apply to legacy-format rules (those that export a function and thus don't have meta)?

@brettz9
Copy link
Contributor Author

@brettz9 brettz9 commented Jul 14, 2020

Also to confirm: this doesn't apply to legacy-format rules (those that export a function and thus don't have meta)?

Per @nzakas in #13349 (comment)

...The result of using a rule with a legacy fixer is that it will only warn and not fix, which I wouldn't consider a breaking change.
The bigger question is why RuleTester isn't testing for this already...

While this is speaking of only warning, it seems to me from this indication of (no longer) fixing, that the idea is to start treating legacy format rules the same, i.e., that all your steps should apply to both the legacy format (functions) and to missing meta on objects (as with an object missing meta.fixable) and also that another step should be added, presumably preceding your current step 3, that avoids fixing for legacy (function) format.

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Jul 16, 2020

I interpreted this comment differently since it wasn't originally intended to disallow fixable legacy-format rules.

I'm not sure whether we should now break legacy-format rules that are following the spec for that format (working-with-rules-deprecated).

@nzakas can you clarify this? I think we all agree to disallow new-format rules that produce fixes but don't export meta. The question is what to do with legacy-format rules.

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Jul 16, 2020

As clarified in today's TSC meeting, steps 1-3 will apply to legacy (function) format as well.

Meaning that, as of v8.0.0, only rules that export meta.fixable (which implies that it's a new-format rule, and that it exports meta) can be fixable. In v8.0.0, an error will be thrown during linting whenever a rule that doesn't satisfy this condition produces a fix.

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Jul 16, 2020

and also that another step should be added, presumably preceding your current step 3, that avoids fixing for legacy (function) format.

I think this isn't necessary, similar to how it seems unnecessary for new-format rules that don't export meta.

kaicataldo pushed a commit that referenced this issue Jul 18, 2020
kaicataldo pushed a commit that referenced this issue Jul 31, 2020
…13489)

* Update: require `meta` for fixable rules in RuleTester (refs #13349)

* Fix JSDoc

* Throw for legacy-format fixable rules
Moeriki added a commit to Moeriki/eslint that referenced this issue Aug 9, 2020
…gnore-default-values

* upstream/master: (66 commits)
  Sponsors: Sync README with website
  Sponsors: Sync README with website
  Sponsors: Sync README with website
  Sponsors: Sync README with website
  Sponsors: Sync README with website
  Chore: remove leche (fixes eslint#13287) (eslint#13533)
  Sponsors: Sync README with website
  Sponsors: Sync README with website
  7.6.0
  Build: changelog update for 7.6.0
  Update: require `meta` for fixable rules in RuleTester (refs eslint#13349) (eslint#13489)
  Docs: fix broken links in developer guide (eslint#13518)
  Fix: Do not output `undefined` as line and column when it's unavailable (eslint#13519)
  Sponsors: Sync README with website
  Sponsors: Sync README with website
  Fix: Update the chatroom link to go directly to help channel (eslint#13536)
  Sponsors: Sync README with website
  Update: Change no-duplicate-case to comparing tokens (fixes eslint#13485) (eslint#13494)
  Docs: add ECMAScript 2020 to README (eslint#13510)
  7.5.0
  ...
@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Aug 20, 2020

Added "breaking" label for the remaining step 3.

@mdjermanovic mdjermanovic self-assigned this Aug 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
9 participants
You can’t perform that action at this time.