Skip to content

Add a method to create an arbitrary OpTy from outside rustc_mir #61718

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

bjorn3
Copy link
Member

@bjorn3 bjorn3 commented Jun 10, 2019

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 10, 2019
@bjorn3 bjorn3 force-pushed the priroda_read_local_value branch from 0a38eb4 to 5ee6f8a Compare June 10, 2019 14:26
{
/// This is used by [priroda](https://github.com/oli-obk/priroda) to create an `OpTy`.
///
/// Do not use this from inside rustc.
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 expand a bit on why this shouldn't be used from inside rustc? Is it just that this operation is unchecked or does it violate some other assumption?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought that somebody wanted to prevent having a method like this to prevent accidental misusage or something like that, but in #55179 I can really find anything like that except for "I wanted to have something like an abstraction here" by @RalfJung.

Copy link
Member

Choose a reason for hiding this comment

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

The *Ty types have successfully found and prevent several bugs. Currently the only "hole" in the scheme is ImmTy, you can construct arbitrary immediates from the outside. I was very happy when I managed to make OpTy private and only expose ImmTy instead, having to un-do this would be very sad. :(

Copy link
Member

Choose a reason for hiding this comment

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

Really the advice here should be "do not use this, period". This is not an inside-rustc vs outside-rustc distinction.

@@ -395,7 +405,6 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> InterpretCx<'a, 'mir, 'tcx, M>
})
}

/// This is used by [priroda](https://github.com/oli-obk/priroda) to get an OpTy from a local
pub fn access_local(
Copy link
Member

Choose a reason for hiding this comment

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

Does that mean we can make the method private?

@RalfJung
Copy link
Member

This is necessary to make priroda work again without the ugly catch panic hack at https://github.com/oli-obk/priroda/blob/0810d79b474f6ad66686c664925fd595f011c581/src/render/locals.rs#L39-L54.

Looking at that, what about making access_local return an Option<OpTy> instead?

@bjorn3
Copy link
Member Author

bjorn3 commented Jun 10, 2019

Looking at that, what about making access_local return an Option instead?

That's possible.

@RalfJung
Copy link
Member

I'd strongly prefer that.

@oli-obk
Copy link
Contributor

oli-obk commented Jun 19, 2019

Closing in favour of making access_local return an Option

@oli-obk oli-obk closed this Jun 19, 2019
@bjorn3 bjorn3 deleted the priroda_read_local_value branch July 10, 2019 18:25
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants