DonIsaac commented 1 month ago

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

[x] #4187
[x] #4211
https://github.com/oxc-project/oxlint-ecosystem-ci/issues/19

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.

jordan-boyer commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

DonIsaac commented 1 month ago

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

4223

jelly commented 1 month ago

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 https://github.com/oxc-project/oxc/commit/ff72fcd3ea03f57dfaf6ae21fa326d5981f68285

  × 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

heygsc commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

@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 commented 2 weeks ago

@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.

oxc-project / oxc

linter: rule fixer improvements #4179

Prerequisites

Rules

Legend

4223