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.

Sign up for GitHub

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

Jump to bottom

Test validity of pattern types #136145

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Test validity of pattern types #136145

wants to merge 3 commits into from
+338 −2

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Jan 27, 2025 •
edited
Loading

r? @RalfJung

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

@rustbot

This comment was marked as outdated.

Sign in to view
@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 Jan 27, 2025
@rustbot rustbot assigned RalfJung and unassigned Noratrieb Jan 27, 2025
@RalfJung
Copy link
Member

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
Copy link
Contributor Author

oli-obk commented Jan 27, 2025

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

@RalfJung
Copy link
Member

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
Copy link
Contributor Author

oli-obk commented Jan 28, 2025

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
Copy link
Collaborator

rustbot commented Jan 28, 2025

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
Copy link
Contributor Author

oli-obk commented Jan 28, 2025

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
Copy link
Contributor Author

oli-obk commented Jan 28, 2025

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

@rustbot
Copy link
Collaborator

rustbot commented Jan 28, 2025

HIR ty lowering was modified

cc @fmease

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@oli-obk oli-obk force-pushed the push-wxvpklmkppqz branch 2 times, most recently from de6c522 to 6697731 Compare January 28, 2025 16:47
@oli-obk
Copy link
Contributor Author

oli-obk commented Jan 28, 2025

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

@oli-obk oli-obk force-pushed the push-wxvpklmkppqz branch 2 times, most recently from d743801 to bd1a54c Compare January 30, 2025 07:37
RalfJung
RalfJung reviewed Feb 2, 2025
View reviewed changes
compiler/rustc_const_eval/src/interpret/validity.rs Outdated Show resolved Hide resolved
compiler/rustc_const_eval/src/interpret/validity.rs
// tests/ui/type/pattern_types/validity.rs
match **pat {
// Range patterns are handled fully by looking at the layout
ty::PatternKind::Range { .. } => {},
Copy link
Member

@RalfJung RalfJung Feb 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That did it, yea

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

@RalfJung
Copy link
Member

RalfJung commented Feb 2, 2025

@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 2, 2025
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 2, 2025

@rustbot ready

@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 Feb 2, 2025
@RalfJung
Copy link
Member

RalfJung commented Feb 2, 2025

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

RalfJung
RalfJung reviewed Feb 2, 2025
View reviewed changes
tests/ui/type/pattern_types/pattern_type_mismatch.rs Outdated
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 2, 2025

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
Copy link
Contributor Author

oli-obk commented Feb 2, 2025

@rustbot ready

RalfJung
RalfJung reviewed Feb 2, 2025
View reviewed changes
compiler/rustc_const_eval/src/interpret/validity.rs
@@ -1240,6 +1240,17 @@ impl<'rt, 'tcx, M: Machine<'tcx>> ValueVisitor<'tcx, M> for ValidityVisitor<'rt,
self.visit_field(val, 0, &self.ecx.project_index(val, 0)?)?;
}
}
ty::Pat(base, pat) => {
// First check that the base type is valid
self.visit_value(&val.transmute(self.ecx.layout_of(*base)?, self.ecx)?)?;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Member

RalfJung commented Feb 2, 2025

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

@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 2, 2025

@bors r=RalfJung

@bors
Copy link
Contributor

bors commented Feb 2, 2025

📌 Commit 7e4ccc2 has been approved by RalfJung

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-review Status: Awaiting review from the assignee but also interested parties. labels Feb 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

Assignees

@RalfJung RalfJung

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@oli-obk @rustbot @RalfJung @rust-log-analyzer @bors @Noratrieb