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

feat(lint): add the ignoreRestSiblings option for lint/correctness/noUnusedVariables #5157

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

iamakulov
Copy link

@iamakulov iamakulov commented Feb 19, 2025

Summary

This pull request adds an ignoreRestSiblings option for lint/correctness/noUnusedVariables. The option is similar to its ESLint alternative: https://eslint.org/docs/latest/rules/no-unused-vars#ignorerestsiblings

The option defaults to true (unlike its ESLint alternative) to avoid introducing a breaking change. (I’d love it if we could flip the default to false in the next major – as that would allow stricter linting by default – but this is not my call to make.)

Closes: #5152

Note: I have zero experience with Rust, so I had to Cursor my way through this PR. The (new) tests do pass, but sorry if there are any glaring omissions!

Test Plan

Tests should pass, I guess?

@github-actions github-actions bot added A-Linter Area: linter L-JavaScript Language: JavaScript and super languages labels Feb 19, 2025
@ematipico
Copy link
Member

@Conaclos what do you think about the default value?

@iamakulov
Copy link
Author

Thanks for approving the CI run! I fixed linting/formatting checks. Two jobs (codegen and tests) are still falling.

Codegen

Codegen is obvious – but I can’t run it locally due to a silly error (which I can’t figure out how to fix), so I’d appreciate if you can do it:

$ cargo codegen-configuration
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.52s
     Running `target/debug/xtask_codegen configuration`
> rustfmt --version
rustfmt 1.8.0
Error: Failed to run rustfmt from toolchain 'stable'. Please run `rustup component add rustfmt --toolchain stable` to install it.

$ rustup component add rustfmt --toolchain stable
info: component 'rustfmt' for target 'aarch64-apple-darwin' is up to date

$ which rustfmt
/opt/homebrew/bin/rustfmt

Tests

I’m not sure why there’s such a diff between local and CI test runs, but I’m happy to hand-fix the snapshots if you say this is expected:

Screenshot 2025-02-19 at 14 07 23

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add a changeset?

@ematipico
Copy link
Member

$ which rustfmt
/opt/homebrew/bin/rustfmt

I don't usually use brew to handle rust on my machine. Rust has a tool called rustup that does everything without problems. https://www.rust-lang.org/tools/install

@iamakulov
Copy link
Author

I don't usually use brew to handle rust on my machine. Rust has a tool called rustup that does everything without problems. https://www.rust-lang.org/tools/install

Thanks! Replacing the brew-installed rust and rustup with the full version helped. I updated the codegen + addressed the review feedback.

@Conaclos
Copy link
Member

what do you think about the default value?

I think it makes sense of turning off ignoreRestSiblings by default.
This will match the behavior of ESLint (easier migration).
Also, restructuring and ignoring some properties seems as an edge case to me.
Making it opt-in seems the good call.

5 │

i Unused variables usually are result of incomplete refactoring, typos and other source of bugs.

Copy link
Member

@Conaclos Conaclos Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we could add a note indicating that users can ignore rest sibling with the ignoreRestSiblings option.
This could improve option discovery.
We could display this note only for rest siblings like in the current case.

Copy link

codspeed-hq bot commented Feb 19, 2025

CodSpeed Performance Report

Merging #5157 will not alter performance

Comparing iamakulov:no-unused-vars-rest (996fa33) with main (d739883)

Summary

✅ 94 untouched benchmarks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Linter Area: linter L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants