Skip to content

Deduplicate pretty printing of constants #67133

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

Merged
merged 18 commits into from
Mar 16, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions src/librustc/mir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2562,15 +2562,15 @@ impl<'tcx> Debug for Constant<'tcx> {

impl<'tcx> Display for Constant<'tcx> {
fn fmt(&self, fmt: &mut Formatter<'_>) -> fmt::Result {
use crate::ty::print::PrettyPrinter;
write!(fmt, "const ")?;
// FIXME make the default pretty printing of raw pointers more detailed. Here we output the
// debug representation of raw pointers, so that the raw pointers in the mir dump output are
// detailed and just not '{pointer}'.
if let ty::RawPtr(_) = self.literal.ty.kind {
write!(fmt, "{:?} : {}", self.literal.val, self.literal.ty)
} else {
write!(fmt, "{}", self.literal)
}
ty::tls::with(|tcx| {
let literal = tcx.lift(&self.literal).unwrap();
let mut cx = FmtPrinter::new(tcx, fmt, Namespace::ValueNS);
cx.print_alloc_ids = true;
cx.pretty_print_const(literal, true)?;
Ok(())
})
}
}

Expand Down
400 changes: 295 additions & 105 deletions src/librustc/ty/print/pretty.rs

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions src/librustc_mir/interpret/intrinsics/type_name.rs
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@ impl<'tcx> Printer<'tcx> for AbsolutePathPrinter<'tcx> {
}
}
}

impl PrettyPrinter<'tcx> for AbsolutePathPrinter<'tcx> {
fn region_should_not_be_omitted(&self, _region: ty::Region<'_>) -> bool {
false
Expand Down
83 changes: 41 additions & 42 deletions src/librustc_mir/interpret/operand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,20 @@

use std::convert::{TryFrom, TryInto};

use rustc::ty::layout::{
self, HasDataLayout, IntegerExt, LayoutOf, PrimitiveExt, Size, TyLayout, VariantIdx,
};
use rustc::{mir, ty};

use super::{InterpCx, MPlaceTy, Machine, MemPlace, Place, PlaceTy};
pub use rustc::mir::interpret::ScalarMaybeUndef;
use rustc::mir::interpret::{
sign_extend, truncate, AllocId, ConstValue, GlobalId, InterpResult, Pointer, Scalar,
};
use rustc_ast::ast;
use rustc::ty::layout::{
self, HasDataLayout, IntegerExt, LayoutOf, PrimitiveExt, Size, TyLayout, VariantIdx,
};
use rustc::ty::print::{FmtPrinter, PrettyPrinter, Printer};
use rustc::ty::Ty;
use rustc::{mir, ty};
use rustc_hir::def::Namespace;
use rustc_macros::HashStable;
use std::fmt::Write;

/// An `Immediate` represents a single immediate self-contained Rust value.
///
Expand Down Expand Up @@ -92,47 +94,44 @@ pub struct ImmTy<'tcx, Tag = ()> {
pub layout: TyLayout<'tcx>,
}

// `Tag: Copy` because some methods on `Scalar` consume them by value
impl<Tag: Copy> std::fmt::Display for ImmTy<'tcx, Tag> {
fn fmt(&self, fmt: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match &self.imm {
// We cannot use `to_bits_or_ptr` as we do not have a `tcx`.
// So we use `is_bits` and circumvent a bunch of sanity checking -- but
// this is anyway only for printing.
Immediate::Scalar(ScalarMaybeUndef::Scalar(s)) if s.is_ptr() => {
fmt.write_str("{pointer}")
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
/// Helper function for printing a scalar to a FmtPrinter
fn p<'a, 'tcx, F: std::fmt::Write, Tag>(
cx: FmtPrinter<'a, 'tcx, F>,
s: ScalarMaybeUndef<Tag>,
ty: Ty<'tcx>,
) -> Result<FmtPrinter<'a, 'tcx, F>, std::fmt::Error> {
match s {
ScalarMaybeUndef::Scalar(s) => {
cx.pretty_print_const_scalar(s.erase_tag(), ty, true)
}
ScalarMaybeUndef::Undef => cx.typed_value(
|mut this| {
this.write_str("{undef ")?;
Ok(this)
},
|this| this.print_type(ty),
" ",
),
}
Immediate::Scalar(ScalarMaybeUndef::Scalar(s)) => {
let s = s.assert_bits(self.layout.size);
match self.layout.ty.kind {
ty::Int(_) => {
return write!(fmt, "{}", super::sign_extend(s, self.layout.size) as i128,);
}
ty::Uint(_) => return write!(fmt, "{}", s),
ty::Bool if s == 0 => return fmt.write_str("false"),
ty::Bool if s == 1 => return fmt.write_str("true"),
ty::Char => {
if let Some(c) = u32::try_from(s).ok().and_then(std::char::from_u32) {
return write!(fmt, "{}", c);
}
}
ty::Float(ast::FloatTy::F32) => {
if let Ok(u) = u32::try_from(s) {
return write!(fmt, "{}", f32::from_bits(u));
}
}
ty::Float(ast::FloatTy::F64) => {
if let Ok(u) = u64::try_from(s) {
return write!(fmt, "{}", f64::from_bits(u));
}
}
ty::tls::with(|tcx| {
match self.imm {
Immediate::Scalar(s) => {
if let Some(ty) = tcx.lift(&self.layout.ty) {
let cx = FmtPrinter::new(tcx, f, Namespace::ValueNS);
p(cx, s, ty)?;
return Ok(());
}
_ => {}
write!(f, "{:?}: {}", s.erase_tag(), self.layout.ty)
}
Immediate::ScalarPair(a, b) => {
// FIXME(oli-obk): at least print tuples and slices nicely
write!(f, "({:?}, {:?}): {}", a.erase_tag(), b.erase_tag(), self.layout.ty,)
}
write!(fmt, "{:x}", s)
}
Immediate::Scalar(ScalarMaybeUndef::Undef) => fmt.write_str("{undef}"),
Immediate::ScalarPair(..) => fmt.write_str("{wide pointer or tuple}"),
}
})
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/test/mir-opt/const-promotion-extern-static.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ fn main() {}
// START rustc.FOO.PromoteTemps.before.mir
// bb0: {
// ...
// _5 = const Scalar(alloc1+0) : &i32;
// _5 = const {alloc1+0: &i32};
Copy link
Member

Choose a reason for hiding this comment

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

If +0 is common, maybe it should be omitted?
And maybe it should say &alloc1: &i32, and for a raw pointer be &raw const alloc1: *const i32.
Or we could have &(alloc1: i32) (and no braces?), not sure which one is nicer.

Copy link
Member

Choose a reason for hiding this comment

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

If +0 is common, maybe it should be omitted?

I feel that could be confusing... it is more consistent to always have the offset.

Copy link
Member

Choose a reason for hiding this comment

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

I almost feel like {alloc1+12: &i32} should be this monster:

&*((&raw const alloc1).add(12) as *const i32)

Is that too much? I don't know.

I care much more about showing what alloc1 actually consists of, somewhere in the same file, than the syntax here though.

Copy link
Member

Choose a reason for hiding this comment

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

OMG WTF.^^
At least for Miri tracing, that would be a disaster -- way too verbose. And it's still not actual Rust code so why bother?

Copy link
Member

Choose a reason for hiding this comment

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

And it's still not actual Rust code so why bother?

I think I agree for miri purposes, but I'd prefer if constants in MIR were as close to Rust as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to address this when also starting to emit the allocations

// _4 = &(*_5);
// _3 = [move _4];
// _2 = &_3;
Expand All @@ -31,7 +31,7 @@ fn main() {}
// START rustc.BAR.PromoteTemps.before.mir
// bb0: {
// ...
// _5 = const Scalar(alloc0+0) : &i32;
// _5 = const {alloc0+0: &i32};
// _4 = &(*_5);
// _3 = [move _4];
// _2 = &_3;
Expand Down
2 changes: 1 addition & 1 deletion src/test/mir-opt/const_prop/discriminant.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ fn main() {
// START rustc.main.ConstProp.after.mir
// bb0: {
// ...
// _3 = const Scalar(0x01) : std::option::Option<bool>;
// _3 = const {transmute(0x01): std::option::Option<bool>};
// _4 = const 1isize;
// switchInt(const 1isize) -> [1isize: bb2, otherwise: bb1];
// }
Expand Down
2 changes: 1 addition & 1 deletion src/test/mir-opt/const_prop/issue-66971.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ fn main() {
// START rustc.main.ConstProp.after.mir
// bb0: {
// ...
// _3 = const Scalar(<ZST>) : ();
// _3 = const ();
// _2 = (move _3, const 0u8, const 0u8);
// ...
// _1 = const encode(move _2) -> bb1;
Expand Down
4 changes: 2 additions & 2 deletions src/test/mir-opt/const_prop/read_immutable_static.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@ fn main() {
// START rustc.main.ConstProp.before.mir
// bb0: {
// ...
// _3 = const Scalar(alloc0+0) : &u8;
// _3 = const {alloc0+0: &u8};
// _2 = (*_3);
// ...
// _5 = const Scalar(alloc0+0) : &u8;
// _5 = const {alloc0+0: &u8};
Copy link
Member

Choose a reason for hiding this comment

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

We should probably make sure that when printing out some MIR we include all of the referenced allocations as well. Please make an issue and add FIXMEs in every file that mentions miri allocs anywhere (also, MIR tests should include the definitions of allocs).

// _4 = (*_5);
// _1 = Add(move _2, move _4);
// ...
Expand Down
24 changes: 12 additions & 12 deletions src/test/mir-opt/simplify-locals-removes-unused-consts.rs
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
// compile-flags: -C overflow-checks=no

fn use_zst(_: ((), ())) { }
fn use_zst(_: ((), ())) {}

struct Temp {
x: u8
x: u8,
}

fn use_u8(_: u8) { }
fn use_u8(_: u8) {}

fn main() {
let ((), ()) = ((), ());
use_zst(((), ()));

use_u8((Temp { x : 40 }).x + 2);
use_u8((Temp { x: 40 }).x + 2);
}

// END RUST SOURCE
Expand All @@ -35,28 +35,28 @@ fn main() {
// bb0: {
// StorageLive(_1);
// StorageLive(_2);
// _2 = const Scalar(<ZST>) : ();
// _2 = const ();
// StorageLive(_3);
// _3 = const Scalar(<ZST>) : ();
// _1 = const Scalar(<ZST>) : ((), ());
// _3 = const ();
// _1 = const {transmute(()): ((), ())};
// StorageDead(_3);
// StorageDead(_2);
// StorageDead(_1);
// StorageLive(_4);
// StorageLive(_6);
// _6 = const Scalar(<ZST>) : ();
// _6 = const ();
// StorageLive(_7);
// _7 = const Scalar(<ZST>) : ();
// _7 = const ();
// StorageDead(_7);
// StorageDead(_6);
// _4 = const use_zst(const Scalar(<ZST>) : ((), ())) -> bb1;
// _4 = const use_zst(const {transmute(()): ((), ())}) -> bb1;
// }
// bb1: {
// StorageDead(_4);
// StorageLive(_8);
// StorageLive(_10);
// StorageLive(_11);
// _11 = const Scalar(0x28) : Temp;
// _11 = const {transmute(0x28) : Temp};
// _10 = const 40u8;
// StorageDead(_10);
// _8 = const use_u8(const 42u8) -> bb2;
Expand All @@ -75,7 +75,7 @@ fn main() {
// }
// bb0: {
// StorageLive(_1);
// _1 = const use_zst(const Scalar(<ZST>) : ((), ())) -> bb1;
// _1 = const use_zst(const {transmute(()): ((), ())}) -> bb1;
// }
// bb1: {
// StorageDead(_1);
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/const-generics/cannot-infer-const-args.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ error[E0282]: type annotations needed
--> $DIR/cannot-infer-const-args.rs:9:5
|
LL | foo();
| ^^^ cannot infer type for fn item `fn() -> usize {foo::<_: usize>}`
| ^^^ cannot infer type for fn item `fn() -> usize {foo::<{_: usize}>}`

error: aborting due to previous error

Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/const-generics/const-generic-type_name.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,5 @@
struct S<const N: usize>;

fn main() {
assert_eq!(std::any::type_name::<S<3>>(), "const_generic_type_name::S<3usize>");
assert_eq!(std::any::type_name::<S<3>>(), "const_generic_type_name::S<3>");
}
12 changes: 6 additions & 6 deletions src/test/ui/const-generics/fn-const-param-infer.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,12 @@ error[E0308]: mismatched types
--> $DIR/fn-const-param-infer.rs:16:31
|
LL | let _: Checked<not_one> = Checked::<not_two>;
| ---------------- ^^^^^^^^^^^^^^^^^^ expected `not_one`, found `not_two`
| ---------------- ^^^^^^^^^^^^^^^^^^ expected `{not_one as fn(usize) -> bool}`, found `{not_two as fn(usize) -> bool}`
| |
| expected due to this
|
= note: expected struct `Checked<not_one>`
found struct `Checked<not_two>`
= note: expected struct `Checked<{not_one as fn(usize) -> bool}>`
found struct `Checked<{not_two as fn(usize) -> bool}>`

error[E0308]: mismatched types
--> $DIR/fn-const-param-infer.rs:20:24
Expand All @@ -36,12 +36,12 @@ error[E0308]: mismatched types
--> $DIR/fn-const-param-infer.rs:25:40
|
LL | let _: Checked<{generic::<u32>}> = Checked::<{generic::<u16>}>;
| ------------------------- ^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `generic::<u32>`, found `generic::<u16>`
| ------------------------- ^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `{generic::<u32> as fn(usize) -> bool}`, found `{generic::<u16> as fn(usize) -> bool}`
| |
| expected due to this
|
= note: expected struct `Checked<generic::<u32>>`
found struct `Checked<generic::<u16>>`
= note: expected struct `Checked<{generic::<u32> as fn(usize) -> bool}>`
found struct `Checked<{generic::<u16> as fn(usize) -> bool}>`

error: aborting due to 4 previous errors

Expand Down
4 changes: 2 additions & 2 deletions src/test/ui/const-generics/raw-ptr-const-param.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,6 @@
struct Const<const P: *const u32>;

fn main() {
let _: Const<{15 as *const _}> = Const::<{10 as *const _}>; //~ mismatched types
let _: Const<{10 as *const _}> = Const::<{10 as *const _}>;
let _: Const<{ 15 as *const _ }> = Const::<{ 10 as *const _ }>; //~ mismatched types
let _: Const<{ 10 as *const _ }> = Const::<{ 10 as *const _ }>;
}
10 changes: 5 additions & 5 deletions src/test/ui/const-generics/raw-ptr-const-param.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,15 @@ LL | #![feature(const_generics, const_compare_raw_pointers)]
= note: `#[warn(incomplete_features)]` on by default

error[E0308]: mismatched types
--> $DIR/raw-ptr-const-param.rs:7:38
--> $DIR/raw-ptr-const-param.rs:7:40
|
LL | let _: Const<{15 as *const _}> = Const::<{10 as *const _}>;
| ----------------------- ^^^^^^^^^^^^^^^^^^^^^^^^^ expected `{pointer}`, found `{pointer}`
LL | let _: Const<{ 15 as *const _ }> = Const::<{ 10 as *const _ }>;
| ------------------------- ^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `{0xf as *const u32}`, found `{0xa as *const u32}`
| |
| expected due to this
|
= note: expected struct `Const<{pointer}>`
found struct `Const<{pointer}>`
= note: expected struct `Const<{0xf as *const u32}>`
found struct `Const<{0xa as *const u32}>`

error: aborting due to previous error

Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/consts/offset_from_ub.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ error: any use of this value will cause an error
LL | intrinsics::ptr_offset_from(self, origin)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
| |
| exact_div: 1 cannot be divided by 2 without remainder
| exact_div: 1isize cannot be divided by 2isize without remainder
| inside call to `std::ptr::const_ptr::<impl *const u16>::offset_from` at $DIR/offset_from_ub.rs:36:14
|
::: $DIR/offset_from_ub.rs:31:1
Expand Down