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

Adding new field kind should be considered breaking change #112033

Closed
AaronRobinsonMSFT opened this issue Jan 31, 2025 · 4 comments · Fixed by #112087
Closed

Adding new field kind should be considered breaking change #112033

AaronRobinsonMSFT opened this issue Jan 31, 2025 · 4 comments · Fixed by #112087
Labels
area-Meta in-pr There is an active PR which will close this issue when it is merged
Milestone

Comments

@AaronRobinsonMSFT
Copy link
Member

AaronRobinsonMSFT commented Jan 31, 2025

In #111584, the alignment of ref struct types was relaxed if the type didn't contained object or byref fields. The previous behavior was that a ref struct was always pointer aligned. This change resulted in the following question:

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?

_Originally posted by @MichalStrehovsky in #111584 (review)

Initially this was accepted but then reverted due to pushback about the layout of types was an implementation details unless specifically documented.

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.

Originally posted by @jkotas in #111584 (comment)

The question now goes to the Libraries team, @stephentoub, and perhaps the .NET API review team, @bartonjs, to help us decide if adding a new kind of field (that is, objref, byref or generic type argument), to an existing value type should be considered a change that should result in a breaking change announcement.

@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Jan 31, 2025
@AaronRobinsonMSFT AaronRobinsonMSFT removed the untriaged New issue has not been triaged by the area owner label Jan 31, 2025
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 10.0.0 milestone Jan 31, 2025
@AaronRobinsonMSFT AaronRobinsonMSFT changed the title Adding new field type should be considered breaking change Adding new field kind should be considered breaking change Jan 31, 2025
@stephentoub
Copy link
Member

stephentoub commented Jan 31, 2025

decide if adding a new kind of field (that is, objref, byref or generic type argument), to an existing value type should be considered a change that should result in a breaking change announcement

Yes, anywhere adding a new field to a struct could impact how Roslyn handles that type should be breaking. There's also the issue of tearing, where adding an additional field to a one primitive/reference field struct could introduce the possibility of tearing, but we generally haven't documented that as breaking (though we also rarely do it).

@jkotas
Copy link
Member

jkotas commented Jan 31, 2025

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.

I am not sure whether this is accurate. Should objrefs and byrefs be treated as the same kind? Adding an objref field to a ref struct with a ref field should be fine. Also, adding a byref field to a ref struct with an objref field should be fine too.

@AaronRobinsonMSFT
Copy link
Member Author

AaronRobinsonMSFT commented Jan 31, 2025

I am not sure whether this is accurate. Should objrefs and byrefs be treated as the same kind? Adding an objref field to a ref struct with a ref field should be fine. Also, adding a byref field to a ref struct with an objref field should be fine too.

From the perspective of the newly added layout support that started this conversation, yes, they have the same impact.

Yes, anywhere adding a new field to a struct could impact how Roslyn handles that type should be breaking.

Roslyn isn't the issue in this specific case, although I do agree that impacting Roslyn should be a breaking change. The question that started the current conversation is only currently detected at run-time and would result in a TypeLoadException.

Motivating example:

ref struct FxType
{
    public int F;
}

[StructLayout(LayoutKind.Explicit)]
ref struct UserType
{
    public int F1;

    [FieldOffset(4)] 
    public FxType F2;
}

In the next release of .NET, the FxType is updated to the following causing the loading of UserType to fail on a 64-bit runtime.

ref struct FxType
{
    public int F;
    public ref int RF; // Could also be an objref instead of byref.
}

@stephentoub
Copy link
Member

The question that started the current conversation is only currently detected at run-time and would result in a TypeLoadException.

Then definitely yes :)

@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Meta in-pr There is an active PR which will close this issue when it is merged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants