-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Honor avoid-breaking-exported-api
in needless_pass_by_ref_mut
#11647
Honor avoid-breaking-exported-api
in needless_pass_by_ref_mut
#11647
Conversation
r? @Centri3 (rustbot has picked a reviewer for you, use r? to override) |
That's actually a good question... The API changed, so I suppose it's breaking semver but it doesn't break it for the API users. Interesting. |
The function will no longer be Don't have any examples of this but it's probably possible when passing the function to some iterator methods, like |
Huh, isn't this only about what the function captures? But I think you're hinting to the right thing: You won't be able to pass the function as an argument to another function, expecting a I will run |
So lintcheck gave some interesting discoveries: Passing the function is only a semi-issue: You can work around that by doing something like Further than that, the Here are the results:
Reports
Stats:
In conclusion, I would say that we don't want to break public API, because passing a function with |
I'm not sure whether that's actually possible by just changing the signature, since yeah that is based on what it captures. It might just be that any function implements What you showed is a larger concern though which I forgot about and is already checked for by the lint for cases of this in the current crate. This might be rare enough that we can just always give a note on public items, I can't really think of any non-contrived example where somebody passes a function pointer to smth expecting |
Ah right, I forgot about that. This is good, but on public items we can't tell. So we should definitely emit a warning there, document it in the lint documentation and apply the exported API config correctly IMO.
See my comment above with an example from the |
This would not be linted as it has an If it's passed to another function that's both "safe" and takes |
You are way ahead of me :D You're right. |
I'll add the warning and documentation about this in this PR and then mark it as ready for review some time this week. (I'll be at EuroRust, where I might get some time for this and other Clippy TODOs I still have) |
For the PS: I'll be at eurorust too. :D |
☔ The latest upstream changes (presumably #11622) made this pull request unmergeable. Please resolve the merge conflicts. |
46de00f
to
feabebc
Compare
@Centri3 rebased and ready for final review. |
feabebc
to
63b69fe
Compare
☔ The latest upstream changes (presumably #11685) made this pull request unmergeable. Please resolve the merge conflicts. |
avoid-breaking-exported-api
in needless_pass_by_ref_mut
Seems this only needs to be blessed, after that this is ready |
The failing test has a |
63b69fe
to
c5bf27a
Compare
☔ The latest upstream changes (presumably #11879) made this pull request unmergeable. Please resolve the merge conflicts. |
I've spent some time investigating this. fn bar() {} // or a `mod bar {}` or a `use` item
fn foo() {}
async fn _f(v: &mut Vec<()>) {
let x = || v.pop();
_ = || || x;
} When removing anything from that reproducer, like the functions So it seems like, in order to hit this, there must be at least 2 items before |
c5bf27a
to
b26c6a3
Compare
I completely forgot about it. ^^' Putting it back to my to do list. |
Sorry, I totally forgot about this PR, since it wasn't in my inbox anymore 🙈 I'll add it to my todo list. You'll get a review next week! |
I totally missed this, since it wasn't in my inbox 🙈 I'll try to review it tomorrow or otherwise early next week. Also ping @GuillaumeGomez, if you still want to help with the review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the long review time, it wasn't in my inbox 🙈
Anyhow, one small nit, but the current version also looks good to me.
let is_exported = cx.effective_visibilities.is_exported(*fn_def_id); | ||
if self.avoid_breaking_exported_api && is_exported { | ||
continue; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be cleaner to have this check in check_fn
where the function is added to NeedlessPassByRefMut::fn_def_ids_to_maybe_unused_mut
. (Or where it wouldn't be added if it's exported and the config is set) But this is a viable position as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also rebase the branch? :) |
Until now, the lint only emitted a warning, when breaking public API. Now it doesn't lint at all when the config value is not set to `false`, bringing it in line with the other lints using this config value. Also ensures that this config value is documented in the lint.
b26c6a3
to
125c778
Compare
Rebased and this weird bug: #11647 (comment) is also gone. Infinite monkey problem solving never disappoints! |
LGTM, thank you for the quick update =^.^= Now, let's give our favorite non money something to do. Roses are red, |
…Frednet Honor `avoid-breaking-exported-api` in `needless_pass_by_ref_mut` Until now, the lint only emitted a warning, when breaking public API. Now it doesn't lint at all when the config value is not set to `false`, bringing it in line with the other lints using this config value. Also ensures that this config value is documented in the lint. changelog: none (I don't think a changelog is necessary, since this lint is in `nursery`) --- Fixes #11374 cc `@GuillaumeGomez` Marking as draft: Does this lint even break public API? If I change a function signature from `fn foo(x: &mut T)` to `fn foo(x: &T)`, I can still call it with `foo(&mut x)`. The only "breaking" thing is that the `clippy::unnecessary_mut_passed` lint will complain that `&mut` at the callsite is not necessary, possibly trickling down to the crate user having to remote a `mut` from a variable. [Playground](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=058165a7663902e84af1d23e35c10d66). Are there examples where this actually breaks public API, that I'm missing?
💔 Test failed - checks-action_test |
Could you run |
@bors r=xFrednet Thanks for the review! |
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
With this merged, I think we can move this lint out of |
None that I heard of at least. |
It triggers quite a bit on the 400 most popular crates. I still have to look into the source, whether any of those are FPs. Going through the first 10 or so I found two issues: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=90b85826ab9d38fed7ff9657f7d24287
clippy 0.1.81 (f7f0cd395c0 2024-07-02) Reports
Stats:
|
Until now, the lint only emitted a warning, when breaking public API. Now it doesn't lint at all when the config value is not set to
false
, bringing it in line with the other lints using this config value.Also ensures that this config value is documented in the lint.
changelog: none
(I don't think a changelog is necessary, since this lint is in
nursery
)Fixes #11374
cc @GuillaumeGomez
Marking as draft: Does this lint even break public API? If I change a function signature from
fn foo(x: &mut T)
tofn foo(x: &T)
, I can still call it withfoo(&mut x)
. The only "breaking" thing is that theclippy::unnecessary_mut_passed
lint will complain that&mut
at the callsite is not necessary, possibly trickling down to the crate user having to remote amut
from a variable. Playground.Are there examples where this actually breaks public API, that I'm missing?