-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix equatable_if_let: don't suggest = in const context
#15482
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
base: master
Are you sure you want to change the base?
Conversation
4b19a31 to
96838e2
Compare
equatable_if_let: FP in const contextequatable_if_let: don't suggest = in const context
tests/ui/equatable_if_let.rs
Outdated
| //~^ ERROR: this pattern matching can be expressed using `matches!` | ||
|
|
||
| // FIXME:suggests `==` | ||
| //const _: u32 = if let Some(NonConstEq) = None { 0 } else { 1 }; |
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.
why on earth does this happen
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.
is_stable_const_fn doesn't handle this case at all.
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.
Do you know of a way to make it to?..
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'm guessing this part:
cx.tcx
.trait_of_assoc(def_id)
.and_then(|trait_def_id| cx.tcx.lookup_const_stability(trait_def_id))Needs to be changed to getting the container and not the trait.
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 haven't followed this discussion closely, but it might be something similar to what I had to do in 86beecc. Maybe this could get factored into a utility function taking both the trait id and the concrete type.
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 don't completely get that code if I'm being honest. If I understand correctly, it checks that:
- the given trait:
- defines one and only function
- and that function is
const
- the given type has at least one (non-blanket) impl of that trait
- if there are multiple such impls, checks that the implementation of the one and only function is
constas well
(1.i) is fine for assign_op_patterns, since the traits checked there only ever define one function. And it should be okay for PartialEq as well, since eq is its first method. But I think that is a potential footgun worth mentioning in the docs of this function if we do move it to clippy_utils
But am I understanding correctly that (3) is a bit too strict? After all, only the impl that applies to the given situation should have its constness considered -- but it's probably unrealistic to try and find out which impl that is, from inside Clippy. If my understanding is correct, I'd add this as a comment to the function.
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.
@samueltardieu WDYT?
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.
If the function were to be moved to clippy_utils, it would probably need to take the method name as well. As you noted, in the current case there is no need to do so, and we could even have used [0] here to get the only method.
And yes, 3 is stricter than necessary, but finding the right implementation to use in the general case would probably be difficult. The risk with the current solution is a false negative for the lint, which in the current case is not critical.
43721b0 to
d5a3511
Compare
194bcde to
283f89d
Compare
turns out `is_in_const_context` wasn't even supposed to be put in the main let-chain, since it's only relevant for the `is_structural_partial_eq` case Co-authored-by: yanglsh <[email protected]>
we've created it already, so might as well..
before this, the lint would suggest `==` even for types that aren't `const PartialEq` still doesn't work on nested types for some reason
283f89d to
815db0a
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
fixes #15376
changelog: [
equatable_if_let]: don't suggest=in const context