Skip to content

Ignore false positive only_used_in_recursion Clippy warning #15635

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

Merged
merged 2 commits into from
Apr 9, 2025

Conversation

DerGut
Copy link
Contributor

@DerGut DerGut commented Apr 8, 2025

Which issue does this PR close?

Ignores a Clippy warning identified in #15625. Note, that this warning seems to be a false positive. I've submitted an issue with the Clippy project rust-lang/rust-clippy#14566. Ideally we'd get this fixed upstream instead of having to add this ignore directive.

Rationale for this change

As mentioned in #15625 (review), we get a Clippy warning

warning: parameter is only used in recursion
  --> datafusion/physical-optimizer/src/aggregate_statistics.rs:48:9
   |
48 |         config: &ConfigOptions,
   |         ^^^^^^ help: if this is intentional, prefix it with an underscore: `_config`
   |
note: parameter used here
  --> datafusion/physical-optimizer/src/aggregate_statistics.rs:86:42
   |
86 |                     self.optimize(child, config).map(Transformed::yes)
   |                                          ^^^^^^
...
91 |             plan.map_children(|child| self.optimize(child, config).map(Transformed::yes))
   |                                                            ^^^^^^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#only_used_in_recursion
   = note: `#[warn(clippy::only_used_in_recursion)]` on by default

when linting the datafusion-physical-optimizer crate: cargo clippy -p datafusion-physical-optimizer.

What changes are included in this PR?

No real change was possible, so I added an ignore directive.

Clippy suggests to prefix the config parameter with an underscore but it is still used in the function body to pass it to recursive calls. Removing the parameter was also not an option because it is defined on the PhysicalOptimizerRule trait and other implementations use it.

Are these changes tested?

Running cargo clippy -p datafusion-physical-optimizer now reports no warnings.

Are there any user-facing changes?

No, no actual code change.

@github-actions github-actions bot added the optimizer Optimizer rules label Apr 8, 2025
@alamb
Copy link
Contributor

alamb commented Apr 8, 2025

Thanks @DerGut -- rust-lang/rust-clippy#14566 is an interesting report

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @DerGut -- this is a great contribution

@@ -42,6 +42,7 @@ impl AggregateStatistics {

impl PhysicalOptimizerRule for AggregateStatistics {
#[cfg_attr(feature = "recursive_protection", recursive::recursive)]
#[allow(clippy::only_used_in_recursion)] // See https://github.com/rust-lang/rust-clippy/issues/14566
Copy link
Contributor

Choose a reason for hiding this comment

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

💯 for leaving context in comment

@alamb alamb merged commit 68475eb into apache:main Apr 9, 2025
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
optimizer Optimizer rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants