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

linter: rule fixer improvements #4179

Open
2 tasks done
DonIsaac opened this issue Jul 11, 2024 · 10 comments
Open
2 tasks done

linter: rule fixer improvements #4179

DonIsaac opened this issue Jul 11, 2024 · 10 comments
Labels
A-linter Area - Linter C-enhancement Category - New feature or request good first issue Experience Level - Good for newcomers

Comments

@DonIsaac
Copy link
Collaborator

DonIsaac commented Jul 11, 2024

I've been adding and repairing fixers to a lot of rules, but my ad-hoc approach
is not sustainable. This issue is a call for assistance, and tracks fixer
progress for each rule.

Prerequisites

Rules

For rules with an existing fixer:

  • If the rule has a fixer (using diagnostic_with_fix), ensure that there are
    no cases where the fixer could create a syntax error or semantically different
    code.
  • If the fix could be correct in some cases, but could produce semantically
    differently different code in others, use diagnostic_with_fix_dangerous.
  • Link this issue to your PR updating the rule's fixer, and a maintainer will
    update this table. If no changes are needed, leave a comment here and we'll
    also update the table.

For rules without a fixer:

  • Check if a fixer could be safely added in some cases. If so, create a PR with
    a fixer implementation that links to this issue.
  • If no fixer can (or should) be created for the rule, leave a comment below and
    a maintainer will update the table.

Legend

  • : Not checked
  • ✅ Has a working fixer
  • 🚧 No fixer, but one could be added
  • 🚫 No fixer, and one cannot be added

Correctness (159):

Code that is outright wrong or useless.

Rule name Source Status
for-direction eslint
no-async-promise-executor eslint
no-caller eslint
no-class-assign eslint
no-compare-neg-zero eslint
no-cond-assign eslint
no-const-assign eslint
no-constant-binary-expression eslint
no-constant-condition eslint
no-control-regex eslint
no-debugger eslint
no-delete-var eslint
no-dupe-class-members eslint
no-dupe-else-if eslint
no-dupe-keys eslint
no-duplicate-case eslint
no-empty-character-class eslint
no-empty-pattern eslint
no-empty-static-block eslint
no-ex-assign eslint
no-extra-boolean-cast eslint
no-func-assign eslint
no-global-assign eslint
no-import-assign eslint
no-irregular-whitespace eslint
no-loss-of-precision eslint
no-new-native-nonconstructor eslint
no-nonoctal-decimal-escape eslint
no-obj-calls eslint
no-self-assign eslint
no-setter-return eslint
no-shadow-restricted-names eslint
no-sparse-arrays eslint
no-this-before-super eslint
no-unsafe-finally eslint
no-unsafe-negation eslint
no-unused-labels eslint
no-unused-private-class-members eslint
no-useless-catch eslint
no-useless-escape eslint
no-useless-rename eslint
no-with eslint
require-yield eslint
use-isnan eslint
valid-typeof eslint
default import
named import
namespace import
expect-expect jest
no-conditional-expect jest
no-disabled-tests jest
no-export jest
no-focused-tests jest
no-standalone-expect jest
require-to-throw-message jest
valid-describe-callback jest
valid-expect jest
valid-title jest
check-property-names jsdoc
check-tag-names jsdoc
implements-on-classes jsdoc
no-defaults jsdoc
require-property jsdoc
require-property-description jsdoc
require-property-name jsdoc
require-property-type jsdoc
require-yields jsdoc
alt-text jsx_a11y
anchor-has-content jsx_a11y
anchor-is-valid jsx_a11y
aria-activedescendant-has-tabindex jsx_a11y
aria-props jsx_a11y
aria-role jsx_a11y
aria-unsupported-elements jsx_a11y
autocomplete-valid jsx_a11y
click-events-have-key-events jsx_a11y
heading-has-content jsx_a11y
html-has-lang jsx_a11y
iframe-has-title jsx_a11y
img-redundant-alt jsx_a11y
lang jsx_a11y
media-has-caption jsx_a11y
mouse-events-have-key-events jsx_a11y
no-access-key jsx_a11y
no-aria-hidden-on-focusable jsx_a11y
no-autofocus jsx_a11y
no-distracting-elements jsx_a11y
no-redundant-roles jsx_a11y
prefer-tag-over-role jsx_a11y
role-has-required-aria-props jsx_a11y
role-supports-aria-props jsx_a11y
scope jsx_a11y
tabindex-no-positive jsx_a11y
google-font-display nextjs
google-font-preconnect nextjs
inline-script-id nextjs
next-script-for-ga nextjs
no-assign-module-variable nextjs
no-async-client-component nextjs
no-before-interactive-script-outside-document nextjs
no-css-tags nextjs
no-document-import-in-page nextjs
no-duplicate-head nextjs
no-head-element nextjs
no-head-import-in-document nextjs
no-img-element nextjs
no-page-custom-font nextjs
no-script-component-in-head nextjs
no-styled-jsx-in-document nextjs
no-sync-scripts nextjs
no-title-in-document-head nextjs
no-typos nextjs
no-unwanted-polyfillio nextjs
bad-array-method-on-arguments oxc
bad-char-at-comparison oxc
bad-comparison-sequence oxc
bad-min-max-func oxc
bad-object-literal-comparison oxc
bad-replace-all-arg oxc
const-comparisons oxc
double-comparisons oxc
erasing-op oxc
missing-throw oxc
number-arg-out-of-range oxc
only-used-in-recursion oxc
uninvoked-array-callback oxc
jsx-key react
jsx-no-duplicate-props react
jsx-no-target-blank react
jsx-no-undef react
no-children-prop react
no-direct-mutation-state react
no-find-dom-node react
no-is-mounted react
no-render-return-value react
no-string-refs react
void-dom-elements-no-children react
no-extra-non-null-assertion typescript
no-misused-new typescript
no-namespace typescript
no-non-null-asserted-optional-chain typescript
no-this-alias typescript
no-unsafe-declaration-merging typescript
no-useless-empty-export typescript
prefer-as-const typescript
triple-slash-reference typescript
no-await-in-promise-methods unicorn
no-document-cookie unicorn
no-empty-file unicorn
no-invalid-remove-event-listener unicorn
no-new-array unicorn
no-single-promise-in-promise-methods unicorn
no-thenable unicorn
no-unnecessary-await unicorn
no-useless-fallback-in-spread unicorn
no-useless-length-check unicorn
no-useless-spread unicorn
prefer-set-size unicorn
prefer-string-starts-ends-with unicorn

Perf (6):

Code that can be written to run faster.

Rule name Source Status
no-await-in-loop eslint
no-accumulating-spread oxc
jsx-no-jsx-as-prop react_perf
jsx-no-new-array-as-prop react_perf
jsx-no-new-function-as-prop react_perf
jsx-no-new-object-as-prop react_perf

Restriction (49):

Lints which prevent the use of language and library features. Must not be enabled as a whole, should be considered on a case-by-case basis before enabling.

Rule name Source Status
default-case eslint
no-bitwise eslint
no-console eslint
no-div-regex eslint
no-empty eslint
no-empty-function eslint
no-eq-null eslint
no-eval eslint
no-iterator eslint
no-proto eslint
no-regex-spaces eslint
no-restricted-globals eslint
no-undefined eslint
no-unsafe-optional-chaining eslint
no-var eslint
no-void eslint
unicode-bom eslint
no-amd import
no-cycle import
no-default-export import
check-access jsdoc
empty-tags jsdoc
bad-bitwise-operator oxc
no-async-await oxc
no-barrel-file oxc
no-const-enum oxc
no-optional-chaining oxc
no-rest-spread-properties oxc
button-has-type react
no-danger react
no-unknown-property react
explicit-function-return-type typescript
no-dynamic-delete typescript
no-explicit-any typescript
no-import-type-side-effects typescript
no-non-null-asserted-nullish-coalescing typescript
no-non-null-assertion typescript
no-var-requires typescript
prefer-literal-enum-member typescript
no-abusive-eslint-disable unicorn
no-anonymous-default-export unicorn
no-array-for-each unicorn
no-array-reduce unicorn
no-magic-array-flat-depth unicorn
no-nested-ternary unicorn
no-process-exit unicorn
prefer-modern-math-apis unicorn
prefer-node-protocol unicorn
prefer-number-properties unicorn

Suspicious (14):

code that is most likely wrong or useless.

Rule name Source Status
no-new eslint
no-useless-concat eslint
no-useless-constructor eslint
no-duplicates import
no-named-as-default import
no-named-as-default-member import
no-self-import import
no-commented-out-tests jest
approx-constant oxc
misrefactored-assign-op oxc
jsx-no-comment-textnodes react
react-in-jsx-scope react
no-unnecessary-type-constraint typescript
prefer-add-event-listener unicorn

Pedantic (66):

Lints which are rather strict or have occasional false positives.

Rule name Source Status
array-callback-return eslint
eqeqeq eslint
max-classes-per-file eslint
max-lines eslint
no-array-constructor eslint
no-case-declarations eslint
no-constructor-return eslint
no-fallthrough eslint
no-inner-declarations eslint
no-new-wrappers eslint
no-prototype-builtins eslint
no-redeclare eslint
no-self-compare eslint
radix eslint
require-await eslint
symbol-description eslint
max-dependencies import
require-param jsdoc
require-param-description jsdoc
require-param-name jsdoc
require-param-type jsdoc
require-returns jsdoc
require-returns-description jsdoc
require-returns-type jsdoc
checked-requires-onchange-or-readonly react
jsx-no-useless-fragment react
no-unescaped-entities react
ban-ts-comment typescript
ban-types typescript
no-duplicate-enum-values typescript
prefer-enum-initializers typescript
prefer-ts-expect-error typescript
escape-case unicorn
explicit-length-check unicorn
new-for-builtins unicorn
no-hex-escape unicorn
no-instanceof-array unicorn
no-lonely-if unicorn
no-negated-condition unicorn
no-negation-in-equality-check unicorn
no-new-buffer unicorn
no-object-as-default-parameter unicorn
no-static-only-class unicorn
no-this-assignment unicorn
no-typeof-undefined unicorn
no-unreadable-iife unicorn
no-useless-promise-resolve-reject unicorn
no-useless-switch-case unicorn
prefer-array-flat unicorn
prefer-array-some unicorn
prefer-blob-reading-methods unicorn
prefer-code-point unicorn
prefer-date-now unicorn
prefer-dom-node-append unicorn
prefer-dom-node-dataset unicorn
prefer-dom-node-remove unicorn
prefer-event-target unicorn
prefer-math-trunc unicorn
prefer-native-coercion-functions unicorn
prefer-prototype-methods unicorn
prefer-query-selector unicorn
prefer-regexp-test unicorn
prefer-string-replace-all unicorn
prefer-string-slice unicorn
prefer-type-error unicorn
require-number-to-fixed-digits-argument unicorn

Style (80):

Code that should be written in a more idiomatic way.

Rule name Source Status
default-case-last eslint
default-param-last eslint
guard-for-in eslint
max-params eslint
no-continue eslint
no-multi-str eslint
no-script-url eslint
no-template-curly-in-string eslint
no-ternary eslint
prefer-exponentiation-operator eslint
sort-imports eslint
consistent-test-it jest
max-expects jest
max-nested-describe jest
no-alias-methods jest
no-confusing-set-timeout jest
no-deprecated-functions jest
no-done-callback jest
no-duplicate-hooks jest
no-hooks jest
no-identical-title jest
no-interpolation-in-snapshots jest
no-jasmine-globals jest
no-large-snapshots jest
no-mocks-import jest
no-restricted-jest-methods jest
no-restricted-matchers jest
no-test-prefixes jest
no-test-return-statement jest
no-untyped-mock-factory jest
prefer-called-with jest
prefer-comparison-matcher jest
prefer-equality-matcher jest
prefer-expect-resolves jest
prefer-hooks-on-top jest
prefer-jest-mocked jest
prefer-lowercase-title jest
prefer-mock-promise-shorthand jest
prefer-spy-on jest
prefer-strict-equal jest
prefer-to-be jest
prefer-to-contain jest
prefer-to-have-length jest
prefer-todo jest
require-hook jest
require-top-level-describe jest
no-set-state react
prefer-es-6-class react
adjacent-overload-signatures typescript
array-type typescript
ban-tslint-comment typescript
consistent-indexed-object-style typescript
consistent-type-definitions typescript
no-empty-interface typescript
prefer-for-of typescript
prefer-function-type typescript
catch-error-name unicorn
empty-brace-spaces unicorn
error-message unicorn
filename-case unicorn
no-await-expression-member unicorn
no-console-spaces unicorn
no-null unicorn
no-unreadable-array-destructuring unicorn
no-zero-fractions unicorn
number-literal-case unicorn
numeric-separators-style unicorn
prefer-array-flat-map unicorn
prefer-dom-node-text-content unicorn
prefer-includes unicorn
prefer-logical-operator-over-ternary unicorn
prefer-modern-dom-apis unicorn
prefer-optional-catch-binding unicorn
prefer-reflect-apply unicorn
prefer-spread unicorn
prefer-string-trim-start-end unicorn
require-array-join-separator unicorn
switch-case-braces unicorn
text-encoding-identifier-case unicorn
throw-new-error unicorn

Nursery (9):

New lints that are still under development.

Rule name Source Status
constructor-super eslint
getter-return eslint
no-undef eslint
no-unreachable eslint
export import
require-render-return react
rules-of-hooks react
no-side-effects-in-initialization tree_shaking
consistent-type-imports typescript

These tables were created by running oxlint --rules on commit ca0b4fa0.

@DonIsaac DonIsaac added C-enhancement Category - New feature or request good first issue Experience Level - Good for newcomers A-linter Area - Linter labels Jul 11, 2024
@Boshen Boshen changed the title linter: add, improve, and fix rule fixers linter: rule fixer improvements Jul 11, 2024
@Boshen Boshen pinned this issue Jul 11, 2024
Boshen pushed a commit that referenced this issue Jul 11, 2024
Part of  #4179.

Adds a fixer for some common typos.
Boshen pushed a commit that referenced this issue Jul 11, 2024
Part of #4179.

Fixes cases like:
```js
if (foo) debugger
// got fixed into
if (foo)
// but now gets fixed to
if (foo) {}
```
mysteryven pushed a commit that referenced this issue Jul 14, 2024
…#4244)

Part of #4179

add fixer for unicorn/no_useless_promise_resolve_reject
@jordan-boyer
Copy link
Contributor

jordan-boyer commented Jul 15, 2024

no-const-assign cannot have a fixer because we cannot infer the intend of the user .

Or we can add a dangerous fix that will replace the const with a let.
Tell me what you want on this and in can try to implement it

jelly added a commit to jelly/oxc that referenced this issue Jul 15, 2024
@jelly
Copy link
Contributor

jelly commented Jul 15, 2024

Related to fixes, can oxlint's diagnostic also print that a fix is available. Ruff for example shows:

[jelle@natrium][~/projects/oxc]%ruff check foo.py
foo.py:2:8: F401 [*] `os` imported but unused
  |
1 | #/usr/bin/python
2 | import os
  |        ^^ F401
  |
  = help: Remove unused import: `os`

Found 1 error.
[*] 1 fixable with the `--fix` option.

oxlint prints no information at the moment:

[jelle@natrium][~/projects/oxc]%cargo run --bin oxlint --  -A all -D prefer-string-trim-start-end foo.js
   Compiling oxc_linter v0.6.0 (/home/jelle/projects/oxc/crates/oxc_linter)
   Compiling oxlint v0.6.0 (/home/jelle/projects/oxc/apps/oxlint)
    Finished `dev` profile [unoptimized] target(s) in 3.69s
     Running `target/debug/oxlint -A all -D prefer-string-trim-start-end foo.js`

  × eslint-plugin-unicorn(prefer-string-trim-start-end): Prefer `trimStart` over `trimLeft`
    ╭─[foo.js:34:5]
 33 │ const foo = "lol";
 34 │ foo.trimLeft();
    ·     ────────
    ╰────
  help: Replace with `trimStart`

Finished in 4ms on 1 file with 1 rules using 16 threads.
Found 0 warnings and 1 error.

@DonIsaac
Copy link
Collaborator Author

@jelly check out my PR with suggestions; it does this.

@DonIsaac
Copy link
Collaborator Author

@jelly check out my PR with suggestions; it does this.

#4223

jelly added a commit to jelly/oxc that referenced this issue Jul 17, 2024
Adding tests for the fixer uncovered some bugs where it would produce
the wrong results or invalid JavaScript.

Related: oxc-project#4179
@jelly
Copy link
Contributor

jelly commented Jul 17, 2024

I think another task should be added, rules with fixers without tests. Last night I discovered no-zero-fractions currently produces broken code. See the test cases ff72fcd

  × eslint-plugin-unicorn(no-zero-fractions): Don't use a dangling dot in the number.
   ╭─[bar.js:7:14]
 6 │ const foo4 = -1.e+10;
 7 │ const foo5 = 1.e10;
   ·              ─────
 8 │ const foo6 = .0;
   ╰────
  help: Replace the number literal with `110`

While:

> 1.e10
10000000000

I simply copied some of the failing test cases into a JS file, made a backup and ran eslint fix on it as unicorn sadly has no tests for the fixer it seems.

Collected the list of fixers without expect_fix:

  • crates/oxc_linter/src/rules/eslint/no_unsafe_negation.rs
  • crates/oxc_linter/src/rules/eslint/valid_typeof.rs
  • crates/oxc_linter/src/rules/jest/no_test_prefixes.rs
  • crates/oxc_linter/src/rules/jest/valid_title.rs
  • crates/oxc_linter/src/rules/typescript/ban_ts_comment.rs
  • crates/oxc_linter/src/rules/unicorn/no_null.rs
  • crates/oxc_linter/src/rules/unicorn/no_zero_fractions.rs
  • crates/oxc_linter/src/rules/unicorn/prefer_dom_node_text_content.rs

