BITMAPINFO metadata is wrong (and may be hard to fix)

[AArnott]

The BITMAPINFO struct represents a header and then an array that is arbitrarily long. Its length is determined by the header. It also may not be RGBQUAD at all.

[rumbu13]

Metadata describes exactly the same struct defined in gdi.h:

typedef struct tagBITMAPINFO {
  BITMAPINFOHEADER bmiHeader;
  RGBQUAD          bmiColors[1];
} BITMAPINFO, *LPBITMAPINFO, *PBITMAPINFO;

Metadata:

public struct BITMAPINFO
  {
    public BITMAPINFOHEADER bmiHeader;
    public RGBQUAD[1] bmiColors;
  }

[AArnott]

I don't deny that it matches the header. But the header only tells a part of the story. The docs tell the rest, and the rest is that the header is misleading. For example the header claims the second field is an array with length of 1, but that is perhaps never really the case.
In C, programmers can workaround this. As the metadata is used in many languages, it may need to more correctly describe the struct's size and shape or it may fail on other languages.

[rumbu13]

There are around 700 structures across metadata having variable length arrays at the end, e.g. all the IMAGE_ structs from Windows.Win32.Services describing the PE COFF format.

The problem is that the number of elements [1] is put in the metadata. If the number of elements would be left as [0], this can be interpreted by other languages as an "open" array.

[tannergooding]

Likewise, variable-length arrays are an example of a fundamentally unsafe type which has additional constraints not representable by the type system. This holds true even in C/C++.

The type system does not contain enough metadata to know or even represent that bmiColors is not 1 element in length (nor 0 if you use that, but that also has its own additional problems) and so if it is ever handled by value it will only copy sizeof(BITMAPINFOHEADER) + sizeof(RGBQUAD) (plus any padding, if it exists) bytes. Because of this, these types are never valid to be handled "by value", they must always be handled by pointer or reference.

[AArnott]

We came to a good place over at microsoft/CsWin32#117 where we think we can do the right thing if the metadata can indicate that the array has variable length. Bonus points if it can point out the field that indicates what the length is.

If the header files don't disclose that it has variable length, perhaps we can infer it by the use of [1] in the headers. Why would it define an array of fixed length 1, unless like BITMAPINFO it is just a notation that may be commonly used to mean an array of some length?

[sotteson1]

That seems like a good assumption. I could add an attribute as well if that would help, but @rumbu13 says there are around 700 (!) that do this.

[AArnott]

Are you suggesting that projections figure this out based on the pattern? If projections over time figure out these patterns and hard-code them, exceptions to the rule or refining the rule will be very slow to propagate through all the projections. I would favor finding a way to encode the rule in the metadata so that projections can just take the attribute as truth, and we can fiddle with the attributes as needed over time.

Is there some way your metadata build can add the attribute to the 700 structs automatically so it's less tedious work for you?

[sotteson1]

Yeah, that makes sense for the metadata writer to auto-add the attribute.

[tannergooding]

Yeah, that makes sense for the metadata writer to auto-add the attribute.

ClangSharp already "supports" this and it uses a length of 1 or 0 to assume "variable length" inline arrays for the helpers it generates (which win32metadata disables).
It just doesn't add any attribute or other metadata to the output (aside from the NativeTypeNameAttribute).

[sotteson1]

This was changed a while ago to set the length of the array to be 0. I went with what @tannergooding suggested, that we set the array length to 0 which implies it's a variable-length array.