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

Enable used_underscore_binding clippy lint #14649

Open
findepi opened this issue Feb 13, 2025 · 5 comments
Open

Enable used_underscore_binding clippy lint #14649

findepi opened this issue Feb 13, 2025 · 5 comments
Assignees
Labels
good first issue Good for newcomers

Comments

@findepi
Copy link
Member

findepi commented Feb 13, 2025

_var is a convention to indicate unused variables or unused struct members.
As such, these members should indeed not be used.

used_underscore_binding clippy lint seems to be enforcing that. let's enable it in the workspace

[workspace.lints.clippy]

@findepi findepi added the good first issue Good for newcomers label Feb 13, 2025
@ding-young
Copy link
Contributor

take

@ding-young
Copy link
Contributor

ding-young commented Feb 19, 2025

Hi, @findepi
I've set used_underscore_binding=deny and renamed each _var that was actually used or passed down as fn arg to var. However, this gives another clippy warning only_used_in_recursion in following code.

fn rewrite(
&self,
plan: LogicalPlan,
_config: &dyn OptimizerConfig,
) -> Result<Transformed<LogicalPlan>> {

There are few cases like above where replacing _config with config gives only_used_in_recursion warning, which says I should go back and prefix it with an underscore: _config. I can simply put #[allow(clippy::used_underscore_binding)] on these cases, but suppressing the lint doesn't feel like a clean solution.

Do you have any suggestions about this issue? Thank you :)

@findepi
Copy link
Member Author

findepi commented Feb 19, 2025

We could move logic of PushDownLimit::rewrite to make it inherent method (impl PushDownLimit block).
This way we could recurse without threading useless config parameter. It would be slightly better in a sense that it would indeed use less stack space to recurse.
Or we can simply suppress one of the conflicting lints:

#[allow(clippy::used_underscore_binding)] // for recursive call only
self.rewrite(new_filter, _config)

@Ramjee194
Copy link

can you assign this issue #14649

@ding-young
Copy link
Contributor

Hi @Ramjee194, I assigned myself with writing "take" (you can also do this for other issues, too!) and already working on this issue. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants