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

Duplicate definition of type for rustc::mir::Constant #56137

Closed
bjorn3 opened this issue Nov 21, 2018 · 6 comments · Fixed by #63495
Closed

Duplicate definition of type for rustc::mir::Constant #56137

bjorn3 opened this issue Nov 21, 2018 · 6 comments · Fixed by #63495
Assignees
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html

Comments

@bjorn3
Copy link
Member

bjorn3 commented Nov 21, 2018

rustc::mir::Constant has a field ty, but its field literal contains a Const which itself already contains a Ty.

@oli-obk
Copy link
Contributor

oli-obk commented Nov 21, 2018

These types aren't always equal. e.g. the lifetimes often differ, and I think around impl trait some inconsistencies can happen, too. we should probably document this much better and/or figure out whether we need the duplication

@oli-obk oli-obk added the A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html label Nov 21, 2018
@eddyb eddyb self-assigned this Aug 12, 2019
@eddyb
Copy link
Member

eddyb commented Aug 12, 2019

Looks like #61041 is blocked on fixing this.

EDIT: to expand on that, this example:

trait Zero {
    type Repr;
    const ZERO: Self::Repr;
}

impl Zero for char {
    type Repr = u32;
    const ZERO: u32 = 0;
}

const ONE: u32 = char::ZERO + 1;

Has this for the char::ZERO in the MIR of ONE:

// mir::Constant
// + span: src/lib.rs:11:18: 11:28
// + ty: u32
// + literal: Const { ty: <Self as Zero>::Repr, val: Unevaluated(DefId(0:14 ~ playground[5f8c]::Zero[0]::ZERO[0]), [char]) }

Note the <Self as Zero>::Repr in literal.ty, which is clearly wrong for ONE (appears to be unsubstituted), while ty is correct.

@RalfJung

This comment has been minimized.

@RalfJung

This comment has been minimized.

@eddyb
Copy link
Member

eddyb commented Aug 12, 2019

@RalfJung All of that is fine. ty::Const is "unusual" because it's one of the 3 "entities" (Ty and ty::Region have the same treatment).

I'm already fixing this. MIR construction is just wrong and likely has been since forever.
MIRI kept papering over this issue by not using the ty field of ty::Const.

@eddyb
Copy link
Member

eddyb commented Aug 12, 2019

Huh, looks like the bug is pretty recent: 5cd2806#diff-c599d3d6879ad974960a24abd4e4bdb6R953
It was introduced in #59178, after this issue - had we removed the duplication earlier, the bug would've been prevented, heh.

Centril added a commit to Centril/rust that referenced this issue Aug 14, 2019
 Remove redundant `ty` fields from `mir::Constant` and `hair::pattern::PatternRange`.

Fixes rust-lang#56137.

As a side-effect, associated const literals have the correct type now, which should make things easier for rust-lang#61041.

r? @oli-obk / @matthewjasper cc @davidtwco @varkor
Centril added a commit to Centril/rust that referenced this issue Aug 14, 2019
 Remove redundant `ty` fields from `mir::Constant` and `hair::pattern::PatternRange`.

Fixes rust-lang#56137.

As a side-effect, associated const literals have the correct type now, which should make things easier for rust-lang#61041.

r? @oli-obk / @matthewjasper cc @davidtwco @varkor
Centril added a commit to Centril/rust that referenced this issue Aug 14, 2019
 Remove redundant `ty` fields from `mir::Constant` and `hair::pattern::PatternRange`.

Fixes rust-lang#56137.

As a side-effect, associated const literals have the correct type now, which should make things easier for rust-lang#61041.

r? @oli-obk / @matthewjasper cc @davidtwco @varkor
Centril added a commit to Centril/rust that referenced this issue Aug 14, 2019
 Remove redundant `ty` fields from `mir::Constant` and `hair::pattern::PatternRange`.

Fixes rust-lang#56137.

As a side-effect, associated const literals have the correct type now, which should make things easier for rust-lang#61041.

r? @oli-obk / @matthewjasper cc @davidtwco @varkor
Centril added a commit to Centril/rust that referenced this issue Aug 15, 2019
 Remove redundant `ty` fields from `mir::Constant` and `hair::pattern::PatternRange`.

Fixes rust-lang#56137.

As a side-effect, associated const literals have the correct type now, which should make things easier for rust-lang#61041.

r? @oli-obk / @matthewjasper cc @davidtwco @varkor
@bors bors closed this as completed in db3bae0 Aug 16, 2019
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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants