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

Do not ICE when interpreting a cast between non-monomorphic types #108790

Merged
merged 2 commits into from
Mar 7, 2023

Conversation

cjgillot
Copy link
Contributor

@cjgillot cjgillot commented Mar 5, 2023

Fixes #101596

@rustbot
Copy link
Collaborator

rustbot commented Mar 5, 2023

r? @petrochenkov

(rustbot has picked a reviewer for you, use r? to override)

@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 Mar 5, 2023
@rustbot
Copy link
Collaborator

rustbot commented Mar 5, 2023

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

@saethlin
Copy link
Member

saethlin commented Mar 5, 2023

Thank you so much for fixing this. It's been around for what feels like forever.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 6, 2023

r? @oli-obk

Even ignoring that this is only an issue for const prop which should not be going through all these code paths, bailing out when not monomorphic is always correct.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 6, 2023

📌 Commit 858eab6 has been approved by oli-obk

It is now in the queue for this repository.

@rustbot rustbot assigned oli-obk and unassigned petrochenkov Mar 6, 2023
@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 Mar 6, 2023
@@ -67,12 +67,12 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
}

Pointer(PointerCast::ReifyFnPointer) => {
// All reifications must be monomorphic, bail out otherwise.
ensure_monomorphic_enough(*self.tcx, src.layout.ty)?;
Copy link
Member

Choose a reason for hiding this comment

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

Should we just have a single one of these at the top of fn cast?

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's not necessarily wrong to cast partially polymorphic values, as long as the type parameters don't change the behaviour (layout / choice of unsize branch...). The checks up to this point are enough to make sure of this, without requiring monomorphic types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But the match on src.layout.ty.kind will ignore such leeway, so requires the type to be "monomorphic enough".

Copy link
Member

@RalfJung RalfJung Mar 6, 2023

Choose a reason for hiding this comment

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

The interpreter has no interest in supporting generic code. So IMO it'd make sense, and significantly simplify things, if we could do the check once at the top and then not think about the generic case any more -- rather than having to chase ICEs each time this code gets refactored.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 6, 2023
Do not ICE when interpreting a cast between non-monomorphic types

Fixes rust-lang#101596
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 6, 2023
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#107801 (const_eval: `implies_by` in `rustc_const_unstable`)
 - rust-lang#108750 (Fix `ObligationCtxt::sub`)
 - rust-lang#108780 (Add regression tests for issue 70919)
 - rust-lang#108786 (Check for free regions in MIR validation)
 - rust-lang#108790 (Do not ICE when interpreting a cast between non-monomorphic types)
 - rust-lang#108803 (Do not ICE when failing to normalize in ConstProp.)
 - rust-lang#108807 (Emit the suspicious_auto_trait_impls for negative impls as well)
 - rust-lang#108812 (Add regression test for rust-lang#98444)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 3ae047b into rust-lang:master Mar 7, 2023
@rustbot rustbot added this to the 1.70.0 milestone Mar 7, 2023
@cjgillot cjgillot deleted the mono-cast branch March 7, 2023 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

ICE due to invalid unsizing &B -> [u8]
7 participants