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

Revert #119627 "Remove all ConstPropNonsense" #120347

Closed
wants to merge 1 commit into from

Conversation

djkoloski
Copy link
Contributor

This reverts commit 68411c9, reversing changes made to 7ffc697.

Original PR: #119627
Fixes: #120337

cc @oli-obk, @matthiaskrgr (maybe you want to review this?)

@rustbot
Copy link
Collaborator

rustbot commented Jan 25, 2024

r? @compiler-errors

(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 Jan 25, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jan 25, 2024

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@matthiaskrgr
Copy link
Member

It would be nice to also add a test to make sure this does not regress again :)

@djkoloski
Copy link
Contributor Author

Can do!

@oli-obk
Copy link
Contributor

oli-obk commented Jan 25, 2024

I'd prefer to just add a line to avoid this ICE instead of reverting the entire PR. I can do that tomorrow. If you want to do it now, it should be as simple as getting a backtrace and checking the call site to the entry point to the interpreter. I did a similar thing here: https://github.com/rust-lang/rust/pull/119627/files#r1452800719

@compiler-errors
Copy link
Member

I can put up that PR @oli-obk

@compiler-errors
Copy link
Member

Less invasive fix in #120350

@djkoloski
Copy link
Contributor Author

Sorry got pulled away by meetings. @compiler-errors let's land your change. I'm closing this PR for now, I can reopen if we need.

@djkoloski djkoloski closed this Jan 25, 2024
@djkoloski djkoloski reopened this Jan 25, 2024
@djkoloski
Copy link
Contributor Author

Reopening since it looks like the other PR hit a snag. I'll add the regression tests requested.

@compiler-errors
Copy link
Member

r? oli-obk

I assume that you'll have the same difficulty in creating a faithful regression test as #120350 does? In any case, I want to leave this up to oli to decide whether to do a full revert.

@rustbot rustbot assigned oli-obk and unassigned compiler-errors Jan 25, 2024
@RalfJung
Copy link
Member

RalfJung commented Jan 25, 2024 via email

@djkoloski djkoloski changed the title Revert "Auto merge of #119627" Revert #119627 "Remove all ConstPropNonsense" Jan 25, 2024
This reverts commit 68411c9, reversing
changes made to 7ffc697. This commit
was checked in as PR rust-lang#119627.
@djkoloski
Copy link
Contributor Author

Updated the PR and commit title. The previous title was the default generated by git revert.

As @compiler-errors suggested, I'm going to leave this as a clean revert due to the difficulty of adding a regression test. If we had that figured out then we would just check in the fix PR.

I'll leave it up to the reviewer whether a revert is sufficient.

@rustbot ready

@oli-obk
Copy link
Contributor

oli-obk commented Jan 26, 2024

Please change this to only revert 1f398ab (as per the discussion in the other PR, we'll follow up with just removing the assertion)

r=me with that

@oli-obk
Copy link
Contributor

oli-obk commented Jan 26, 2024

Ah #120367 removes the assertion, let's go with that

@oli-obk oli-obk closed this Jan 26, 2024
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.

ice: !layout.abi.is_uninhabited()
6 participants