Test validity of pattern types by oli-obk · Pull Request #136145 · rust-lang/rust

oli-obk · 2025-01-27T17:16:23Z

pulled out of #136006 so we don't have to rely on libcore types excercising this code path

There's nothing to fix. rustc_layout_scalar_valid_range_start structs just failed their validation on their value instead of their fields' value, causing a diff where moving to pattern types adds an additional .0 field access to the validation error

RalfJung · 2025-01-27T17:33:30Z

Surely patterns can express more than what fits into rustc_layout_scalar_valid_range? We should check the actual pattern, if it is relevant for validity. I assume even non-scalar types can have a pattern?

So some tests that probably still fail are:

transmute 10 to pattern_type!(u32 is 1..10 | 11..)
transmute (0, 0, 0) to pattern_type!((u32, u32, u32) is (1.., 1.., 1..))

oli-obk · 2025-01-27T18:18:19Z

Those patterns are not supported yet and will just error out in ast lowering, never reaching the type system

RalfJung · 2025-01-28T01:48:09Z

What exactly is the full set of patterns supported currently? And how can we increase the probability that when that set gets expanded, the validity check is updated accordingly?

oli-obk · 2025-01-28T07:08:53Z

The validity check automatically handles everything, because it looks at the layout of pattern types. The problem will be once we can't represent something in the layout anymore.

I could add an exhaustive match with a comment on the range pattern saying it's fully handled by the layout logic

rustbot · 2025-01-28T08:05:19Z

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

oli-obk · 2025-01-28T08:13:56Z

I could add an exhaustive match with a comment on the range pattern saying it's fully handled by the layout logic

done

oli-obk · 2025-01-28T10:29:50Z

Ah... I need to check nesting pattern types within each other. And for that we need to actually recurse into the base type.

rustbot · 2025-01-28T16:21:41Z

HIR ty lowering was modified

cc @fmease

oli-obk · 2025-01-28T16:47:42Z

Ok, I think I rejected everything that is relevant for validation and added tests for the rest

RalfJung · 2025-02-02T14:23:01Z

+ // tests/ui/type/pattern_types/validity.rs
+ match **pat {
+ // Range patterns are handled fully by looking at the layout
+ ty::PatternKind::Range { .. } => {},


Seems worth asserting that layout.backend_repr is indeed Scalar... skipping the recursion here is a bit dubious, we don't do this for anything else.

In particular, I think this means that e.g. if a u32 range pattern type is uninitialized, or a char range pattern type is not a valid char, we skip the nice error that try_visit_primitive would generate, and instead show a more generic error in visit_scalar.

Hmm tried recursing, but it doesn't actually do anything better

Can you add char cases to pattern_types/validity.rs, one case where the character is valid but outside the range and one case where it's not even a valid char?

That did it, yea

So that test does show a different error with the extra call to visit_value? Great :)

RalfJung · 2025-02-02T14:35:12Z

@rustbot author

oli-obk · 2025-02-02T18:43:13Z

@rustbot ready

RalfJung · 2025-02-02T19:10:16Z

Please avoid rebasing when there is no conflict; now I can't see what the diff is to my last review.

RalfJung · 2025-02-02T19:11:32Z

This seems like it should be known-bug? We surely don't want to see these ICE reports, right?^^

it is already, just with no issue open, so known-bug: unknown

oli-obk · 2025-02-02T19:21:45Z

Please avoid rebasing when there is no conflict; now I can't see what the diff is to my last review.

There were diffs that confused me, so I thought rebasing would solve them, but it turned out it was diffs in the LOC of the panic location. So now I filtered out those.

oli-obk · 2025-02-02T19:41:56Z

@rustbot ready

RalfJung · 2025-02-02T20:21:58Z

 }
