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

metadata Reader::attribute_args enhancements #2329

Merged
merged 2 commits into from
Feb 6, 2023

Conversation

ChrisDenton
Copy link
Collaborator

@ChrisDenton ChrisDenton commented Feb 6, 2023

I've been playing with reading attribute metadata and wanted to help fix a couple of issues I encountered:

  • ComVisibleAttribute has a bool argument, so it's useful to be able to read it. I also added a panic if the blob bool value is not either 0 or 1 (as per the spec). Admittedly this may be overkill but I figured it'd help detect if something has gone wrong somewhere.
  • UnmanagedFunctionPointerAttribute requires System.Runtime.InteropServices.CallingConvention and AttributeUsageAttribute requires System.AttributeTargets. Neither of which are part of the default metadata files. I've added the type name to the panic message to make this easier to diagnose but I guess this is a win32metadata issue?

@ChrisDenton ChrisDenton changed the title metadata Reader::attribute args enhancements metadata Reader::attribute_args enhancements Feb 6, 2023
@riverar
Copy link
Collaborator

riverar commented Feb 6, 2023

  • UnmanagedFunctionPointerAttribute requires System.Runtime.InteropServices.CallingConvention and AttributeUsageAttribute requires System.AttributeTargets. Neither of which are part of the default metadata files. I've added the type name to the panic message to make this easier to diagnose but I guess this is a win32metadata issue?

Don't believe metadata is using these attributes. Are you by any chance using ILSpy? (It automatically/annoyingly loads in unrelated .NET reference assemblies.)

@ChrisDenton
Copy link
Collaborator Author

ChrisDenton commented Feb 6, 2023

No, just the metadata reader. A simplified example:

let files = metadata::reader::File::with_default(&[]).unwrap();
let reader = &metadata::reader::Reader::new(&files);
let tree = reader.tree("Windows", &[]).unwrap();

for tree in tree.flatten() {
    for ty in reader.namespace_types(tree.namespace) {
        for attr in reader.type_def_attributes(ty) {
            let name = reader.attribute_name(attr);
            if name == "UnmanagedFunctionPointerAttribute" {
                reader.attribute_args(attr);
            }
        }
    }
}

Panics with the message:

thread 'main' panicked at 'Type not found: System.Runtime.InteropServices.CallingConvention', crates\libs\metadata\src\reader\mod.rs:1597:13

@riverar
Copy link
Collaborator

riverar commented Feb 6, 2023

I think I'm having a moment. Are you saying the reader is pulling in default metadata with references to UnmanagedFunctionPointerAttribute or System.Runtime.InteropServices.CallingConvention? I'm not seeing that here, so I think I'm missing something.

@ChrisDenton
Copy link
Collaborator Author

I mean, they do appear to exist in the generated C# as well. E.g. https://github.com/microsoft/win32metadata/search?q=UnmanagedFunctionPointer

@kennykerr
Copy link
Collaborator

If you need full reflection support, then you'll likely need to include metadata for system types. Generally, I avoid that as that's a dependency I don't want to be responsible to track down. 😊

Copy link
Collaborator

@kennykerr kennykerr left a comment

Choose a reason for hiding this comment

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

Looks good - thanks!

@kennykerr kennykerr merged commit 7210273 into microsoft:master Feb 6, 2023
@ChrisDenton ChrisDenton deleted the reader branch February 6, 2023 17:18
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.

3 participants