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

RFC: constants in patterns #3535

Merged
merged 9 commits into from
Jan 26, 2024

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Nov 25, 2023

When a constant appears as a pattern, this is syntactic sugar for writing a pattern that corresponds to the constant's value by hand. This operation is only allowed when (a) the type of the constant implements PartialEq, and (b) the value of the constant being matched on has "structural equality", which means that PartialEq behaves the same way as that desugared pattern.

Thanks a lot to everyone who gave feedback in the Zulip discussion!

Rendered

text/0000-constants-in-patterns.md Outdated Show resolved Hide resolved
text/0000-constants-in-patterns.md Outdated Show resolved Hide resolved
text/0000-constants-in-patterns.md Outdated Show resolved Hide resolved
Co-authored-by: Eric Huss <eric@huss.org>
@jhpratt
Copy link
Member

jhpratt commented Nov 30, 2023

While it is true that not having StructuralPartialEq be implied by #[derive(PartialEq)] would be massively breaking, I feel that requiring it be separate as soon as practicable (i.e. the next edition) would be ideal. Having a StructuralPartialEq implementation is naturally an API commitment, so it should be explicit insofar as it is possible to do so.

Aside from that and permitting manual implementations of StructuralPartialEq (which is mentioned in the RFC), I'm quite happy with the way everything is described.

@RalfJung
Copy link
Member Author

It's opening a new design space though; t-lang expressed concerns that we already have way too long derive(...) declarations and should look into something like derive(Structural) to get "all the usual derives" or so. This RFC does not aim to solve all the problems in this space, it is an incremental step, and IMO decoupling the structural-equality-promise from #[derive(PartialEq)] is not part of that step. I want to push back against scope creep here.

@jhpratt
Copy link
Member

jhpratt commented Nov 30, 2023

My primary fear is that people will be making API commitments (and then breaking them) without even realizing it. It's a similar problem to traits being object safe without an explicit opt-in. If this existed previously, I would have broken it multiple times deliberately, as it's something I never intended to promise. I know I have done this for object safety, hoping1 that no one would notice.

I understand the concerns over long derive declarations, even if I personally disagree. Was that a significant factor in weighing the pros & cons of each option?

Footnotes

  1. successfully

@RalfJung
Copy link
Member Author

RalfJung commented Nov 30, 2023

The issue you describe is pre-existing. I agree it is an issue. This RFC does not aim to solve it since it is entirely orthogonal to what the RFC aims to solve. (This RFC just needs some mechanism for type authors to opt-in to "structural equality". The details of that mechanism do not matter. We have an existing mechanism for this. It is flawed but this RFC does not make it worse.) The RFC already acknowledges this problem and mentions it as a "future possibility". There are other real issues that this RFC aims to solve and I don't think we need to unnecessarily entangle this.

Let's not require an RFC that clearly has net benefit to solve every problem in its space, please. This is just a surefire way to ensure nobody will dare suggest RFCs any more. We need room for incremental RFCs that solve a particular issue without also solving closely related (but entirely orthogonal) problems. It has already taken literal years to arrive at this RFC.

@jhpratt
Copy link
Member

jhpratt commented Nov 30, 2023

The issue you describe is pre-existing.

Code that runs into that can be written on stable? I was under the implication that it was nightly-only, where the traits currently live. If it can already be encountered on stable, then sure, I have no issue with continuing as-written.

Let's not require an RFC that clearly has net benefit to solve every problem in its space, please. This is just a surefire way to ensure nobody will dare suggest RFCs any more. We need room for incremental RFCs that solve a particular issue without also solving closely related (but entirely orthogonal) problems. It has already taken literal years to arrive at this RFC.

I know that it's not easy writing an RFC, let along "defending" it. If my understanding is not correct, then feel free to say so directly 🙂 I'm sure I'm not the only one with this question.

@RalfJung
Copy link
Member Author

RalfJung commented Nov 30, 2023

I didn't realize you assumed that this is nightly-only behavior. :)

This RFC does not allow any new code to work on stable. It only turns some future-compat lints into hard errors.

Comment on lines +293 to +296
- To avoid interpreting `derive(PartialEq)` as a semver-stable promise of being able to structurally match this type,
we could introduce an explicit `derive(PartialEq, StructuralPartialEq)`.
However, that would be a massive breaking change, so it can only be done over an edition boundary.
It probably would also want to come with some way to say "derive me all the usual traits" so that one does not have to spell out so many trait names.
Copy link
Contributor

Choose a reason for hiding this comment

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

One potential alternative to this is to allow negative impls for StructuralPartialEq for derived PartialEq, and/or perhaps to allow typing derive(PartialEq, !StructuralPartialEq).

If we think of StructuralPartialEq "like an auto trait" since it's infectious, this should make some amount of sense.

@RalfJung
Copy link
Member Author

RalfJung commented Dec 2, 2023

Given that we already had a lang design meeting that reached pretty much this conclusion, I am going to nominate this for t-lang discussion.

@RalfJung RalfJung added the I-lang-nominated Indicates that an issue has been nominated for prioritizing at the next lang team meeting. label Dec 2, 2023
text/0000-constants-in-patterns.md Outdated Show resolved Hide resolved
Comment on lines +83 to +84
For matching on values of enum type, it is sufficient if the actually chosen
variant has structural equality; other variants do not matter:
Copy link

Choose a reason for hiding this comment

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

It seems somewhat unfortunate that validity in patterns is value sensitive and not purely type directed. It's at least excusable with floats, since the behavior of NaN is reasonably well known to be non-total, but for other types it certainly surprised me that this was already the case.

Given it's already the case and it's not like it isn't already formally API breaking to change the value of a const item for multiple other reasons already, it's fine, it just surprised me (and I was writing a PR review with other useful comments anyway).

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if it's unfortunate, people certainly like and use it.

- The biggest drawback of this proposal is that it conflates `derive(PartialEq)` with a semver-stable promise that this type will always have structural equality.
Once a type has `derive(PartialEq)`, it may appear in patterns, so replacing this `PartialEq` by a custom implementation is a breaking change.
Once the `StructuralPartialEq` trait is stable, `derive(PartialEq)` *can* be replaced by a custom implementation as long as one also implements `StructuralPartialEq`,
but that entails a promise that the `impl` still behaves structurally, including on all private fields.
Copy link

Choose a reason for hiding this comment

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

(It would be breaking, which means we almost certainly shouldn't do it, but) Given the logic that a const pattern is “logically desugared” into the corresponding literal pattern, it could be justified in forbidding matching over any type with inaccessible fields (or #[non_exhaustive]).

(Fun: because this sees into private fields, you can exhaustively match a type with all private fields. I unfortunately don't have my set up to test multiple crates right now, but I will be extremely amused if it turns out you can exhaustively match a #[non_exhaustive] struct. Downright dtolnay/rust-quiz material.)

Copy link
Member Author

Choose a reason for hiding this comment

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

it could be justified in forbidding matching over any type with inaccessible fields (or #[non_exhaustive]).

This is somewhat deliberate I think: a library can choose to expose certain constants that people can match on despite there being private fields.

I don't think this will let you do exhaustive matching on non_exhaustive enums. For structs there's no separate notion of exhaustiveness anyway, it's all per-field?

Copy link
Member

Choose a reason for hiding this comment

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

For structs and struct variants, #[non_exhaustive] means "I may add fields in the future so you must always deconstruct with , ..". That doesn't change anything for constants-interpreted-as-patterns. I can't find a test for that case though, I'll add one when I get the chance

Copy link
Member

@Nemo157 Nemo157 Dec 17, 2023

Choose a reason for hiding this comment

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

I will be extremely amused if it turns out you can exhaustively match a #[non_exhaustive] struct.

You can 🤦

// bar/src/lib.rs
#[non_exhaustive]
#[derive(PartialEq, Eq)]
pub struct Bar(bool);
pub const TRUE: Bar = Bar(true);
pub const FALSE: Bar = Bar(false);
// foo/src/main.rs
fn main() {
    match bar::TRUE {
        bar::TRUE => {}
        bar::FALSE => {}
    }
}
> cargo check
    Checking bar v0.1.0 (/tmp/scratch.rust.2023-12-17T12-41.GFlTlw/foo/bar)
    Checking foo v0.1.0 (/tmp/scratch.rust.2023-12-17T12-41.GFlTlw/foo)
    Finished dev [unoptimized + debuginfo] target(s) in 0.11s

Copy link
Member Author

Choose a reason for hiding this comment

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

That doesn't seem surprising to me, since this does not deconstruct the value at all, it just compares for equality.

But if this should lint/error then please file an issue.

Copy link

@Skgland Skgland Dec 17, 2023

Choose a reason for hiding this comment

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

I don't think that should error. #[non_exhaustive] means "adding fields musn't be a breaking change", which is the case when using constants. The tricky case is accessing private fields, which is addressed in your RFC.

How would you add a non-unit type field without breaking the match?
Adding such fields would add more possible states, that would not be coverable by the two existing constant patterns.

Copy link
Member

Choose a reason for hiding this comment

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

Oh hm fair point. Maybe it should be opt-in like for private fields

Copy link
Member

Choose a reason for hiding this comment

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

This appears to be a new (at least to me) semver hazard. I think it'd be very surprising to maintainers that it's possible to cause a breaking change by adding/tweaking a private field on a #[non_exhaustive] type.

I think this should either be a future-incompat warning -> error, or at least a lint. If one of the folks here who discovered this issue agrees and would like to open an issue for that, please tag me -- I'd like to keep on top of it for the sake of cargo-semver-checks. If nobody is interested in opening that issue, I'd be happy to do it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah please link the issue from here once it got created. :)

Copy link

Choose a reason for hiding this comment

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

As I looks like no has opened an issue yet, I have opened rust-lang/rust#119264

@clarfonthey
Copy link
Contributor

While I know it's not a priority and not required for the initial RFC, I think that StructuralOrd should at least be mentioned in the future possibilities. For example, being able to do Jan..=Mar patterns for a custom month enum would be nice, and it isn't incompatible with the current proposal.

The main difference is that while matching enum variants for equality doesn't require StructuralEq, ranges would require StructuralOrd.

@rfcbot
Copy link
Collaborator

rfcbot commented Jan 14, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@traviscross
Copy link
Contributor

We've accepted this RFC and it has now been merged.

Huge thanks to @RalfJung for doing the detailed work necessary to put this together and to move Rust forward.

We've opened a tracking issue over in rust-lang/rust#120362. Keep an eye on that issue for future updates.

@RalfJung RalfJung deleted the constants-in-patterns branch January 27, 2024 12:25
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 3, 2024
make matching on NaN a hard error, and remove the rest of illegal_floating_point_literal_pattern

These arms would never be hit anyway, so the pattern makes little sense. We have had a future-compat lint against float matches in general for a *long* time, so I hope we can get away with immediately making this a hard error.

This is part of implementing rust-lang/rfcs#3535.

Closes rust-lang#41620 by removing the lint.

rust-lang/reference#1456 updates the reference to match.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 3, 2024
make matching on NaN a hard error, and remove the rest of illegal_floating_point_literal_pattern

These arms would never be hit anyway, so the pattern makes little sense. We have had a future-compat lint against float matches in general for a *long* time, so I hope we can get away with immediately making this a hard error.

This is part of implementing rust-lang/rfcs#3535.

Closes rust-lang#41620 by removing the lint.

rust-lang/reference#1456 updates the reference to match.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 4, 2024
…, r=petrochenkov

update indirect structural match lints to match RFC and to show up for dependencies

This is a large step towards implementing rust-lang/rfcs#3535.
We currently have five lints related to "the structural match situation":
- nontrivial_structural_match
- indirect_structural_match
- pointer_structural_match
- const_patterns_without_partial_eq
- illegal_floating_point_literal_pattern

This PR concerns the first 3 of them. (The 4th already is set up to show for dependencies, and the 5th is removed by rust-lang#116284.) nontrivial_structural_match is being removed as per the RFC; the other two are enabled to show up in dependencies.

Fixes rust-lang#73448 by removing the affected analysis.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 4, 2024
…, r=petrochenkov

update indirect structural match lints to match RFC and to show up for dependencies

This is a large step towards implementing rust-lang/rfcs#3535.
We currently have five lints related to "the structural match situation":
- nontrivial_structural_match
- indirect_structural_match
- pointer_structural_match
- const_patterns_without_partial_eq
- illegal_floating_point_literal_pattern

This PR concerns the first 3 of them. (The 4th already is set up to show for dependencies, and the 5th is removed by rust-lang#116284.) nontrivial_structural_match is being removed as per the RFC; the other two are enabled to show up in dependencies.

Fixes rust-lang#73448 by removing the affected analysis.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 5, 2024
…, r=petrochenkov

update indirect structural match lints to match RFC and to show up for dependencies

This is a large step towards implementing rust-lang/rfcs#3535.
We currently have five lints related to "the structural match situation":
- nontrivial_structural_match
- indirect_structural_match
- pointer_structural_match
- const_patterns_without_partial_eq
- illegal_floating_point_literal_pattern

This PR concerns the first 3 of them. (The 4th already is set up to show for dependencies, and the 5th is removed by rust-lang#116284.) nontrivial_structural_match is being removed as per the RFC; the other two are enabled to show up in dependencies.

Fixes rust-lang#73448 by removing the affected analysis.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 5, 2024
make matching on NaN a hard error, and remove the rest of illegal_floating_point_literal_pattern

These arms would never be hit anyway, so the pattern makes little sense. We have had a future-compat lint against float matches in general for a *long* time, so I hope we can get away with immediately making this a hard error.

This is part of implementing rust-lang/rfcs#3535.

Closes rust-lang#41620 by removing the lint.

rust-lang/reference#1456 updates the reference to match.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 5, 2024
…, r=petrochenkov

update indirect structural match lints to match RFC and to show up for dependencies

This is a large step towards implementing rust-lang/rfcs#3535.
We currently have five lints related to "the structural match situation":
- nontrivial_structural_match
- indirect_structural_match
- pointer_structural_match
- const_patterns_without_partial_eq
- illegal_floating_point_literal_pattern

This PR concerns the first 3 of them. (The 4th already is set up to show for dependencies, and the 5th is removed by rust-lang#116284.) nontrivial_structural_match is being removed as per the RFC; the other two are enabled to show up in dependencies.

Fixes rust-lang#73448 by removing the affected analysis.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 5, 2024
make matching on NaN a hard error, and remove the rest of illegal_floating_point_literal_pattern

These arms would never be hit anyway, so the pattern makes little sense. We have had a future-compat lint against float matches in general for a *long* time, so I hope we can get away with immediately making this a hard error.

This is part of implementing rust-lang/rfcs#3535.

Closes rust-lang#41620 by removing the lint.

rust-lang/reference#1456 updates the reference to match.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 5, 2024
…, r=petrochenkov

update indirect structural match lints to match RFC and to show up for dependencies

This is a large step towards implementing rust-lang/rfcs#3535.
We currently have five lints related to "the structural match situation":
- nontrivial_structural_match
- indirect_structural_match
- pointer_structural_match
- const_patterns_without_partial_eq
- illegal_floating_point_literal_pattern

This PR concerns the first 3 of them. (The 4th already is set up to show for dependencies, and the 5th is removed by rust-lang#116284.) nontrivial_structural_match is being removed as per the RFC; the other two are enabled to show up in dependencies.

Fixes rust-lang#73448 by removing the affected analysis.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 5, 2024
make matching on NaN a hard error, and remove the rest of illegal_floating_point_literal_pattern

These arms would never be hit anyway, so the pattern makes little sense. We have had a future-compat lint against float matches in general for a *long* time, so I hope we can get away with immediately making this a hard error.

This is part of implementing rust-lang/rfcs#3535.

Closes rust-lang#41620 by removing the lint.

rust-lang/reference#1456 updates the reference to match.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 5, 2024
…, r=petrochenkov

update indirect structural match lints to match RFC and to show up for dependencies

This is a large step towards implementing rust-lang/rfcs#3535.
We currently have five lints related to "the structural match situation":
- nontrivial_structural_match
- indirect_structural_match
- pointer_structural_match
- const_patterns_without_partial_eq
- illegal_floating_point_literal_pattern

This PR concerns the first 3 of them. (The 4th already is set up to show for dependencies, and the 5th is removed by rust-lang#116284.) nontrivial_structural_match is being removed as per the RFC; the other two are enabled to show up in dependencies.

Fixes rust-lang#73448 by removing the affected analysis.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 5, 2024
Rollup merge of rust-lang#116284 - RalfJung:no-nan-match, r=cjgillot

make matching on NaN a hard error, and remove the rest of illegal_floating_point_literal_pattern

These arms would never be hit anyway, so the pattern makes little sense. We have had a future-compat lint against float matches in general for a *long* time, so I hope we can get away with immediately making this a hard error.

This is part of implementing rust-lang/rfcs#3535.

Closes rust-lang#41620 by removing the lint.

rust-lang/reference#1456 updates the reference to match.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 6, 2024
…, r=petrochenkov

update indirect structural match lints to match RFC and to show up for dependencies

This is a large step towards implementing rust-lang/rfcs#3535.
We currently have five lints related to "the structural match situation":
- nontrivial_structural_match
- indirect_structural_match
- pointer_structural_match
- const_patterns_without_partial_eq
- illegal_floating_point_literal_pattern

This PR concerns the first 3 of them. (The 4th already is set up to show for dependencies, and the 5th is removed by rust-lang#116284.) nontrivial_structural_match is being removed as per the RFC; the other two are enabled to show up in dependencies.

Fixes rust-lang#73448 by removing the affected analysis.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 6, 2024
…, r=petrochenkov

update indirect structural match lints to match RFC and to show up for dependencies

This is a large step towards implementing rust-lang/rfcs#3535.
We currently have five lints related to "the structural match situation":
- nontrivial_structural_match
- indirect_structural_match
- pointer_structural_match
- const_patterns_without_partial_eq
- illegal_floating_point_literal_pattern

This PR concerns the first 3 of them. (The 4th already is set up to show for dependencies, and the 5th is removed by rust-lang#116284.) nontrivial_structural_match is being removed as per the RFC; the other two are enabled to show up in dependencies.

Fixes rust-lang#73448 by removing the affected analysis.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 6, 2024
…, r=petrochenkov

update indirect structural match lints to match RFC and to show up for dependencies

This is a large step towards implementing rust-lang/rfcs#3535.
We currently have five lints related to "the structural match situation":
- nontrivial_structural_match
- indirect_structural_match
- pointer_structural_match
- const_patterns_without_partial_eq
- illegal_floating_point_literal_pattern

This PR concerns the first 3 of them. (The 4th already is set up to show for dependencies, and the 5th is removed by rust-lang#116284.) nontrivial_structural_match is being removed as per the RFC; the other two are enabled to show up in dependencies.

Fixes rust-lang#73448 by removing the affected analysis.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 7, 2024
Rollup merge of rust-lang#120423 - RalfJung:indirect-structural-match, r=petrochenkov

update indirect structural match lints to match RFC and to show up for dependencies

This is a large step towards implementing rust-lang/rfcs#3535.
We currently have five lints related to "the structural match situation":
- nontrivial_structural_match
- indirect_structural_match
- pointer_structural_match
- const_patterns_without_partial_eq
- illegal_floating_point_literal_pattern

This PR concerns the first 3 of them. (The 4th already is set up to show for dependencies, and the 5th is removed by rust-lang#116284.) nontrivial_structural_match is being removed as per the RFC; the other two are enabled to show up in dependencies.

Fixes rust-lang#73448 by removing the affected analysis.
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request Feb 8, 2024
make matching on NaN a hard error, and remove the rest of illegal_floating_point_literal_pattern

These arms would never be hit anyway, so the pattern makes little sense. We have had a future-compat lint against float matches in general for a *long* time, so I hope we can get away with immediately making this a hard error.

This is part of implementing rust-lang/rfcs#3535.

Closes rust-lang/rust#41620 by removing the lint.

rust-lang/reference#1456 updates the reference to match.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const Proposals relating to const items A-patterns Pattern matching related proposals & ideas disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this RFC. T-lang Relevant to the language team, which will review and decide on the RFC. to-announce
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.