+ ty::Pat(base, pat) => {
+ // First check that the base type is valid
+ self.visit_value(&val.transmute(self.ecx.layout_of(*base)?, self.ecx)?)?;


Is there a field projection for this? Or is transmute the way this is meant to be done?

There is no field projection. Using a field like representation is what we want to get rid of with pattern types to avoid all the pains we have with the rustc_scalar_layout attributes. We could add something to the validation path to show that we're validating the base type, but there's no projection for it, just like niche optimized enums have no projection for the discriminant really

RalfJung · 2025-02-02T20:22:10Z

r=me with CI green, assuming that there is indeed no field projection.

oli-obk · 2025-02-02T20:53:27Z

@bors r=RalfJung

bors · 2025-02-02T20:53:29Z

📌 Commit 7e4ccc2 has been approved by RalfJung

It is now in the queue for this repository.

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#136145 (Test validity of pattern types) - rust-lang#136339 (CompileTest: Add Directives to Ignore `arm-unknown-*` Targets) - rust-lang#136403 (Fix malformed error annotations in a UI test) - rust-lang#136414 (Shorten error message for callable with wrong return type) - rust-lang#136425 (Move `rustc_middle::infer::unify_key`) - rust-lang#136426 (Explain why we retroactively change a static initializer to have a different type) - rust-lang#136445 (Couple of cleanups to DiagCtxt and EarlyDiagCtxt) - rust-lang#136452 (Miri subtree update) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#136145 - oli-obk:push-wxvpklmkppqz, r=RalfJung Test validity of pattern types r? `@RalfJung` pulled out of rust-lang#136006 so we don't have to rely on libcore types excercising this code path There's nothing to fix. `rustc_layout_scalar_valid_range_start` structs just failed their validation on their value instead of their fields' value, causing a diff where moving to pattern types adds an additional `.0` field access to the validation error

rustbot assigned Noratrieb Jan 27, 2025

This comment was marked as outdated.
Sign in to view

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 Jan 27, 2025

rustbot assigned RalfJung and unassigned Noratrieb Jan 27, 2025

oli-obk force-pushed the push-wxvpklmkppqz branch from d9865d7 to 2ed0e74Compare January 28, 2025 08:05

oli-obk force-pushed the push-wxvpklmkppqz branch from 2ed0e74 to cb79d1bCompare January 28, 2025 08:06

This comment has been minimized.
Sign in to view

oli-obk force-pushed the push-wxvpklmkppqz branch 2 times, most recently from de6c522 to 6697731Compare January 28, 2025 16:47

oli-obk force-pushed the push-wxvpklmkppqz branch 2 times, most recently from d743801 to bd1a54cCompare January 30, 2025 07:37

RalfJung reviewed Feb 2, 2025
View reviewed changes

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 2, 2025

oli-obk force-pushed the push-wxvpklmkppqz branch from 72dea42 to 640d948Compare February 2, 2025 17:27

This comment has been minimized.
Sign in to view

oli-obk force-pushed the push-wxvpklmkppqz branch from 640d948 to b5d3029Compare February 2, 2025 18:13

This comment has been minimized.
Sign in to view

oli-obk force-pushed the push-wxvpklmkppqz branch from b5d3029 to c17eee6Compare February 2, 2025 18:42

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 Feb 2, 2025

RalfJung reviewed Feb 2, 2025
View reviewed changes

oli-obk added 3 commits February 2, 2025 19:28

Test validity of pattern types
2f2b32b

Check the base type of pattern types for validity first
ded0836

Maintain a list of types permitted per pattern
7e4ccc2

oli-obk force-pushed the push-wxvpklmkppqz branch from c17eee6 to 7e4ccc2Compare February 2, 2025 19:41

RalfJung reviewed Feb 2, 2025
View reviewed changes

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-review Status: Awaiting review from the assignee but also interested parties. labels Feb 2, 2025

matthiaskrgr mentioned this pull request Feb 2, 2025
Rollup of 8 pull requests #136454
Merged

bors merged commit f56e4b3 into rust-lang:masterFeb 3, 2025

rustbot added this to the 1.86.0 milestone Feb 3, 2025

Uh oh!

Uh oh!

Conversation

oli-obk commented Jan 27, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

RalfJung commented Jan 27, 2025

Uh oh!

oli-obk commented Jan 27, 2025

Uh oh!

RalfJung commented Jan 28, 2025

Uh oh!

oli-obk commented Jan 28, 2025

Uh oh!

rustbot commented Jan 28, 2025

Uh oh!

oli-obk commented Jan 28, 2025

Uh oh!

oli-obk commented Jan 28, 2025

Uh oh!

rustbot commented Jan 28, 2025

Uh oh!

This comment has been minimized.

oli-obk commented Jan 28, 2025

Uh oh!

Uh oh!

RalfJungFeb 2, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung commented Feb 2, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

oli-obk commented Feb 2, 2025

Uh oh!

RalfJung commented Feb 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oli-obk commented Feb 2, 2025

Uh oh!

oli-obk commented Feb 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung commented Feb 2, 2025

Uh oh!

oli-obk commented Feb 2, 2025

Uh oh!

bors commented Feb 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

oli-obk commented Jan 27, 2025•
edited
Loading

RalfJungFeb 2, 2025•
edited
Loading