Implement type_name() for TypeRef
#2539
Conversation
When running `tool_windows` on the latest `.winmd` generated with microsoft/win32metadata#1591 to test the results of that metadata change, no implementation exists for `TypeRef`. This implementation is rather trivial/straightforward based on `TypeDef`. Going forward, also print the failed value when a `match` statement ends up in `_ => unimplemented!()` so that it is quicker to immediately debug what enum variant needs to be implemented.
|
Note that while These seem to come from EDIT: Regen branch at https://github.com/MarijnS95/windows-rs/compare/regen |
|
I haven't taken the last few releases of the Win32 metadata as there are a number of problematic changes that block Rust adoption. I also have some major changes coming to metadata and code generation and I'd like to land those before dealing with the metadata breaking changes. |
|
@kennykerr ah that's unfortunate, I'd love to have the updated metadata in windows-rs at some point for these small improvements, but makes sense to delay that until getting those major changes in. Will sit tight, thanks! |
|
@kennykerr that doesn't make this PR invalid though? It is still very much necessary to compile with the latest win32metadata and it still rebases nicely on top of Since that's in it sounds like we're unblocked here now to look into "dealing with the metadata breaking changes", this can be reopened? |
|
There are just a bunch of incompatibilities that were introduced by the Win32/Wdk metadata in the last few releases that need to be dealt with. I've worked on, among other things, making the error reporting through |
| @@ -156,13 +179,14 @@ impl<'a> Gen<'a> { | |||
| Type::WinrtArray(ty) => self.type_name(ty), | |||
| Type::WinrtArrayRef(ty) => self.type_name(ty), | |||
| Type::ConstRef(ty) => self.type_name(ty), | |||
| _ => unimplemented!(), | |||
| Type::TypeRef(TypeDefOrRef::TypeRef(ref_)) => self.type_ref_name(*ref_), | |||
There was a problem hiding this comment.
Note that code gen from TypeRef is not a good idea as it implies the TypeDef is not available and thus you have no idea what kind of type you're actually generating code for. Usually this implies you're missing metadata. I should have better diagnostics for this in riddle soon.
There was a problem hiding this comment.
Throwing in my experiences from last night smoke testing riddle. I ran into this too; I was missing base Win32 Metadata winmd in the riddle input folder.
There was a problem hiding this comment.
@kennykerr ah, thanks, so you're basically saying that I fixed this in the wrong way. Which might match the observation of Foundation having vanished.
Improved diagnostics (even something simple as a format-string in an unimplemented!() call) are definitely welcome!
@riverar note that this PR was from before riddle and might be caused by the Win32metadata updates instead.
There was a problem hiding this comment.
I checked the latest Win32/Wdk metadata and riddle can parse it just fine - there are however other complications that will need to be addressed, changes to type and attributes, that go beyond merely parsing.
There was a problem hiding this comment.
Note that code gen from
TypeRefis not a good idea as it implies theTypeDefis not available and thus you have no idea what kind of type you're actually generating code for. Usually this implies you're missing metadata. I should have better diagnostics for this inriddlesoon.
@kennykerr are you still planning on improving the diagnostics here? A similar issue is appearing again because of incomplete metadata: microsoft/win32metadata#1760
There was a problem hiding this comment.
Please open a new issue and include a repro. Thanks.
There was a problem hiding this comment.
Reported at #2742. The repro is generating new metadata with win32metadata main (relevant commit hash is linked in case this is fixed upstream), followed by running tool_windows.
When running
tool_windowson the latest.winmdgenerated with microsoft/win32metadata#1591 to test the results of that metadata change, no implementation exists forTypeRef. This implementation is rather trivial/straightforward based onTypeDef.Going forward, also print the failed value when a
matchstatement ends up in_ => unimplemented!()so that it is quicker to immediately debug what enum variant needs to be implemented.