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

[AlsoUsableFor] and [RAIIFree] #389

Closed
kennykerr opened this issue Mar 25, 2021 · 33 comments
Closed

[AlsoUsableFor] and [RAIIFree] #389

kennykerr opened this issue Mar 25, 2021 · 33 comments
Assignees
Labels
blocking A projection cannot use the latest version namespaces Feedback on namespace names or organization rust Critical for Rust adoption usability Touch-up to improve the user experience for a language projection

Comments

@kennykerr
Copy link
Contributor

kennykerr commented Mar 25, 2021

These attributes refer to other types without including their namespace, making it difficult to use in any meaningful way. It should ideally be something like this:

[RAIIFree("Windows.Win32.SystemServices.HeapDestroy")]
...
[AlsoUsableFor("Windows.Win32.SystemServices.HeapHandle")]
...
@kennykerr
Copy link
Contributor Author

For now, I'm just assuming the type/function is in the same namespace as the type that includes the attribute but I'm guessing that may not always be the case.

@mikebattista mikebattista added namespaces Feedback on namespace names or organization usability Touch-up to improve the user experience for a language projection labels Mar 26, 2021
@sotteson1
Copy link
Contributor

sotteson1 commented Mar 26, 2021

I can fix it fairly easily. And HeapDestroy, it would include the class, right? Windows.Win32.SystemServices.Apis.HeapDestroy

@kennykerr
Copy link
Contributor Author

No, the Apis class is an implementation detail of the metadata that would ideally not exist (e.g. ECMA 335 supports free functions). Logically, the HeapDestroy function is in the Windows.Win32.SystemServices namespace. So it would just be Windows.Win32.SystemServices.HeapDestroy.

@AArnott
Copy link
Member

AArnott commented Mar 29, 2021

The attributes are only in the metadata, so IMO they should refer to other members by their metadata names. The Apis class is not an implementation detail of the metadata, since all direct users of the metadata have to know about it and write projections based on it.

@kennykerr
Copy link
Contributor Author

What I mean is that the very existence of the Apis class regrettable. It is a concession to the implementation choices of using C# to generate metadata. Had it not been for this implementation detail, the Apis class would not need to exist. Logically, at least for languages that support namespace-level functions (like C++ and Rust and perhaps others), the Apis class is just this odd placeholder type where we find the "free functions" for that namespace. There is after all no "Apis" class in the original Win32 API headers that this project parses.

The C++ and Rust parsers perform type lookup via a two-string key consisting of namespace and name. In the example presented here, that would be "Windows.Win32.SystemServices" and "HeapDestroy". The Apis is not considered a namespace nor is it part of the name. I guess in C# you would consider these static methods of the Apis class, but that just seems like a very C# specific view of the world.

@AArnott
Copy link
Member

AArnott commented Mar 29, 2021

Ok, fair enough.

@sotteson1
Copy link
Contributor

sotteson1 commented Mar 30, 2021

It is a concession to the implementation choices of using C# to generate metadata. Had it not been for this implementation detail, the Apis class would not need to exist.

I don't see any other way to represent methods in metadata without putting them on a type, or maybe you know how to do it. I would be happy to get rid of the Apis class if I can. I'm using a metadata writer to output to metadata, even though I start by walking a C# syntax tree generated by ClangSharp. I have flexibility to move away from C# when we need to (which I'm already doing a lot).

@kennykerr
Copy link
Contributor Author

I think you're right. Software is full of compromise and this just seemed like one of those concessions we had to make to get the job done given the constraints we were given, metadata formats and such. I also didn’t fully grasp that you had moved away from a purely C# driven approach to metadata generation, so I was still thinking that’s all we had to work with.

I have to say that I’m thrilled with what you've come up with for win32 metadata. For years I’ve dreamed of having metadata for Win32 APIs and you’ve single-handedly made that a reality. The only reason I made that comment about methods and classes is that IL technically supports free functions. I happen to know that because I wrote about it many years ago, but its been so long I have no idea whether that would work for us in practice nor does it really matter. The Apis class is perfectly fine – it doesn’t bother me in the least. My only point really was that the Apis class is just the construct we use for grouping functions within namespaces. It’s not part of the fully-qualified name of the function, at least in the way that I think about it – every construct has a namespace and a name – and in that worldview there’s no room for the Apis class name. But I can see how that is somewhat subjective. 😊

I'm sorry if I gave the impression of being critical of the implementation. Far from it - I'm really quite thrilled.

@marler8997
Copy link
Contributor

marler8997 commented Apr 3, 2021

This is probably a silly idea, but you could get rid of Apis class by defining the last namespace as a static class rather than a namespace. So instead of:

namespace Windows.Win32.ActiveDirectory
{
    public static class Apis
    {
        public const uint ACTRL_DS_CONTROL_ACCESS = 256u;
        ...
    }
    public struct AccessControlEntry { }
    ...
}

You could do this:

namespace Windows.Win32
{
    public static class ActiveDirectory
    {
        public const uint ACTRL_DS_CONTROL_ACCESS = 256u;
        ...
        public struct AccessControlEntry { }
        ...
    }
}

This would remove the need for the Apis class but it would also mean that the sub-types within the top-level class would typically be printed as ActiveDirectory+AccessControlEntry and it's just weird. Seems like a a choice between the lesser of 2 evils.

@weltkante
Copy link

weltkante commented Apr 6, 2021

IL technically supports free functions

IMHO no, it doesn't, but it really depends on how you see it. The convention is to put "free" functions on the hidden module class. I don't know how well established this pattern is with tooling (maybe its even standardized somewhere), but at the end its just convention.

Considering that the hidden module class is global you would lose the ability to classify methods by namespace/component if you pick up this convention.

@kennykerr
Copy link
Contributor Author

kennykerr commented Apr 14, 2021

I finally tried implementing RAIIFree in Rust (to generate the equivalent of a destructor) but because HANDLE is in the Windows.Win32.SystemServices namespace but CloseHandle is in a different namespace there's no way for me to reliably find it. If HANDLE were defined as follows, it would be a piece of cake:

[RAIIFree("Windows.Win32.WindowsProgramming.CloseHandle")]
[NativeTypedef]
public struct HANDLE
{
	public IntPtr Value;
}

But perhaps HANDLE and CloseHandle belong in the same namespace? How are others handling this today?

@kennykerr
Copy link
Contributor Author

kennykerr commented Apr 15, 2021

Same for Windows.Win32.MenusAndResources.HMENU whose free function is Windows.Win32.WindowsAndMessaging.DestroyMenu.

Same for HICON/HMENU/HCURSOR and possibly others.

@AArnott AArnott added the blocking A projection cannot use the latest version label May 14, 2021
@AArnott
Copy link
Member

AArnott commented May 14, 2021

There are two GetDeviceID functions, defined in different namespaces, so in my current work in CsWin32 I now have to stop ignoring namespaces in the metadata in order to avoid name collisions. In doing so, I need this work done too, because looking up "CloseHandle" won't match anything in my lookup dictionary which must now be keyed off namespace-qualified names.

When we have namespaces prepended to these attributes, should that prepended string include Apis. as well? I could go either way. I'm leaning toward "yes" because then all my dictionaries can just work based on metadata names and I don't have to worry about whether the last segment before the method name is "Apis" vs an interface type name, that actually is significant. But the main point is I need the namespace.

@AArnott
Copy link
Member

AArnott commented May 14, 2021

Or another option is add Namespace as a separate parameter to these attributes. That would actually be the most efficient thing in terms of memory consumption (by CsWin32 at least) because then I don't have to create a bunch of strings repeatedly. And we can dodge the "should .Apis be included in the string?" question too.

@marler8997
Copy link
Contributor

There are two GetDeviceID functions, defined in different namespaces,

I didn't think this was possible since the C/C++ headers don't use namespaces to resolve conflicts like this...?

@marler8997
Copy link
Contributor

I see there's a GetDeviceID function from Media.Audio.DirectMusic and one from System.TpmBaseServices.

It looks like the audio one is documented here: https://docs.microsoft.com/en-us/previous-versions/windows/desktop/ee417724(v=vs.85) So it looks like it comes from "dsound.h".

I'm not seeing where the tpm version of GetDeviceID comes from?

@AArnott
Copy link
Member

AArnott commented May 14, 2021

@marler8997 From what I've heard, conflicting method names do exist in Win32, but typically in different enough areas that you can avoid the ambiguity by including only one of the header files.

@AArnott
Copy link
Member

AArnott commented Jun 14, 2021

How about using typerefs in these two attributes instead of fully-qualified strings?

[RAIIFree(typeof(Windows.Win32.SystemServices.Apis), "HeapDestroy")]
[AlsoUsableFor(typeof(Windows.Win32.SystemServices.HeapHandle))]

That would leverage the metadata for what ECMA-335 can efficiently represent, so the metadata would likely get slightly smaller. It also gives the projection code a convenient way to get each of the three string elements (namespace, class name, and method name) that they may need without allocating strings unnecessarily.

@KalleOlaviNiemitalo
Copy link

According to ECMA-335 6th ed. II.23.3 Custom attributes, an argument for a parameter of type System.Type is encoded as a string anyway. How would that save anything? Or were you going to use a constructor with a different parameter type?.

@marler8997
Copy link
Contributor

@AArnott that format would be fine with me.

@AArnott
Copy link
Member

AArnott commented Jun 22, 2021

@KalleOlaviNiemitalo It's not that we avoid strings in the metadata, since yes every type reference is encoded as a string, like you say.
It's that when we read the metadata in our projections, we needn't decode a long string only to parse and dismantle it into the smaller strings that we require. Instead, we can observe the TypeReferenceHandle and the method name string and proceed with processing.

@jnm2
Copy link

jnm2 commented Jun 22, 2021

@AArnott My recollection is that I didn't get a TypeReferenceHandle for attribute arguments that were type constants, but I had to parse the string myself. I filed an issue with S.R.Metadata to provide a decoder because it was tedious. (Perhaps I'm confused and it was only enum constants cast to object though.)

@kennykerr
Copy link
Contributor Author

Yes, internally it is encoded as a string in the attribute blob. The C# parser probably hides this detail.

mikebattista added a commit that referenced this issue Nov 21, 2022
Generated C# looks correct but ILSpy only shows typeof(Type).
@mikebattista
Copy link
Contributor

I attempted to update RAIIFree to use the proposed model at f0deee3. The generated C# looks as expected, but I'm not sure it's translating to the winmd properly. ILSpy just shows typeof(Type) everywhere.

@AArnott could you take a look? Given the generated C# code looks correct, I'm not sure if this is an ECMA-335 issue, winmd issue, ILSpy issue, or something else.

@kennykerr
Copy link
Contributor Author

Note that storing the namespace as a type doesn't actually make things better. As I mentioned this is just encoded as a string anyway, but the real problem is that the function name is still stored as a string rather than a MethodDef record. Without that, a tool still needs to walk the ECMA-335 MethodDef table looking for the function since this table is not sorted. Now if the attribute were only used for functions with a single parameter, then this wouldn't be necessary to weed out functions that are un-callable - I assume that's still an issue.

@riverar
Copy link
Collaborator

riverar commented Dec 10, 2022

I'm still tinkering with the code for this, but perhaps we can augment the RAIIFreeAttribute with an optional integer containing the Row ID (RID) of the freeing method in the MethodDef table. This would be generated at winmd generation time. This will eliminate the MethodDef table scan, simplifying code generation.

Example from my experimental winmd:

[RAIIFree("Windows.Win32.Graphics.OpenGL.Apis+wglDeleteContext", 7821)]
[InvalidHandleValue(-1L)]
[InvalidHandleValue(0L)]
[NativeTypedef]
public struct HGLRC
{
  public IntPtr Value;
}

Screenshot from ILSpy showing relevant MethodDef table record:
image

Q: Do we still need the method name in the final output?

@kennykerr
Copy link
Contributor Author

If the MethodDef row ID is available, the name isn't needed since that can be looked up directly. Then it can simply be:

[RAIIFree("Windows.Win32.Graphics.OpenGL", 7821)]

@KalleOlaviNiemitalo
Copy link

a tool still needs to walk the ECMA-335 MethodDef table looking for the function since this table is not sorted

The methods of a class have to be consecutive, and the methods of a COM interface have to be in the vtable order; but would it be feasible to sort the methods of Apis classes by name, and perhaps add an attribute to indicate this was done? Projections could then do a binary search.

Is it possible that a metadata assembly of an extension SDK will need to use RAIIFreeAttribute on a type of its own (or a parameter or a return value, if supported) but reference a freeing method in the Windows SDK metadata? In that case, the MethodDef row ID would not be reliable, as the version might not match exactly.

@kennykerr
Copy link
Contributor Author

Yes, a simpler approach may be to guarantee that the functions in the Apis class are sorted by name to allow a binary search. Then the following simpler attribute would be sufficient:

[RAIIFree("Windows.Win32.Graphics.OpenGL.wglDeleteContext")]

@riverar
Copy link
Collaborator

riverar commented Dec 12, 2022

Don't think that'll be possible as metadata relies on .NET for the build up and emission of the winmd, and I don't think there are any ordering guarantees or knobs to tweak there. Will take a deeper look though!

@mikebattista mikebattista assigned riverar and unassigned AArnott and mikebattista Mar 8, 2023
@mikebattista
Copy link
Contributor

Assigning to you @riverar since it sounds like you've gotten pretty far with this already.

@mikebattista
Copy link
Contributor

mikebattista commented May 10, 2023

At this point, all AlsoUsableFor and RAIIFree definitions are in the same namespace as the struct they're on except for:

  • FreeLibrary in Windows.Win32.System.LibraryLoader while HMODULE/HINSTANCE in Windows.Win32.Foundation
  • Local/GlobalFree in Windows.Win32.System.Memory while HLOCAL/HGLOBAL in Windows.Win32.Foundation

Should we just move these Free functions to Windows.Win32.Foundation and call it a day?

Looking at the metadata now compared to 2 years ago when this was filed, I think it's fair to enforce that all AlsoUsableFor and RAIIFree relationships must exist in the same namespace. A violation of that would be a bug in the metadata.

@kennykerr
Copy link
Contributor Author

That's fine with me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocking A projection cannot use the latest version namespaces Feedback on namespace names or organization rust Critical for Rust adoption usability Touch-up to improve the user experience for a language projection
Projects
None yet
Development

No branches or pull requests

9 participants