Fix explicit offset of ByRefLike fields.

[AaronRobinsonMSFT]

A ByRefLike type was previously forced to be pointer aligned so ref fields were implicitly aligned. Using Explicit layout one can create valid aligned ByRefLike types with no object pointers. This change propagates the base offset of the ByRefLike type and includes that for determining offsets of fields in the ByRefLike type.

Fixes #111260

[jkotas]

LGTM. Thank you!

[MichalStrehovsky]

Do we need to add this to breaking-change-rules.md? Would we consider it a breaking change to add a ref field to a ref struct that didn't previously have a ref field?

(We do consider switching between struct and ref struct a breaking change in that doc.)

[AaronRobinsonMSFT]

Do we need to add this to breaking-change-rules.md?

+1. I wasn't aware of this document, but I've updated it accordingly.

[jkotas]

I am not following what's the rationale for this. How is this different from adding/removing any fields? We have no guarantees for internal type layout compatibility, unless the guarantee is explicitly documented.

"Changing a struct type to a ref struct type and vice versa" is very different - it is very obvious compile time breaking change.

[AaronRobinsonMSFT]

I think the point of it is that now adding a ref field could break someone if they were using the type in an Explicit offset scenario. For example, the BCL has some ref struct that just has an int field. A user consumes that type in a user defined type as an Explicit field and the containing library is published to NuGet. In the next major version a ref int field is added. When that NuGet package is consumed on a new BCL, the library fails to load because the ref int field forces a new alignment restriction.

By definition this would now break. We can argue if it matters, but it would cause a failure at type load time.

[jkotas]

I think the point of it is that now adding a ref field could break someone if they were using the type in an Explicit offset scenario.

It is the case for any field changes in public BCL value types. You can get the same break by adding/removing int or object fields too. There is nothing special about ref fields here.

Field layout of BCL value types is not a public contract that people can depend on, unless it is explicitly documented for specific types.

[AaronRobinsonMSFT]

"Changing a struct type to a ref struct type and vice versa" is very different - it is very obvious compile time breaking change.

It is the case for any field changes in public BCL value types. You can get the same break by adding/removing int or object fields too. There is nothing special about ref fields here.

Between the two statements above, it looks like the distinction in this case is compile time vs run time failure. Is that the conversation we are having? In the sense that compile time failure is not acceptable, but run time failure is?

[jkotas]

There happens to be a compile time vs. runtime time difference between these two case, but that's just accidental. Runtime breaking changes are not acceptable either in general.

I think that the discussion that we are having here is about distinction between what we define as a breaking change vs. change that can break apps that depend on undocumented behaviors.

[MichalStrehovsky]

It is the case for any field changes in public BCL value types. You can get the same break by adding/removing int or object fields too. There is nothing special about ref fields here.

I thought we already consider such things breaking as well - for example adding an object typed field to a struct makes the struct no longer meet unmanaged constraint. I recall Roslyn team even did work to put a dummy object field into such struct for reference assemblies because these non-contractual private implementation details actually matter.

[jkotas]

If the libraries folks agree, I think it would be fine to explicitly match the current Roslyn behavior and say that adding first field of a new kind to an existing value type is disallowed change. Kind can be any of objref, byref or generic type argument, and the rule applies recursively.

Introducing new significant state in existing value types is close to impossible to do in compatible way for other reasons (e.g. it is why we have both DateTime and DateTimeOffset), so these situations do not come up in practice. I guess it is the reason why nobody bothered to mention it in the breaking changes doc up until now.

[AaronRobinsonMSFT]

I will create a discussion issue for this topic and loop in libraries folks. Updating the markdown is a cheaper PR than continually running the build/test legs.

[jkotas]

This falls into the same category as dangers of private reflection that are not particularly well documented either.

I have created #112114

[jkotas]

Should we mention it in https://learn.microsoft.com/en-us/dotnet/standard/native-interop/best-practices

Opened dotnet/docs#44672

[AaronRobinsonMSFT]

native AOT legs failing due to dotnet/dnceng#4892

[AaronRobinsonMSFT]

/ba-g Unrelated failures to PR. All failures are in Mono or related to script failures captured in dotnet/dnceng#4892.