Skip to content

Add bug! for unused OpaqueCast post borrowck #116140

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

Closed
wants to merge 1 commit into from

Conversation

ouz-a
Copy link
Contributor

@ouz-a ouz-a commented Sep 25, 2023

While working on #115025 we discovered some of the ProjectionElem::OpaqueCast were unused, so this change reflects their current behavior more clearly.

r? @lcnr

@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 Sep 25, 2023
@rustbot
Copy link
Collaborator

rustbot commented Sep 25, 2023

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

can you add a comment to ProjectionElem::OpaqueCast that this projection should not exist in MirPase::Runtime, extend the changes mentioned in the MirPhase::Runtime(RuntimePhase::Initial) docs, and change the mir validator to fail if it encounters an OpaqueCast during MirPhase::Runtime

r=me afterwards

@@ -316,7 +316,9 @@ where
{
use rustc_middle::mir::ProjectionElem::*;
Ok(match proj_elem {
OpaqueCast(ty) => base.transmute(self.layout_of(ty)?, self)?,
OpaqueCast(_) => {
bug!("OpaqueCast should be unreachable here!");
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 make this span_bug!(self.cur_span(), ...? That will make this much easier to debug, should that ever come up.

Copy link
Contributor

Choose a reason for hiding this comment

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

this PR is sadly wrong given that OpaqueCast does exist after analysis, we just don't have a single test which actually encounters it 😅 @ouz-a will add such tests in a different PR 😁

@ouz-a ouz-a mentioned this pull request Sep 25, 2023
@ouz-a
Copy link
Contributor Author

ouz-a commented Sep 25, 2023

As lcnr mentioned this pr is wrong, so closing it in favor of newer one.

@ouz-a ouz-a closed this Sep 25, 2023
@ouz-a ouz-a deleted the opaque_cast_clean branch September 26, 2023 19:04
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 26, 2023
Monomorphize OpaqueCast

In previous attempt, rust-lang#116140 we thought `OpaqueCast` was unused during few places, but we were wrong, in reality we didn't have any test that could test it, so in this pr attempt is to correct the behavior of few `OpaqueCast`s in rustc.

r? `@lcnr`
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 27, 2023
Monomorphize OpaqueCast

In previous attempt, rust-lang#116140 we thought `OpaqueCast` was unused during few places, but we were wrong, in reality we didn't have any test that could test it, so in this pr attempt is to correct the behavior of few `OpaqueCast`s in rustc.

r? `@lcnr`
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 27, 2023
Monomorphize OpaqueCast

In previous attempt, rust-lang#116140 we thought `OpaqueCast` was unused during few places, but we were wrong, in reality we didn't have any test that could test it, so in this pr attempt is to correct the behavior of few `OpaqueCast`s in rustc.

r? `@lcnr`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants