fix equatable_if_let: don't suggest = in const context
#15482
Conversation
rustbot
added
the
S-waiting-on-review
clippy_lints/src/equatable_if_let.rs
Outdated
| @@ -105,6 +106,7 @@ impl<'tcx> LateLintPass<'tcx> for PatternEquality { | |||
| if let ExprKind::Let(let_expr) = expr.kind | |||
| && unary_pattern(let_expr.pat) | |||
| && !expr.span.in_external_macro(cx.sess().source_map()) | |||
| && !is_in_const_context(cx) | |||
There was a problem hiding this comment.
I initially wanted to add a check for the const_trait_impl check here, similar to how it's done in #15476, but it turns out that ~const PartialEq in particular also requires the const_cmp feature, and cx.tcx.features().const_cmp() resulted in an error about that method not existing.. I guess it's too new of a feature for the Clippy's version of nightly Rust to know about?
There was a problem hiding this comment.
Hi! I was working on this previously and sorry I forgot to claim it, so you can keep going.
There was a problem hiding this comment.
There is a helper called is_stable_const_fn you can use to check the Eq trait. Here is the changes:
fn is_structural_partial_eq<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, other: Ty<'tcx>, msrv: Msrv) -> bool {
if let Some(def_id) = cx.tcx.lang_items().eq_trait()
&& implements_trait(cx, ty, def_id, &[other.into()])
{
if !is_in_const_context(cx) {
return true;
}
if let Some(AssocItem { def_id, .. }) = cx.tcx.provided_trait_methods(def_id).next() {
return is_stable_const_fn(cx, *def_id, msrv);
}
return false;
}
false
}There was a problem hiding this comment.
Hi! Sorry for unknowingly stealing your work^^ And thanks for the tip! I think I was able to even build upon it a little
There was a problem hiding this comment.
Apparently provided_trait_methods isn't the right thing to use here -- for PartialEq, it returns ne (and only ne), which is fair because that method is the one that is provided, i.e. has a default implementation. What we need instead is probably cx.tcx.associated_items(def_id).in_definition_order().next(), assuming eq is defined first.
ada4a
force-pushed
the
equatable-if-in-consts
branch
from
4b19a31 to
96838e2
Compare
ada4a
changed the title
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.
why on earth does this happen
ada4a
force-pushed
the
equatable-if-in-consts
branch
from
43721b0 to
d5a3511
Compare
clippy_lints/src/equatable_if_let.rs
Outdated
| && let args = cx.tcx.mk_args(&[other.into()]) | ||
| && let Ok(Some(instance)) = Instance::try_resolve(cx.tcx, cx.typing_env(), eq_method, args) |
There was a problem hiding this comment.
The argument list should be [ty, other]. The first generic argument for traits is the self type.
There was a problem hiding this comment.
That didn't fix the error.. Pushed the change anyway
Also I wonder why I didn't get an ICE? I guess the missing Rhs type was filled in by the default Rhs=Self
ada4a
force-pushed
the
equatable-if-in-consts
branch
from
d5a3511 to
194bcde
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 <yanglsh@shanghaitech.edu.cn>
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
ada4a
force-pushed
the
equatable-if-in-consts
branch
from
194bcde to
283f89d
Compare
fixes #15376
changelog: [
equatable_if_let]: don't suggest=in const context