Use a type visitor to collect type metadata, remove layout #68
Conversation
src/printer.rs
Outdated
| impl Visitor for TyCollector { | ||
| type Break = (); | ||
| fn visit_ty(&mut self, ty: &stable_mir::ty::Ty) -> ControlFlow<Self::Break> { | ||
| self.types.insert(*ty, (ty.kind(), None)); |
There was a problem hiding this comment.
Actually thinking about this again, I don't think this is a good idea, because the attempt to get a Layout failing is because we are still collecting Tys on unmonomorphised function like items. I think the vistor / collector needs to be corrected before we merge this on second thoughts.
There was a problem hiding this comment.
Agree this is indicative of a problem we have (quite probably in stdlib code only, but still).
We can maybe also just fix the collect_ty function to consider types within TyConst (that was the original motivation to look into this).
There was a problem hiding this comment.
I updated the visitor to resolve the unmonomorphised items like before.
Now, this visitor does a way better job of visiting every possible thing in these types. This is good, obviously, but it also means we hit some stuff we weren't previously that we don't know how to handle just yet. So, I've arranged the visitor to break on these items to avoid any panics. These are:
- Bound regions (
RegionKind::ReBound). Some of these have escaping vars that cause a message saying they can't be wrapped in a dummy binder. There may be some bound regions that don't have this issue, but at the moment I'm just skipping all of them. RigidTy::CoroutineWitness. Trying get the layout for these causes a panic. Maybe we just skip calling layout and useNone, but I have it skipping them altogether right now.
This passes the ui tests now. I think this is in a good place now and should make it much easier to special case specific things and track what we aren't supporting.
There was a problem hiding this comment.
Is this intended as a temporary fix? I am hesitant to move forward merging in code knowing that we are accessing MIR that is not resolved. But I understand that if we don't know where it is originating from and we need the Tys collected immediately we need to weigh that in too.
There was a problem hiding this comment.
Or am I misunderstanding what we are avoiding with breaking the control flow?
There was a problem hiding this comment.
The two bullet points I listed above are cases where we don't add the type to the metadata output. It's intended to prevent any panics at runtime, and it's temporary in the sense that we (I) don't understand why the panic happens or how to resolve it.
As far as whether or not we're still hitting some MIR, maybe we can determine somehow if we are with breakpoints/cases/print statements. Not sure.
This reverts commit b5975e2.
There was a problem hiding this comment.
I am a bit puzzled by the differences in the expectation files.
The code does not collect any function types any more ? That's OK I guess, we don't use them, but... we also don't get I8 any more (in this example)? Why not?
There was a problem hiding this comment.
I also can't understand why i8 is now removed from the map
There was a problem hiding this comment.
I noticed functions aren't in the map as well. I think it's because they get inserted by Stephen's code before monomorphization:
if val_collector
.visited_tys
.insert(val, (val.kind(), maybe_layout_shape))
.is_some()
{
match val.kind() { The monomorphization gets done inside the match statement, but the function is already added to the map! In my code, the argument and return types for the function are added only. But, we should probably figure out how to get the resolved function signature and add it as well.
Not sure why the I8 disappears, idk why it's there in the first place either. The integers in the source don't have any type annotations, rust defaults to I32 (which is in the type map), so there shouldn't be any issue with that.
|
Some IDs seem to be unstable (type info records change location in the expectation files on different runs). We could sort the type infos by something that stabilises the order (the |
I think this is the correct way to access the function signature args for a function pointer, even though it goes through internal. I checked kani kani-compiler/src/codegen_cprover_gotoc/codegen/statement.rs and this is what they are doing. There was one failure `rust/tests/ui/issues/issue-26641.rs (exit 101)` that uses a `RigidTy::Dynamic(Binder...)` but we haven't really looked at details for `dyn` and `vtable` yet so this is moved to failing for now. Also improved the UI testing output but adding verbosity and some metrics
Fixes #67
As discussed, using a type visitor to collect type information is more robust than the prior
collect_ty.Layout information causes issues with polymorphic functions again, so we do not collect it any more for now as we aren't currently using it at all in
mir-semantics.