mysteryven pushed a commit that referenced this issue Jul 31, 2024
Add a `FIX` constant to `RuleMeta` that describes what kinds of auto
fixes a
rule can perform (if any). This PR also updates `declare_oxc_lint` to
accept a
fix capabilities field. Follow-up PRs in this stack will update existing
rules
and update other parts of the codebase to use this new field.

The end goal of this stack is to
1. automate creation of #4179 in a similar way we auto-update rule
progress,
2. use it in rule documentation pages when we eventually add lint rules
to the website
@heygsc
Copy link
Contributor

heygsc commented Aug 4, 2024

For eslint itself, some rules have fixers, while others do not.
For those eslint without fixers, is it not recommended to do it with oxc, or is it okay to add it as a unique feature of oxc?

@rzvxa
Copy link
Collaborator

rzvxa commented Aug 4, 2024

For eslint itself, some rules have fixers, while others do not. For those eslint without fixers, is it not recommended to do it with oxc, or is it okay to add it as a unique feature of oxc?

I'm not sure if I'm getting it right or not, So feel free to correct me.


If you are talking about adding new fixers where there isn't one in eslint(or other linting solutions), It is completely welcomed here, We want backward compatibility with eslint as a drop-in replacement for most parts; But as long as we are adding(and not subtracting) features, It is allowed with no restrictions whatsoever. As long as it makes sense and is useful.

Although for non-exiting fixers/linters, you'd have to write through tests - since there isn't any source to borrow the tests from - and it might need a more rigorous review process to make sure it is doing the intended behavior we have in mind and assess the risks for the fixer(in case it have to be a dangerous fix).

With that said if you ever think the change you have in mind might be controversial it'd be best to create an issue for it beforehand or ask for reviews early in the process to make sure there aren't any strong oppositions against it. The last thing we want is to allow contributors' time to go to waste.

Let me know if I didn't answer your question.

@heygsc
Copy link
Contributor

heygsc commented Aug 4, 2024

For itself, some rules have fixers, while others do not. For those without fixers, is it not recommended to do it with , or is it okay to add it as a unique feature of ?eslint``eslint``oxc``oxc

I'm not sure if I'm getting it right or not, So feel free to correct me.

If you are talking about adding new fixers where there isn't one in eslint(or other linting solutions), It is completely welcomed here, We want backward compatibility with eslint as a drop-in replacement for most parts; But as long as we are adding(and not subtracting) features, It is allowed with no restrictions whatsoever. As long as it makes sense and is useful.

Although for non-exiting fixers/linters, you'd have to write through tests - since there isn't any source to borrow the tests from - and it might need a more rigorous review process to make sure it is doing the intended behavior we have in mind and assess the risks for the fixer(in case it have to be a dangerous fix).

With that said if you ever think the change you have in mind might be controversial it'd be best to create an issue for it beforehand or ask for reviews early in the process to make sure there aren't any strong oppositions against it. The last thing we want is to allow contributors' time to go to waste.

Let me know if I didn't answer your question.

Yes, thank you. That's what I wanted to ask.
If we prioritize which one to do first, would it be recommended in Oxc's plan to start with existing fixers (such as in ESLint)?

@DonIsaac
Copy link
Collaborator Author

DonIsaac commented Aug 4, 2024

@heygsc it depends. I'd recommend starting there if you're just getting your feet wet. Porting existing fixers is definitely easier than writing new ones.

In some cases, rules intentionally do not have auto fixes for DX reasons. For example, imagine an unused import getting deleted when your editor saves a file. That would get really annoying quite quickly. For those cases, we've added infrastructure for suggestions and "dangerous" fixes, which do not get applied when --fix is used, but instead get their own CLI flags.

@heygsc
Copy link
Contributor

heygsc commented Aug 4, 2024

@heygsc it depends. I'd recommend starting there if you're just getting your feet wet. Porting existing fixers is definitely easier than writing new ones.

In some cases, rules intentionally do not have auto fixes for DX reasons. For example, imagine an unused import getting deleted when your editor saves a file. That would get really annoying quite quickly. For those cases, we've added infrastructure for suggestions and "dangerous" fixes, which do not get applied when --fix is used, but instead get their own CLI flags.

What you said makes sense. Thank you.

mysteryven added a commit that referenced this issue Aug 8, 2024
part of #4179

---------

Co-authored-by: wenzhe <mysteryven@gmail.com>
@Boshen Boshen unpinned this issue Sep 4, 2024
Boshen pushed a commit that referenced this issue Oct 22, 2024
…6732)

Relates to #4179

---------

Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-linter Area - Linter C-enhancement Category - New feature or request good first issue Experience Level - Good for newcomers
Projects
None yet
Development

No branches or pull requests

5 participants