Skip to content
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

Clarify the 'use a constant in a pattern' error message #108548

Merged
merged 1 commit into from
Mar 28, 2023

Conversation

jamen
Copy link
Contributor

@jamen jamen commented Feb 28, 2023

use std::borrow::Cow;

const ERROR_CODE: Cow<'_, str> = Cow::Borrowed("23505");

fn main() {
    let x = Cow::from("23505");
    
    match x {
        ERROR_CODE => {}
    }
}
error: to use a constant of type `Cow` in a pattern, `Cow` must be annotated with `#[derive(PartialEq, Eq)]`
 --> src/main.rs:9:9
  |
9 |         ERROR_CODE => {}
  |         ^^^^^^^^^^

error: could not compile `playground` due to previous error

It seems helpful to link to StructuralEq in this message. I was a little confused, because Cow<'_, str> implements PartialEq and Eq, but they're not derived, which I learned is necessary for structural equality and using constants in patterns (thanks to the Rust community Discord server)

For tests, should I update every occurrence of this message? I see tests where this is still a warning and I'm not sure if I should update those.

@rustbot
Copy link
Collaborator

rustbot commented Feb 28, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon.

Please see the contribution instructions for more information.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 28, 2023
@jamen jamen marked this pull request as draft February 28, 2023 00:48
@compiler-errors
Copy link
Member

I don't think that this error message should mention StructuralEq, but regardless, it should be done as a separate "note:" section on the diagnostic, and not in the primary message of the error.

@jamen
Copy link
Contributor Author

jamen commented Feb 28, 2023

Ok I attempted that

@petrochenkov petrochenkov marked this pull request as ready for review February 28, 2023 10:02
@petrochenkov
Copy link
Contributor

CI is failing.
x.py test --bless tests/ui should update the affected test outputs.
@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 28, 2023
@jamen
Copy link
Contributor Author

jamen commented Feb 28, 2023

fixed an accidental edit. tests unaffected

@petrochenkov
Copy link
Contributor

@rustbot ready
r? @compiler-errors

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 1, 2023
@jamen
Copy link
Contributor Author

jamen commented Mar 2, 2023

I don't think that this error message should mention StructuralEq

Why not? To me the message explains the what but not the why. Seems like a helpful piece of information.

@compiler-errors
Copy link
Member

compiler-errors commented Mar 7, 2023

My only concern is I'm not sure if it's conventional to document never-stable, compiler implementation details in user-facing (non-nightly-feature-related) error messages...

Maybe someone else from @rust-lang/wg-diagnostics can help make a call here.

This also could really use a test demonstrating the note. otherwise r=me on the implementation though.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 7, 2023

I think we could mention StructualEq as "extended reading material", but in order to fix your issue, the more important thing to me is to have a note saying "the traits must be derived, manual impls are not sufficient" first.

@bors
Copy link
Contributor

bors commented Mar 12, 2023

☔ The latest upstream changes (presumably #108682) made this pull request unmergeable. Please resolve the merge conflicts.

@compiler-errors
Copy link
Member

This has a conflict

Would appreciate an additional note (maybe above the one you added) that mentions that manual impls of PartialEq and Eq are not sufficient

@compiler-errors compiler-errors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 14, 2023
@@ -315,7 +315,7 @@ mir_build_moved = value is moved into `{$name}` here
mir_build_union_pattern = cannot use unions in constant patterns

mir_build_type_not_structural =
to use a constant of type `{$non_sm_ty}` in a pattern, `{$non_sm_ty}` must be annotated with `#[derive(PartialEq, Eq)]`
to use a constant of type `{$non_sm_ty}` in a pattern, `{$non_sm_ty}` must be annotated with `#[derive(PartialEq, Eq)]`. the traits must be derived, manual `impl`s are not sufficient
Copy link
Member

@compiler-errors compiler-errors Mar 25, 2023

Choose a reason for hiding this comment

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

please make this into a note, not the primary message. same below.

also, why did you take away the link to Structural{Partial}Eq? That was fine, as long as it was paired with an additional note that explains what those traits means in practice.

@jamen jamen force-pushed the master branch 2 times, most recently from 3372da0 to 07d3077 Compare March 25, 2023 08:39
@estebank
Copy link
Contributor

Can we also make sure that there's a test exercising this message?

@jamen
Copy link
Contributor Author

jamen commented Mar 26, 2023

I see tests in tests/ui/consts/const_in_pattern but they're unaffected by adding notes (they are affected when changing the main message). How do I go about that?

@compiler-errors
Copy link
Member

You need to give the second note another name and declare it in the diagnostic struct -- the first is being overwritten. Fluent file slug names are not deduplicated.

@rustbot
Copy link
Collaborator

rustbot commented Mar 27, 2023

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@@ -6,6 +6,8 @@ LL | FOO_REF_REF => {},
|
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #62411 <https://github.com/rust-lang/rust/issues/62411>
= note: the traits must be derived, manual `impl`s are not sufficient
= note: see https://doc.rust-lang.org/stable/std/marker/trait.StructuralEq.html for details
= note: `-D indirect-structural-match` implied by `-D warnings`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

unsure why this appears in the middle of these other notes

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it really matters.

@compiler-errors
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 27, 2023

📌 Commit 73c34cb has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 27, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 28, 2023
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#108548 (Clarify the 'use a constant in a pattern' error message)
 - rust-lang#109565 (Improve documentation for E0223)
 - rust-lang#109661 (Fix LVI test post LLVM 16 update)
 - rust-lang#109667 (Always set `RUSTC_BOOTSTRAP` with `x doc`)
 - rust-lang#109669 (Update books)
 - rust-lang#109678 (Don't shadow the `dep_node` var in `incremental_verify_ich_failed`)
 - rust-lang#109682 (Add `#[inline]` to CStr trait implementations)
 - rust-lang#109685 (Make doc comment a little bit more accurate)
 - rust-lang#109687 (Document the heuristics IsTerminal uses on Windows)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a7c07cf into rust-lang:master Mar 28, 2023
@rustbot rustbot added this to the 1.70.0 milestone Mar 28, 2023
flip1995 pushed a commit to flip1995/rust that referenced this pull request Apr 6, 2023
Clarify the 'use a constant in a pattern' error message

```rs
use std::borrow::Cow;

const ERROR_CODE: Cow<'_, str> = Cow::Borrowed("23505");

fn main() {
    let x = Cow::from("23505");

    match x {
        ERROR_CODE => {}
    }
}
```
```
error: to use a constant of type `Cow` in a pattern, `Cow` must be annotated with `#[derive(PartialEq, Eq)]`
 --> src/main.rs:9:9
  |
9 |         ERROR_CODE => {}
  |         ^^^^^^^^^^

error: could not compile `playground` due to previous error
```

It seems helpful to link to StructuralEq in this message. I was a little confused, because `Cow<'_, str>` implements PartialEq and Eq, but they're not derived, which I learned is necessary for structural equality and using constants in patterns (thanks to the Rust community Discord server)

For tests, should I update every occurrence of this message? I see tests where this is still a warning and I'm not sure if I should update those.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants