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

Clean up cast categorization #97649

Open
RalfJung opened this issue Jun 2, 2022 · 5 comments
Open

Clean up cast categorization #97649

RalfJung opened this issue Jun 2, 2022 · 5 comments
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html A-technical-debt Area: Internal cleanup work C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Jun 2, 2022

Currently, our classification of casts is kind of a mess:

  • We have mir::CastKind, but it has a Misc variant and most casts fall in that category (this is what ends up in the MIR).
  • We also have ty::cast::CastKind that has a seemingly independent classification (no idea where this is used).
  • We have a CastTy type, but can it really represent all types in a cast? If yes, I'd expect to see unwraps around here.
  • In thir::ExprKind, we have Cast and Pointer expressions, the latter also being casts.

I don't know how these all relate, but it looks like some cleanup is dearly necessary here.

@RalfJung RalfJung added C-cleanup Category: PRs that clean code up or issues documenting cleanup. A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html A-technical-debt Area: Internal cleanup work labels Jun 2, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Jun 3, 2022
add cast kind of from_exposed_addr (int-to-ptr casts)

This is basically the dual to rust-lang#97582, for int2ptr casts.

Cc `@tmiasko` rust-lang#97649
flip1995 pushed a commit to flip1995/rust-clippy that referenced this issue Jun 4, 2022
add cast kind of from_exposed_addr (int-to-ptr casts)

This is basically the dual to rust-lang/rust#97582, for int2ptr casts.

Cc `@tmiasko` rust-lang/rust#97649
@JakobDegen
Copy link
Contributor

ty::cast::CastKind is used by type checking and can probably be left alone. That area has its own needs anyway, especially because of diagnostics and such things.

Besides that, I've taken a look at some of the surrounding stuff. My rough guess is that the principled thing to do here would be as follows: First, stop sharing types (specifically PointerCast) between THIR/MIR and HIR. HIR has a notion of adjustments that are removed and made explicit in THIR lowering. Instead, we should add a CastKind that is used by THIR and MIR and covers all the casts that exist in those IRs. This CastKind would then replace mir::CastKind in Rvalue::Cast and we would unify thir::ExprKind::{Cast,Pointer} into one variant that also uses this CastKind.

As to what this CastKind should look like, I'm not so sure. I see two possibilities:

  1. We take the current approach, which is to make it an enum that just exhaustively lists all the permitted cast categories
  2. We make it a { src: CastTy, dest: CastTy } pair. This would require also making CastTy exhaustive (it currently misses some categories of types, eg FnDef). Essentially this is just giving up on enumerating the permitted set of casts in the type system, but maybe that's not so bad... clearly our current attempt to do it isn't exactly panning out.

I'm not sure who is best to ask about the viability of this plan, so cc @oli-obk I guess?

@RalfJung
Copy link
Member Author

RalfJung commented Jul 4, 2022

stop sharing types (specifically PointerCast) between THIR/MIR and HIR

Fully agreed.

As to what this CastKind should look like, I'm not so sure. I see two possibilities:

The source and dest type are available anyway, right? In MIR at least, the source is an operand and that always has a type. So only a destination type would be needed.

Miri indeed mostly ignores the CastKind and just looks at the pair of source and destination types. However, two kinds of casts are very different from the others and should be somehow singled out: ptr2int and int2ptr. In fact maybe they should become their own Rvalue; unlike the other casts, they are both in some sense "not pure" (ptr2int is expose_addr and has a side-effect; int2ptr is from_exposed_addr and makes a non-deterministic choice).

@oli-obk
Copy link
Contributor

oli-obk commented Jul 4, 2022

ptr2int and int2ptr seem like fairly ok candidates for intrinsics instead of a special MIR rvalue. I think the only thing that cares about them is const checking, and we could just not mark ptr2int as const.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 4, 2022

I do prefer full enumeration of all casts, mostly because it keeps biting us to redo categorization everywhere.

@RalfJung
Copy link
Member Author

RalfJung commented Jul 4, 2022

ptr2int and int2ptr seem like fairly ok candidates for intrinsics instead of a special MIR rvalue. I think the only thing that cares about them is const checking, and we could just not mark ptr2int as const.

I would say they are great candidates for the new StatementKind::Intrinsic that keeps coming up. ;)

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Oct 8, 2022
Remove `mir::CastKind::Misc`

As discussed in rust-lang#97649 `mir::CastKind::Misc` is not clear, this PR addresses that by creating a new enum variant for every valid cast.

r? `@oli-obk`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 8, 2022
Remove `mir::CastKind::Misc`

As discussed in rust-lang#97649 `mir::CastKind::Misc` is not clear, this PR addresses that by creating a new enum variant for every valid cast.

r? ``@oli-obk``
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Oct 8, 2022
Remove `mir::CastKind::Misc`

As discussed in rust-lang#97649 `mir::CastKind::Misc` is not clear, this PR addresses that by creating a new enum variant for every valid cast.

r? ```@oli-obk```
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 8, 2022
Remove `mir::CastKind::Misc`

As discussed in rust-lang#97649 `mir::CastKind::Misc` is not clear, this PR addresses that by creating a new enum variant for every valid cast.

r? ````@oli-obk````
flip1995 pushed a commit to flip1995/rust that referenced this issue Oct 20, 2022
Remove `mir::CastKind::Misc`

As discussed in rust-lang#97649 `mir::CastKind::Misc` is not clear, this PR addresses that by creating a new enum variant for every valid cast.

r? ````@oli-obk````
@Noratrieb Noratrieb added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html A-technical-debt Area: Internal cleanup work C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants