lint towards rejecting consts in patterns that do not implement PartialEq by Ral...
source link: https://github.com/rust-lang/rust/pull/115893
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
Conversation
Member
I think we definitely don't want to allow such consts, so even while the general plan around structural matching is up in the air, we can start the process of getting non-PartialEq matches out of the ecosystem.
Collaborator
r? @b-naber (rustbot has picked a reviewer for you, use r? to override) |
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
Collaborator
Some changes might have occurred in exhaustiveness checking cc @Nadrieril |
This comment has been minimized.
Member
Author
Crucially, tests/ui/rfcs/rfc-1445-restrict-constants-in-patterns/allow-use-behind-cousin-variant.rs is still accepted; this is the example where #62411 (indirect_structural_match) was initially over-eager. |
added T-lang Relevant to the language team, which will review and decide on the PR/issue. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). I-lang-nominated Indicates that an issue has been nominated for discussion during a lang team meeting.
labels
Member
Author
@rust-lang/lang I found a surprising gap in our pattern-related future-incompat lints, which lead to us still accepting (without a warning) some consts as patterns that don't even implement So the question for you is, are you okay with consts in patterns requiring at least a If you are curious about what the affected code looks like, here are some examples: On both cases, the reason the constant isn't |
Contributor
Checking in from the @rust-lang/lang meeting and unnominating: Our judgment is that this is a bug fix and only causes more warnings, so no FCP is required, and we are in favor of going forward. However, we were pretty confused about what the story is around constants and matching -- requiring a @rustbot labels -I-lang-nominated |
removed the I-lang-nominated Indicates that an issue has been nominated for discussion during a lang team meeting. label
Member
Author
Yeah I think a design meeting is due, also see here. Maybe we shouldn't add further warnings until that happened, to avoid having to take back the warning again. |
Member
Author
I started writing up a design meeting document and there good reasons for requiring |
This comment has been minimized.
Member
Regarding the lint's name, maybe it is too general? I assume you do not want to forbid code like this:
Where Would the name |
Member
Author
I renamed it to |
This comment has been minimized.
// Always check for `PartialEq`, even if we emitted other lints. (But not if there were |
||
// any errors.) This ensures it shows up in cargo's future-compat reports as well. |
||
if !self.type_has_partial_eq_impl(cv.ty()) { |
Member
Author
There is the potential alternative of checking for Eq
here instead of PartialEq
. This would then subsume the "disallow floats in patterns" lint as well. However with t-lang in the past having been disinclined to disallow float patterns, it's not clear that we want to require Eq
.
Contributor
@bors r+ let's land this and figure out the details of consts in patterns in a design meeting with T-lang |
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-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label).
labels
Contributor
☀️ Test successful - checks-actions |
Collaborator
Finished benchmarking commit (1f2bacf): 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 CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 631.784s -> 630.892s (-0.14%) |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
Successfully merging this pull request may close these issues.
None yet
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK