Search code, repositories, users, issues, pull requests...

Copy link

Member

RalfJung commented Jan 27, 2025

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..))

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

Copy link

Member

RalfJung commented Jan 28, 2025

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?

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

@oli-obk oli-obk force-pushed the push-wxvpklmkppqz branch from d9865d7 to 2ed0e74 Compare

January 28, 2025 08:05

@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 oli-obk force-pushed the push-wxvpklmkppqz branch from 2ed0e74 to cb79d1b Compare

January 28, 2025 08:06

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

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.

@oli-obk oli-obk force-pushed the push-wxvpklmkppqz branch 2 times, most recently from de6c522 to 6697731 Compare

January 28, 2025 16:47

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

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

@RalfJung RalfJung Feb 2, 2025

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

@oli-obk oli-obk Feb 2, 2025

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

@RalfJung RalfJung Feb 2, 2025

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

@oli-obk oli-obk Feb 2, 2025

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

@RalfJung RalfJung Feb 2, 2025

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

@oli-obk oli-obk Feb 2, 2025

Choose a reason for hiding this comment

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

Yes

Copy link

Member

RalfJung commented Feb 2, 2025

@rustbot author

@rustbot rustbot added S-waiting-on-author and removed S-waiting-on-review labels

@oli-obk oli-obk force-pushed the push-wxvpklmkppqz branch from 72dea42 to 640d948 Compare

February 2, 2025 17:27

@rust-log-analyzer

This comment has been minimized.

@oli-obk oli-obk force-pushed the push-wxvpklmkppqz branch from 640d948 to b5d3029 Compare

February 2, 2025 18:13

@rust-log-analyzer

This comment has been minimized.

@oli-obk oli-obk force-pushed the push-wxvpklmkppqz branch from b5d3029 to c17eee6 Compare

February 2, 2025 18:42

Copy link

Contributor Author

oli-obk commented Feb 2, 2025

@rustbot ready

@rustbot rustbot added S-waiting-on-review and removed S-waiting-on-author labels

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

View reviewed changes

tests/ui/type/pattern_types/pattern_type_mismatch.rs Outdated

Copy link

Member

@RalfJung RalfJung Feb 2, 2025

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

@oli-obk oli-obk Feb 2, 2025

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

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 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 oli-obk force-pushed the push-wxvpklmkppqz branch from c17eee6 to 7e4ccc2 Compare

February 2, 2025 19:41

Copy link

Contributor Author

oli-obk commented Feb 2, 2025

@rustbot ready

RalfJung

RalfJung reviewed

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

@RalfJung RalfJung Feb 2, 2025

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

@oli-obk oli-obk Feb 2, 2025

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

Copy link

Member

RalfJung commented Feb 2, 2025

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

Copy link

Contributor Author

oli-obk commented Feb 2, 2025

@bors r=RalfJung

@bors

Copy link

Collaborator

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 and removed S-waiting-on-review labels

@matthiaskrgr matthiaskrgr mentioned this pull request

Rollup of 8 pull requests #136454

Merged

bors added a commit to rust-lang-ci/rust that referenced this pull request

@bors


 Auto merge of rust-lang#136454 - matthiaskrgr:rollup-ewejzmp, r=matth...

0bafdaf

...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

bors added a commit to rust-lang-ci/rust that referenced this pull request