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

Pattern types MVP #107606

Closed
wants to merge 13 commits into from
Closed

Pattern types MVP #107606

wants to merge 13 commits into from

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Feb 2, 2023

Doing #107299 the right way ™️

cc @joshtriplett for the language side

This PR adds pattern types to the parser and a minimal version to the type system. This isn't useful yet, as you can't create values of pattern types type except via transmute, but I thought it best to land this MVP and then iterate on the feature instead of producing a bigger and bigger PR that becomes harder to review with every addition.

@rustbot
Copy link
Collaborator

rustbot commented Feb 2, 2023

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

Please see the contribution instructions for more information.

@rustbot rustbot added A-rustdoc-json Area: Rustdoc JSON backend A-testsuite Area: The testsuite used to check the correctness of rustc A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic 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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Feb 2, 2023
@rustbot
Copy link
Collaborator

rustbot commented Feb 2, 2023

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @estebank, @TaKO8Ki

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

rustdoc-json-types is a public (although nightly-only) API. If possible, consider changing src/librustdoc/json/conversions.rs; otherwise, make sure you bump the FORMAT_VERSION constant.

cc @CraftSpider, @aDotInTheVoid, @Enselic, @obi1kenobi

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in src/librustdoc/clean/types.rs

cc @camelid

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

@@ -83,6 +83,9 @@ pub(crate) fn eval_nullary_intrinsic<'tcx>(
ty::Alias(..) | ty::Param(_) | ty::Placeholder(_) | ty::Infer(_) => {
throw_inval!(TooGeneric)
}
ty::Pat(..) => {
unimplemented!("pattern types need to calculate pattern from their pattern")
Copy link
Member

Choose a reason for hiding this comment

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

My confusion about this error message remains.^^ Who's asking for a pattern to be computed from anything here? We just want a variant count! What does it even mean to compute a pattern from a pattern? Seems trivial, just use the identity function...

@@ -342,6 +342,12 @@ impl<'a> Parser<'a> {
let span = lo.to(self.prev_token.span);
let mut ty = self.mk_ty(span, kind);

if self.eat_keyword(sym::is) {
Copy link
Member

@compiler-errors compiler-errors Feb 2, 2023

Choose a reason for hiding this comment

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

doesn't this change the behavior of macro code like:

macro_rules! foo {
  ($ty:ty) => {};
  (() is $pat:pat) => {};
}

foo!(() is 0..1);

Copy link
Member

Choose a reason for hiding this comment

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

macro rules matches continue being the least forwards compatible thing ever :|

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it's better to expose this through a builtin macro instead of surface syntax, like rustc_pattern_type!() which expands into ast::TyKind::Pat

Copy link
Member

@fmease fmease Feb 3, 2023

Choose a reason for hiding this comment

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

Or builtin#Pat(…) instead of a built-in macro (with builtin# from T-compiler MCP #580).

Copy link
Contributor

Choose a reason for hiding this comment

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

macro rules matches continue being the least forwards compatible thing ever :|

And this is where the editions system can shine.

Copy link
Contributor

Choose a reason for hiding this comment

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

... I'm a little surprised/sad that foo! works at all today. Follow sets and greedy parsing were supposed to handle this -- you're not allowed to follow a :ty fragment with is -- but follow set restrictions are only applied within an arm, and not across arms. Being able to follow a fragment with some token in a simultaneously-taken arm matching the fragment's tokens essentially makes follow sets ineffectual at their job of making fragments forwards compatible. But simultaneously, applying follow set rules between arms would be very restrictive and probably not even solve the problem entirely, since you can attack non-follow-set grammar extensions similarly. I guess you'd need to ensure that the choice of which arm to take is always known to be forwards-compatibly LL(k) via some sort of "leading set," and if the choice isn't LL(k), ensure that the refining match fits within the grammar and follow set of the refined fragment.

i.e., complicated. And still overly restrictive, since we also have the rule for lexically prior arms taking priority over latter arms, so there's only a forwards compatibility concern if the refining arms come after the refined arm. (And until today I thought that macro parsing was greedier than it actually is, preventing foo! from ever matching the second arm here, since the $:ty arm is prior and matched the prefix. Despite probably relying on that not being the case.)

Comment on lines +850 to +854
ast::TyKind::Pat(ref ty, ref pat) => {
let ty = ty.rewrite(context, shape)?;
let pat = pat.rewrite(context, shape)?;
Some(format!("{ty} is {pat}"))
},
Copy link
Member

Choose a reason for hiding this comment

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

cc @rust-lang/style

This seems quite reasonable and noncontroversial to me, and I'd be surprised if there's any vehement objections to this formatting. However, want to make sure everyone has seen it in case we'd like a chance to discuss this as a team first.

In the event we do want to review as a team to identify and codify the formatting rules, then this can be changed to re-emit the original input contents in the interim via something like:

Suggested change
ast::TyKind::Pat(ref ty, ref pat) => {
let ty = ty.rewrite(context, shape)?;
let pat = pat.rewrite(context, shape)?;
Some(format!("{ty} is {pat}"))
},
ast::TyKind::Pat(..) => Some(context.snippet(self.span).to_owned()),

Comment on lines +128 to +131
hir_analysis_pattern_type_wild_pat = "wildcard patterns are not permitted for pattern types"
.label = "this type is the same as the inner type without a pattern"

hir_analysis_pattern_type_non_const_range = "range patterns must have constant range start and end"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
hir_analysis_pattern_type_wild_pat = "wildcard patterns are not permitted for pattern types"
.label = "this type is the same as the inner type without a pattern"
hir_analysis_pattern_type_non_const_range = "range patterns must have constant range start and end"
hir_analysis_pattern_type_wild_pat = wildcard patterns are not permitted for pattern types
.label = this type is the same as the inner type without a pattern
hir_analysis_pattern_type_non_const_range = range patterns must have constant range start and end

@scottmcm
Copy link
Member

scottmcm commented Feb 2, 2023

Not asking for it to be implemented in this PR, but is the idea that these will have subtyping or coercions to less-restrictive patterns? Is u8 the same as u8 is 0..=255?

I think I'm in favour, but it'd be nice to have a link to a post or something about a sketch of the goal here.

@joboet
Copy link
Member

joboet commented Mar 7, 2023

What if types with patterns didn't have niches on their own, but were subtypes of the unbounded type? This would solve construction and usage:

Construction is accomplished by matching the unbounded type against the pattern:

match value as i32 {
    x @ 0.. => Some(x), // x has type `i32 is 0..`
    _ => None,
}

Since i32 is 0.. is a subtype of i32, this is backwards compatible.

Only if the pattern type is explicitly used in a field does the niche get exposed:

struct NonZeroI32(i32 is ..0 | 1..);

println!("{}", size_of::<Option<NonZeroI32>>()); // Prints "4"
println!("{}", size_of::<Option<i32 is ..0 | 1..>>(); // Prints "5"

Since the field is not generic, the subtyping information is hidden, so having a niche is sound. #[repr(transparent)] would hide the niche again.

@Maix0
Copy link

Maix0 commented Mar 8, 2023

I am currently fiddling with rustc as a first project to implement way to make subenum (only at the declaration of the enum tho). This is kinda the same thing but with a broader range.

I was thinking of allowing a sort of "specialization" where you can override function that exists in the parent enum. it would allow for some optimization.

If I understand correctly these "pattern restricted types" are just for type checking and the layout right ?
For the rest of rustc they are the parent type.

Would it be possible to add this functionnality to this or not (I doubt it since in my approch the subenums are declared WITH the main enum, and no consumer can create new subenum, so no orphan rule applies).

@tgross35
Copy link
Contributor

Just some drive by prior art - this somewhat reminds me of ada pre & post conditions1, which I've thought was a cool feature that could potentially fit in where clauses. Basically it just adds a runtime check (or compile time) for the arguments and for the function results, and panics if it's not true (for things like train/plane control systems and other high-SIL applications)

function Area(C: Circle)
   with Pre => C.X_Coord – C.Radius > 0.0,
        Post => Area'Result > 3.1 * C.Radius**2 and
                Area'Result < 3.2 * C.Radius**2;

Of course this is really more like the type constraints23 that Ada also has, but just a bit of an interesting related note.

Footnotes

  1. http://www.ada-auth.org/standards/12rat/html/Rat12-2-3.html

  2. https://docs.actian.com/ingres/10s/index.html#page/EmbedSQL/Type_Constraints.htm

  3. http://archive.adaic.com/standards/83rat/html/ratl-04-04.html

@SoniEx2
Copy link
Contributor

SoniEx2 commented Apr 30, 2023

this does support #[non_exhaustive] yes?

@tialaramex
Copy link
Contributor

I wrote "Where do I pick up a shovel and help?" four months ago. There has been no answer in the intervening period.

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 15, 2023

There's related discussion in https://rust-lang.zulipchat.com/#narrow/stream/144729-t-types/topic/Implementing.20Enum.20Variant.20Types/near/322878006 (TLDR: someone is working on reviving this PR with a placeholder macro for syntax (to eliminate the parser/macro issues this PR has).

I don't know what the status of that work is, but maybe you two can collaborate?

@lcnr
Copy link
Contributor

lcnr commented Jun 15, 2023

please open a types team MCP before trying to merge anything in this area (even if unstable). The exact way this new type kind should work is non-trivial and may have complex interactions with the rest of the type system.

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 15, 2023

Apropos, keeping this PR open serves no purpose :D I'm closing it, we can continue the discussion on zulip

@oli-obk oli-obk closed this Jun 15, 2023
@oli-obk oli-obk deleted the pattern_types_syntax branch June 15, 2023 13:45
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 20, 2024
Don't forget that the lifetime on hir types is `'tcx`

This PR just tracks the `'tcx` lifetime to wherever the original objects actually have that lifetime. This code is needed for rust-lang#107606 (now rust-lang#120131) so that `ast_ty_to_ty` can invoke `lit_to_const` on an argument passed to it. Currently the argument is `&hir::Ty<'_>`, but after this PR it is `&'tcx hir::Ty<'tcx>`.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 21, 2024
Don't forget that the lifetime on hir types is `'tcx`

This PR just tracks the `'tcx` lifetime to wherever the original objects actually have that lifetime. This code is needed for rust-lang#107606 (now rust-lang#120131) so that `ast_ty_to_ty` can invoke `lit_to_const` on an argument passed to it. Currently the argument is `&hir::Ty<'_>`, but after this PR it is `&'tcx hir::Ty<'tcx>`.
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Jan 21, 2024
Don't forget that the lifetime on hir types is `'tcx`

This PR just tracks the `'tcx` lifetime to wherever the original objects actually have that lifetime. This code is needed for rust-lang/rust#107606 (now #120131) so that `ast_ty_to_ty` can invoke `lit_to_const` on an argument passed to it. Currently the argument is `&hir::Ty<'_>`, but after this PR it is `&'tcx hir::Ty<'tcx>`.
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request Jan 25, 2024
Don't forget that the lifetime on hir types is `'tcx`

This PR just tracks the `'tcx` lifetime to wherever the original objects actually have that lifetime. This code is needed for rust-lang/rust#107606 (now #120131) so that `ast_ty_to_ty` can invoke `lit_to_const` on an argument passed to it. Currently the argument is `&hir::Ty<'_>`, but after this PR it is `&'tcx hir::Ty<'tcx>`.
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 5, 2024
…iler-errors

Implement minimal, internal-only pattern types in the type system

rebase of rust-lang#107606

You can create pattern types with `std::pat::pattern_type!(ty is pat)`. The feature is incomplete and will panic on you if you use any pattern other than integral range patterns. The only way to create or deconstruct a pattern type is via `transmute`.

This PR's implementation differs from the MCP's text. Specifically

> This means you could implement different traits for different pattern types with the same base type. Thus, we just forbid implementing any traits for pattern types.

is violated in this PR. The reason is that we do need impls after all in order to make them usable as fields. constants of type `std::time::Nanoseconds` struct are used in patterns, so the type must be structural-eq, which it only can be if you derive several traits on it. It doesn't need to be structural-eq recursively, so we can just manually implement the relevant traits on the pattern type and use the pattern type as a private field.

Waiting on:

* [x] move all unrelated commits into their own PRs.
* [x] fix niche computation (see 2db07f9)
* [x] add lots more tests
* [x] T-types MCP rust-lang/types-team#126 to finish
* [x] some commit cleanup
* [x] full self-review
* [x] remove 61bd325, it's not necessary anymore I think.
* [ ] ~~make sure we never accidentally leak pattern types to user code (add stability checks or feature gate checks and appopriate tests)~~ we don't even do this for the new float primitives
* [x] get approval that [the scope expansion to trait impls](https://rust-lang.zulipchat.com/#narrow/stream/326866-t-types.2Fnominated/topic/Pattern.20types.20types-team.23126/near/427670099) is ok

r? `@BoxyUwU`
lnicola pushed a commit to lnicola/rust-analyzer that referenced this pull request Apr 7, 2024
Don't forget that the lifetime on hir types is `'tcx`

This PR just tracks the `'tcx` lifetime to wherever the original objects actually have that lifetime. This code is needed for rust-lang/rust#107606 (now #120131) so that `ast_ty_to_ty` can invoke `lit_to_const` on an argument passed to it. Currently the argument is `&hir::Ty<'_>`, but after this PR it is `&'tcx hir::Ty<'tcx>`.
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 8, 2024
…iler-errors

Implement minimal, internal-only pattern types in the type system

rebase of rust-lang#107606

You can create pattern types with `std::pat::pattern_type!(ty is pat)`. The feature is incomplete and will panic on you if you use any pattern other than integral range patterns. The only way to create or deconstruct a pattern type is via `transmute`.

This PR's implementation differs from the MCP's text. Specifically

> This means you could implement different traits for different pattern types with the same base type. Thus, we just forbid implementing any traits for pattern types.

is violated in this PR. The reason is that we do need impls after all in order to make them usable as fields. constants of type `std::time::Nanoseconds` struct are used in patterns, so the type must be structural-eq, which it only can be if you derive several traits on it. It doesn't need to be structural-eq recursively, so we can just manually implement the relevant traits on the pattern type and use the pattern type as a private field.

Waiting on:

* [x] move all unrelated commits into their own PRs.
* [x] fix niche computation (see 2db07f9)
* [x] add lots more tests
* [x] T-types MCP rust-lang/types-team#126 to finish
* [x] some commit cleanup
* [x] full self-review
* [x] remove 61bd325, it's not necessary anymore I think.
* [ ] ~~make sure we never accidentally leak pattern types to user code (add stability checks or feature gate checks and appopriate tests)~~ we don't even do this for the new float primitives
* [x] get approval that [the scope expansion to trait impls](https://rust-lang.zulipchat.com/#narrow/stream/326866-t-types.2Fnominated/topic/Pattern.20types.20types-team.23126/near/427670099) is ok

r? `@BoxyUwU`
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 8, 2024
…iler-errors

Implement minimal, internal-only pattern types in the type system

rebase of rust-lang#107606

You can create pattern types with `std::pat::pattern_type!(ty is pat)`. The feature is incomplete and will panic on you if you use any pattern other than integral range patterns. The only way to create or deconstruct a pattern type is via `transmute`.

This PR's implementation differs from the MCP's text. Specifically

> This means you could implement different traits for different pattern types with the same base type. Thus, we just forbid implementing any traits for pattern types.

is violated in this PR. The reason is that we do need impls after all in order to make them usable as fields. constants of type `std::time::Nanoseconds` struct are used in patterns, so the type must be structural-eq, which it only can be if you derive several traits on it. It doesn't need to be structural-eq recursively, so we can just manually implement the relevant traits on the pattern type and use the pattern type as a private field.

Waiting on:

* [x] move all unrelated commits into their own PRs.
* [x] fix niche computation (see 2db07f9)
* [x] add lots more tests
* [x] T-types MCP rust-lang/types-team#126 to finish
* [x] some commit cleanup
* [x] full self-review
* [x] remove 61bd325, it's not necessary anymore I think.
* [ ] ~~make sure we never accidentally leak pattern types to user code (add stability checks or feature gate checks and appopriate tests)~~ we don't even do this for the new float primitives
* [x] get approval that [the scope expansion to trait impls](https://rust-lang.zulipchat.com/#narrow/stream/326866-t-types.2Fnominated/topic/Pattern.20types.20types-team.23126/near/427670099) is ok

r? `@BoxyUwU`
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request Apr 18, 2024
Implement minimal, internal-only pattern types in the type system

rebase of rust-lang/rust#107606

You can create pattern types with `std::pat::pattern_type!(ty is pat)`. The feature is incomplete and will panic on you if you use any pattern other than integral range patterns. The only way to create or deconstruct a pattern type is via `transmute`.

This PR's implementation differs from the MCP's text. Specifically

> This means you could implement different traits for different pattern types with the same base type. Thus, we just forbid implementing any traits for pattern types.

is violated in this PR. The reason is that we do need impls after all in order to make them usable as fields. constants of type `std::time::Nanoseconds` struct are used in patterns, so the type must be structural-eq, which it only can be if you derive several traits on it. It doesn't need to be structural-eq recursively, so we can just manually implement the relevant traits on the pattern type and use the pattern type as a private field.

Waiting on:

* [x] move all unrelated commits into their own PRs.
* [x] fix niche computation (see 2db07f94f44f078daffe5823680d07d4fded883f)
* [x] add lots more tests
* [x] T-types MCP rust-lang/types-team#126 to finish
* [x] some commit cleanup
* [x] full self-review
* [x] remove 61bd325da19a918cc3e02bbbdce97281a389c648, it's not necessary anymore I think.
* [ ] ~~make sure we never accidentally leak pattern types to user code (add stability checks or feature gate checks and appopriate tests)~~ we don't even do this for the new float primitives
* [x] get approval that [the scope expansion to trait impls](https://rust-lang.zulipchat.com/#narrow/stream/326866-t-types.2Fnominated/topic/Pattern.20types.20types-team.23126/near/427670099) is ok

r? `@BoxyUwU`
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 27, 2024
Don't forget that the lifetime on hir types is `'tcx`

This PR just tracks the `'tcx` lifetime to wherever the original objects actually have that lifetime. This code is needed for rust-lang/rust#107606 (now #120131) so that `ast_ty_to_ty` can invoke `lit_to_const` on an argument passed to it. Currently the argument is `&hir::Ty<'_>`, but after this PR it is `&'tcx hir::Ty<'tcx>`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-json Area: Rustdoc JSON backend A-testsuite Area: The testsuite used to check the correctness of rustc A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)
Projects
None yet
Development

Successfully merging this pull request may close these issues.