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.

Sign up for GitHub

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

Implement type_name() for TypeRef #2539

Closed
wants to merge 1 commit into from
Closed

Implement type_name() for TypeRef #2539

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
34 changes: 29 additions & 5 deletions crates/libs/bindgen/src/gen.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,29 @@ impl<'a> Gen<'a> {
}
}

//
// TypeRef
//

pub fn type_ref_name(&self, ref_: TypeRef) -> TokenStream {
self.type_ref_name_imp(ref_, "")
}
pub fn type_ref_vtbl_name(&self, ref_: TypeRef) -> TokenStream {
self.type_ref_name_imp(ref_, "_Vtbl")
}
pub fn type_ref_name_imp(&self, ref_: TypeRef, suffix: &str) -> TokenStream {
let type_name = self.reader.type_ref_type_name(ref_);

assert!(!type_name.namespace.is_empty());

let mut namespace = self.namespace(type_name.namespace);
let mut name = to_ident(type_name.name);
name.push_str(suffix);

namespace.combine(&name);
namespace
}

//
// Type
//
Expand Down Expand Up @@ -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_),
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@kennykerr are you still planning on improving the diagnostics here? A similar issue is appearing again because of incomplete metadata: microsoft/win32metadata#1760

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please open a new issue and include a repro. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

x => unimplemented!("{:?}", x),
}
}
pub fn type_vtbl_name(&self, ty: &Type) -> TokenStream {
match ty {
Type::TypeDef((def, generics)) => self.type_def_vtbl_name(*def, generics),
_ => unimplemented!(),
x => unimplemented!("{:?}", x),
}
}
pub fn type_abi_name(&self, ty: &Type) -> TokenStream {
Expand Down Expand Up @@ -524,7 +548,7 @@ impl<'a> Gen<'a> {
tokens.push('\"');
tokens.into()
}
_ => unimplemented!(),
x => unimplemented!("{:?}", x),
}
}
pub fn typed_value(&self, value: &Value) -> TokenStream {
Expand All @@ -544,7 +568,7 @@ impl<'a> Gen<'a> {
Value::String(_) => {
quote! { &str = #literal }
}
_ => unimplemented!(),
x => unimplemented!("{:?}", x),
}
}

Expand Down Expand Up @@ -638,7 +662,7 @@ impl<'a> Gen<'a> {
AsyncKind::OperationWithProgress => {
quote! { AsyncOperationWithProgressCompletedHandler }
}
_ => unimplemented!(),
x => unimplemented!("{:?}", x),
};

let namespace = self.namespace("Windows.Foundation");
Expand Down
2 changes: 1 addition & 1 deletion crates/libs/metadata/src/reader/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ impl SignatureParamKind {
}
}

#[derive(PartialEq, Eq)]
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
pub enum AsyncKind {
None,
Action,
Expand Down