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.

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

Harden nested type discovery #3471

Merged
merged 2 commits into from
Feb 3, 2025
Merged

Harden nested type discovery #3471

merged 2 commits into from
Feb 3, 2025

Conversation

kennykerr
Copy link
Collaborator

It turns out that ECMA-335 may include a valid TypeNamespace for a nested TypeDef. I have never encountered this until #3468 but I can't find anything in the spec to indicate that this is invalid. So I've hardened the reader to check whether a TypeDef is nested directly rather than relying on the absence of a TypeNamespace value.

Scanning the NestedClass table is slightly slower, but fortunately it is not much slower as this table is sorted and a binary search can be used. I may revisit this further and attempt to index the types in a single pass instead, but for now this solves the issue encountered by #3468 as I can now also reliably exclude nested types that are not structs. Only nested structs are supported by windows-bindgen.

Fixes: #3468

@riverar
Copy link
Collaborator

riverar commented Feb 3, 2025

I couldn't find it explicitly codified either, but the spec does state that nested type names are scoped by their enclosing type. So I don't think the namespace field serves any meaningful purpose at that point.

Can we instead peek at the ClassType and check for the nested accessibility attribute?

@kennykerr
Copy link
Collaborator Author

Good question.

Here's the nested Version type I encountered in the metadata referred to in the #3468 issue:

image

Unfortunately, it does not have any of the nested bits set. This is what it should look like:

image

So it may still be that this is bad metadata. I'm not too sure, but that doesn't work reliably.

@riverar
Copy link
Collaborator

riverar commented Feb 3, 2025

Interesting. I agree, looks like bad metadata.

Would file a bug on win32metadata; it seems only nested structs are emitted with the appropriate nested attributes.
https://github.com/microsoft/win32metadata/blob/main/sources/ClangSharpSourceToWinmd/ClangSharpSourceWinmdGenerator.cs#L840-L843

(Aside: ILSpy appears to mistakenly refer to these as visibility attributes but is otherwise functioning correctly.)

@kennykerr
Copy link
Collaborator Author

Here's a small optimization: 02eb56b

That at least avoids the binary search in most cases.

@kennykerr kennykerr merged commit d993221 into master Feb 3, 2025
78 checks passed
@kennykerr kennykerr deleted the harden-nested branch February 3, 2025 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bindgen cannot find nested enum Version definition
2 participants