Suggest using equality comparison instead of pattern matching on non-structural constant in pattern#154010
Conversation
…strutural constant in pattern
```
error: constant of non-structural type `partial_eq::S` in a pattern
--> $DIR/suggest_equality_comparison_instead_of_pattern_matching.rs:16:18
|
LL | struct S;
| -------- `partial_eq::S` must be annotated with `#[derive(PartialEq)]` to be usable in patterns
...
LL | const C: S = S;
| ---------- constant defined here
...
LL | Some(C) => {}
| ^ constant of non-structural type
|
note: the `PartialEq` trait must be derived, manual `impl`s are not sufficient; see https://doc.rust-lang.org/stable/std/marker/trait.StructuralPartialEq.html for details
--> $DIR/suggest_equality_comparison_instead_of_pattern_matching.rs:5:5
|
LL | impl PartialEq<S> for S {
| ^^^^^^^^^^^^^^^^^^^^^^^
help: add a condition to the match arm checking for equality
|
LL - Some(C) => {}
LL + Some(binding) if binding == C => {}
|
```
|
Some changes occurred in match checking cc @Nadrieril |
|
r? @nnethercote rustbot has assigned @nnethercote. Use Why was this reviewer chosen?The reviewer was selected based on:
|
…ural type const
```
error: constant of non-structural type `partial_eq::S` in a pattern
--> $DIR/suggest_equality_comparison_instead_of_pattern_matching.rs:22:18
|
LL | struct S;
| -------- `partial_eq::S` must be annotated with `#[derive(PartialEq)]` to be usable in patterns
...
LL | const C: S = S;
| ---------- constant defined here
...
LL | let Some(C) = Some(S) else { return; };
| ^ constant of non-structural type
|
note: the `PartialEq` trait must be derived, manual `impl`s are not sufficient; see https://doc.rust-lang.org/stable/std/marker/trait.StructuralPartialEq.html for details
--> $DIR/suggest_equality_comparison_instead_of_pattern_matching.rs:5:5
|
LL | impl PartialEq<S> for S {
| ^^^^^^^^^^^^^^^^^^^^^^^
help: check for equality instead of pattern matching
|
LL - let Some(C) = Some(S) else { return; };
LL + if Some(C) == Some(S) { return; };
|
```
|
Can you update the PR description to include more detail? I've read #42753, but links to issues aren't always helpful because they often include extraneous details. I also don't know how to interpret the two error outputs in the PR description. Are they new error messages? If so, how do they compare to the old error messages? I don't have enough context to proceed. Thanks. @rustbot author |
|
Reminder, once the PR becomes ready for a review, use |
|
@nnethercote added some more context to the description, let me know if that sufficient, otherwise I can try to expand further. |
| | | ||
| LL | pub struct CustomEq; | ||
| | ------------------- `CustomEq` must be annotated with `#[derive(PartialEq)]` to be usable in patterns | ||
| | ------------------- `CustomEq` is not usable in patterns |
There was a problem hiding this comment.
This non-local case is tricky, because the other crate might be under the author's control and able to be changed. I guess it's impossible to know for sure.
There was a problem hiding this comment.
I was going back and forth on this one. I settled on "we're giving them a link to the docs". I was concerned with giving the impression to the user that they had to somehow fork their dep. We could instead say something like "CustomEq could be used in patterns if it was annotated with #[derive(PartialEq)]", but I fear that has the same problem.
| help: add a condition to the match arm checking for equality | ||
| | | ||
| LL - BAR_BAZ => panic!(), | ||
| LL + binding if binding == BAR_BAZ => panic!(), |
There was a problem hiding this comment.
It's a bit confusing that the error message suggests two separate ways of fixing this -- add derive(PartialEq) and use a match arm condition. Could the one-or-the-other nature be made clearer? Maybe by inserting "alternatively, " at the start of the help message?
There was a problem hiding this comment.
Ah, I was looking on how to address this. I'll follow up.
|
@bors r+ |
This comment has been minimized.
This comment has been minimized.
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 6d8bc65 (parent) -> 3ea2fbc (this PR) Test differencesShow 5 test diffsStage 1
Stage 2
Additionally, 3 doctest diffs were found. These are ignored, as they are noisy. Job group index
Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 3ea2fbcb2a872b7e1fa3cada256f8e97f9e6636f --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (3ea2fbc): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (primary 0.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 3.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 483.836s -> 485.545s (0.35%) |
When encountering a pattern containing a non-structural constant (not marked as
#[derive(PartialEq)]to make it suitable for pattern matching,Cin the examples below), we would previously not provide additional guidance. With this PR, thehelpin the following examples are added:The suggestion accounts for a few conditions:
PartialEqimpl, the user can't make it structural, so we don't provide the suggestionPartialEq, explain that with a derivedPartialEqyou could use equalityPartialEqand use equality check instead of pattern matchingif-letto suggest chaining (edition dependent),matcharm with a presentifcheck,matcharm without an existingifchecklet-else, we suggest turning it into anifexpression instead (this doesn't check for additional bindings beyond the constant, which would suggest incorrect code in some more complex cases).Fix #42753.