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

Remove DefiningAnchor::Bubble from MIR validation #116803

Closed
wants to merge 2 commits into from
Closed

Remove DefiningAnchor::Bubble from MIR validation #116803

wants to merge 2 commits into from

Conversation

compiler-errors
Copy link
Member

If we're in the MIR validator, our body is local, and our reveal mode is Reveal::UserFacing, then use DefiningAnchor::Bind -- otherwise, we should never be constraining opaques, so use DefiningAnchor::Error like codegen does.

r? oli-obk

@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 Oct 16, 2023
@rustbot
Copy link
Collaborator

rustbot commented Oct 16, 2023

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

compiler-errors
compiler-errors commented Oct 16, 2023
View reviewed changes
compiler/rustc_const_eval/src/interpret/eval_context.rs Outdated
if util::relate_types(
tcx,
param_env,
DefiningAnchor::Error,
Copy link
Member Author

Choose a reason for hiding this comment

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

CTFE is always performed with a Reveal::All param-env (or it should, as far as I'm aware?) so it makes sense to use DefiningAnchor::Error here.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that's true? We run CTFE for const generics as well and the param-env is different there.
Cc @oli-obk @lcnr

Copy link
Member Author

@compiler-errors compiler-errors Oct 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

My justification is wrong here. We're not using a Reveal::All param-env, but we have passed this body through the RevealAll mir-pass.

Not totally clear if that's sufficient, but if it isn't, then we don't have a test for it.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can also approximate the defining anchor from the current stack frame. See newest commit for approach.

Copy link
Contributor

@lcnr lcnr Oct 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

My justification is wrong here. We're not using a Reveal::All param-env, but we have passed this body through the RevealAll mir-pass.

Not totally clear if that's sufficient, but if it isn't, then we don't have a test for it.

This is a bit of a mess. We re-use the result from running with Reveal::UserFacing when running with Reveal::All in CTFE. However, afaik there is (and well... should be) no call site actually using Reveal::UserFacing. I think we should switch to always use Reveal::All:

  • using Reveal::UserFacing after revealing opaque types can result in weird errors and failures, as we end up trying to relate an opaque and its revealed (but normally hidden) type
  • when people call a const-function returning impl Iterator<Item = u32> they actually want to be able to use the returned value as an iterator. The check that we only treat the type opaquely happened in typeck but during codegen this behavior is undesirable. I haven't thought about specialization and default associated types yet, but also, idc. cc Stop using Reveal::All before borrowck #101478 where I tried to change const generics CTFE to use Reveal::UserFacing.

RalfJung
RalfJung reviewed Oct 16, 2023
View reviewed changes
compiler/rustc_const_eval/src/util/compare_types.rs
Comment on lines +64 to +65
if anchor != DefiningAnchor::Error {
// With `Reveal::All`, opaque types get normalized away, with `Reveal::UserFacing`
Copy link
Member

Choose a reason for hiding this comment

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

The relationship between what the if is checking (DefiningAnchor, whatever that is) and what the comment talks about (Reveal::UserFacing/Reveal::All) is completely unclear.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well I didn't touch that comment, but I'll rework it I guess.

@RalfJung
Copy link
Member

Is there any explanation anywhere on what a DefiningAnchor is? The type sadly does not have a doc comment that would help here.

@oli-obk oli-obk mentioned this pull request Jan 10, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 10, 2024
@bors
Auto merge of rust-lang#119821 - oli-obk:reveal_all_const_evals, r=<try>
488c25a 
Always use RevealAll for const eval queries

implements what is described in rust-lang#116803 (comment)
@compiler-errors
Copy link
Member Author

i dont have time to continue with this

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 20, 2024
@bors
Auto merge of rust-lang#119821 - oli-obk:reveal_all_const_evals, r=lcnr
5378c1c 
Always use RevealAll for const eval queries

implements what is described in rust-lang#116803 (comment)

Using `UserFacing` for const eval does not make sense anymore, unless we significantly change things like avoiding revealing opaque types.

New tests are copied from rust-lang#101478
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Jan 21, 2024
@bors
Auto merge of #119821 - oli-obk:reveal_all_const_evals, r=lcnr
02804e0 
Always use RevealAll for const eval queries

implements what is described in rust-lang/rust#116803 (comment)

Using `UserFacing` for const eval does not make sense anymore, unless we significantly change things like avoiding revealing opaque types.

New tests are copied from rust-lang/rust#101478
lnicola pushed a commit to lnicola/rust-analyzer that referenced this pull request Apr 7, 2024
@bors
Auto merge of #119821 - oli-obk:reveal_all_const_evals, r=lcnr
2ea6980 
Always use RevealAll for const eval queries

implements what is described in rust-lang/rust#116803 (comment)

Using `UserFacing` for const eval does not make sense anymore, unless we significantly change things like avoiding revealing opaque types.

New tests are copied from rust-lang/rust#101478
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 27, 2024
@bors
Auto merge of #119821 - oli-obk:reveal_all_const_evals, r=lcnr
d15f277 
Always use RevealAll for const eval queries

implements what is described in rust-lang/rust#116803 (comment)

Using `UserFacing` for const eval does not make sense anymore, unless we significantly change things like avoiding revealing opaque types.

New tests are copied from rust-lang/rust#101478
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lcnr lcnr lcnr left review comments

@RalfJung RalfJung RalfJung left review comments

Assignees

@oli-obk oli-obk

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

Successfully merging this pull request may close these issues.
5 participants
@compiler-errors @rustbot @RalfJung @lcnr @oli-obk