Support nightly-2023-01-23
#49
Conversation
Previously, we would incorrectly produce types like `dyn Fn<((),)>`, where no `Output` type is specified, causing a panic later due to an unnormalized projection. Now we correctly look at supertraits (in this case, `FnOnce`, where `Output` is defined) to make sure we don't miss any associated type predicates.
Without the disambiguation, allocations from different crates can clash, leading to inexplicable errors.
This hack is responsible for `mir-json-callgraph`'s results excluding some important information. It is unclear why this hack was necessary in the first place, so let's just remove it.
`&str` is an unsized type, and the `Layout` for an unsized type has a `size` of `0`, which isn't what we want. As far as I can tell, the proper way to compute the size of an unsized type is to use `MPlaceTy`'s `len` function, which sadly isn't exported. I have copied it here and used it.
`AnonConst`s are like `Fn`s in that they have `DefId`s and MIR boies, but they aren't actual functions. As such, we don't want to emit `#[test]` for `AnonConst`s. Fixes #47.
There was a problem hiding this comment.
Looks good overall. I left a bunch of comments, but most are trivial, like removing old commented-out code. I tried to leave suggestions for these so you can apply them directly in the github UI. The non-trivial comments are around DynKind::DynStar and test cases for non-literal array sizes (e.g. const SIZE: usize = 123; ... [i32; SIZE] ...).
src/analyz/to_json.rs
Outdated
| projs.push(ty::ExistentialProjection { | ||
| def_id: ai.def_id, | ||
| substs: ex_trait_ref.substs, | ||
| term: actual_ty.into(), // TO REVIEW: structure of Term has changed | ||
| }); |
There was a problem hiding this comment.
What does this "to review" comment refer to?
There was a problem hiding this comment.
I spoke to @m10f. This comment is referring to the fact that this rust commit (rust-lang/rust@79db32b) changed Term from an enum:
pub enum Term<'tcx> {
Ty(Ty<'tcx>),
Const(Const<'tcx>),
}To a struct:
pub struct Term<'tcx> {
ptr: NonZeroUsize,
marker: PhantomData<(Ty<'tcx>, Const<'tcx>)>,
}And this comment served as a reminder to check if changing this line in mir-json from term: ty::Term::Ty(actual_ty) to term: actual_ty.into() is the proper way to migrate the code. That being said, I took a closer look at the rust commit, and I am fairly certain that this is correct. That commit also migrates a similar bit of code here in the exact same way:
- term: Term::Ty(output_ty),
+ term: output_ty.into(),As such, I think the current code is fine. I will remove the TO REVIEW comment and land this, but feel free to object if I've overlooked something.
There was a problem hiding this comment.
Yes, that sounds like the correct fix. The new Term is a sort of "packed enum" that uses (I assume) pointer tagging instead of a separate discriminant field to distinguish the Ty and Const cases, so it's only one word wide instead of two.
Co-authored-by: spernsteiner <spernsteiner@galois.com>
This is a valid way to convert from a `Ty` to a `Term`, as the commit which changed `Term` to a struct does the same thing here: rust-lang/rust@79db32b#diff-ea6f9d55e90333a1f5fc5e202d46862a6a031410831f2df42317ac024f821162R352
This is a fairly large overhaul of
mir-json's internals to support a more recent Rust nightly (nightly-2023-01-23). The work is split between myself, @m10f, and @spernsteiner.There is a lot happening here, so if you want to learn specifics, I'd encourage you to look at individual commits. Some of the broad themes in the patchset include:
enumdiscriminant types so thatcrucible-mircan know about non-isizediscriminant types (e.g.,Ordering, which uses ani8discriminant)mir-jsonemit a--sysrootargument when invokingrustcso thatrustcknows how to disambiguate it from the default sysroot&str, analogous to a special case for&[T]AnonConsts aren't marked with#[test]This should fix several issues:
saw-rustccompilation failure (#[test] can only be applied to functions) #47