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

[API Proposal]: Add ObsoleteAttribute.BinaryCompatOnly #65965

Open
jaredpar opened this issue Feb 28, 2022 · 19 comments
Open

[API Proposal]: Add ObsoleteAttribute.BinaryCompatOnly #65965

jaredpar opened this issue Feb 28, 2022 · 19 comments
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Runtime
Milestone

Comments

@jaredpar
Copy link
Member

jaredpar commented Feb 28, 2022

Background and motivation

The .NET Runtime goes to great lengths to have binary compatibility between releases. That dramatically lowers the cost of version to version updates and provides a smooth update experience for customers. It also means that API decisions are forever, once shipped we are stuck with an API.

This is true even as the language and ecosystem evolves and better patterns emerge that we'd like to promote. For example consider the CallerArgumentExpression feature in C#. Given this feature it's possible to have a better default experience for Debug.Assert by defining the method as such:

public static class Debug
{
    public static void Assert(bool condition, [CallerArgumentExpression("condition")] string message = null);
}

This though is a bit of a non-starter. Due to the rules of C# this method will never be bound to because it will always prefer Debug.Assert(string). Changing the overload rules that make this determination is a bit of a non-starter because it would result in sweeping changes across the ecosystem. Where as the desired impact is much smaller: stop using Debug.Assert(string) for languages that support CallerArgumentExpression.

This desire to remove one method from source and replace with a highly compatible alternative has come up several times over the last few releases:

  • Several interactions around CallerArgumentExpressionAttribute
  • Desire for the CallerIdentityAttribute
  • The C# 10 improved lambda overload resolution support revealed several regrettable overloads in ASP.NET that we wished could be replaced by more modern versions.

The proposal is to provide a mechanism by which APIs can be marked as "for binary compatibility only". The member will not be considered for source compilation purposes by languages.

This can be done by extending our existing ObsoleteAttribute to have a new property: BinaryCompatOnly.

Note that this is substantially different than ObsoleteAttribute.IsError. That in no way changes what members a language will bind to. Instead it is only considered after a language has gone through member look up, overload resolution, etc ... At the moment the final decision on a member is made only then does the language look at ObsoleteAttribute and determine a diagnostic should be emitted.

This proposal removes the API at a much lower level. It asks languages to not even consider the member during compilation. For all intents and purposes the member should not be imported from metadata except for what is necessary to ensure a type definition is sound (for example that it implements all interface members).

API Proposal

public sealed class ObsoleteAttribute : Attribute
{
    public ObsoleteAttribute(string? message, bool error, bool binaryCompatOnly)
    {
        Message = message;
        IsError = error;
        BinaryCompatOnly = binaryCompatOnly;
    }

    public bool BinaryCompatOnly { get; set; }
}

API Usage

public static class Debug
{

    [Obsolete(BinaryCompatOnly = true)]
    public static void Assert(bool condition);

    public static void Assert(bool condition, [CallerArgumentExpression("condition")] string message = null);
}

Alternative Designs

Leverage reference assemblies

An alternative design is to change the reference assembly generation tool such that these APIs are just not included. That would have roughly the same impact as this change: on upgrade only the new APIs would be available for compilation hence they would win out.

The downside of this approach is that it's going to be less friendly to languages other than C#, F# and VB. As detailed in CallerIdentityAttribute proposal it's likely that the alternative source methods provided are going to rely on new features. Those features are less likely to be present in other languages and hence is more likely to result in compilation errors on upgrade.

This is not the case with the proposal to add ObsoleteAttribute.BinaryCompateOnly. Languages have to make a change to recognize that attribute. Languages can then weigh the cost of supporting the features most relied on by BinaryCompateOnly and take them as a single feature in a release.

Alternate attribute

It's reasonable to see this as expanding ObsoleteAttribute beyond it's intended scope and instead a new attribute should be considered like BinaryCompateOnlyAttribute

Risks

Method Group Support

The motivating cases around this tend to be about overload resolution. Essentially an existing API is always preferred due to C#, VB, F# overload resolution rules and that is preventing us from adding a new, better API. The [Obsolete(BinaryCompatOnly = true)] in combination with a new overload, which is largely source compatible, will minimize upgrade experience pain to a large degree for method invocations.

// Compiles today, compiles tomorrow just to a diff overload
Debug.Assert(SomeCondition);

This will not fix method group conversions though as they are focused on the signature of the method

// Compiles today, fails tomorrow
// Note: presence of [Conditional] makes this fail today but the general method group problem exists
Action<bool> action = Debug.Assert;

This is likely a minority case compared to method invocation though and may be an acceptable trade off for forward progress in the platform.

@jaredpar jaredpar added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Feb 28, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Feb 28, 2022
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@jaredpar jaredpar changed the title [API Proposal]: Add ObseleteAttribute.BinaryCompatOnly [API Proposal]: Add ObsoleteAttribute.BinaryCompatOnly Feb 28, 2022
@ghost
Copy link

ghost commented Feb 28, 2022

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

The .NET Runtime goes to great lengths to have binary compatibility between releases. That dramatically lowers the cost of version to version updates and provides a smooth update experience for customers. It also means that API decisions are forever, once shipped we are stuck with an API.

This is true even as the language and ecosystem evolves and better patterns emerge that we'd like to promote. For example consider the CallerArgumentExpression feature in C#. Given this feature it's possible to have a better default experience for Debug.Assert by defining the method as such:

public static class Debug
{
    public static void Assert(bool condition, [CallerArgumentExpression("condition")] string message = null);
}

This though is a bit of a non-starter. Due to the rules of C# this method will never be bound to because it will always prefer Debug.Assert(string). Changing the overload rules that make this determination is a bit of a non-starter because it would result in sweeping changes across the ecosystem. Where as the desired impact is much smaller: stop using Debug.Assert(string) for languages that support CallerArgumentExpression.

This desire to remove one method from source and replace with a highly compatible alternative has come up several times over the last few releases:

  • Several interactions around CallerArgumentExpressionAttribute
  • Desire for the CallerIdentityAttribute
  • The C# 10 improved lambda overload resolution support revealed several regrettable overloads in ASP.NET that we wished could be replaced by more modern versions.

The proposal is to provide a mechanism by which APIs can be marked as "for binary compatibility only". The member will not be considered for source compilation purposes by languages.

This can be done by extending our existing ObsoleteAttribute to have a new property: BinaryCompatOnly.

Note that this is substantially different than ObsoleteAttribute.IsError. That in no way changes what members a language will bind to. Instead it is only considered after a language has gone through member look up, overload resolution, etc ... At the moment the final decision on a member is made only then does the language look at ObsoleteAttribute and determine a diagnostic should be emitted.

This proposal removes the API at a much lower level. It asks languages to not even consider the member during compilation. For all intents and purposes the member should not be imported from metadata except for what is necessary to ensure a type definition is sound (for example that it implements all interface members).

API Proposal

public sealed class ObsoleteAttribute : Attribute
{
    public ObsoleteAttribute(string? message, bool error, bool binaryCompatOnly)
    {
        Message = message;
        IsError = error;
        BinaryCompatOnly = binaryCompatOnly;
    }

    public bool BinaryCompatOnly { get; set; }
}

API Usage

public static class Debug
{

    [Obsolete(BinaryCompatOnly = true)]
    public static void Assert(bool condition);

    public static void Assert(bool condition, [CallerArgumentExpression("condition")] string message = null);
}

Alternative Designs

Leverage reference assemblies

An alternative design is to change the reference assembly generation tool such that these APIs are just not included. That would have roughly the same impact as this change: on upgrade only the new APIs would be available for compilation hence they would win out.

The downside of this approach is that it's going to be less friendly to languages other than C#, F# and VB. As detailed in CallerIdentityAttribute proposal it's likely that the alternative source methods provided are going to rely on new features. Those features are less likely to be present in other languages and hence is more likely to result in compilation errors on upgrade.

This is not the case with the proposal to add ObsoleteAttribute.BinaryCompateOnly. Languages have to make a change to recognize that attribute. Languages can then weigh the cost of supporting the features most relied on by BinaryCompateOnly and take them as a single feature in a release.

Alternate attribute

It's reasonable to see this as expanding ObsoleteAttribute beyond it's intended scope and instead a new attribute should be considered like BinaryCompateOnlyAttribute

Risks

Method Group Support

The motivating cases around this tend to be about overload resolution. Essentially an existing API is always preferred due to C#, VB, F# overload resolution rules and that is preventing us from adding a new, better API. The [Obsolete(BinaryCompatOnly = true)] in combination with a new overload, which is largely source compatible, will minimize upgrade experience pain to a large degree for method invocations.

// Compiles today, compiles tomorrow just to a diff overload
Debug.Assert(SomeCondition);

This will not fix method group conversions though as they are focused on the signature of the method

// Compiles today, fails tomorrow
Action<string> action = Debug.Assert;

This is likely a minority case compared to method invocation though and may be an acceptable trade off for forward progress in the platform.

Author: jaredpar
Assignees: -
Labels:

api-suggestion, area-System.Runtime, untriaged

Milestone: -

@KalleOlaviNiemitalo
Copy link

// Compiles today, fails tomorrow
Action<string> action = Debug.Assert;

I think you mean Action<bool>, but that fails with error CS1618: Cannot create delegate with 'Debug.Assert(bool)' because it or a method it overrides has a Conditional attribute

Because BinaryCompatOnly would affect overload resolution, it would need a C# specification change and should not affect C# language versions that have already been standardized.

@jaredpar
Copy link
Member Author

jaredpar commented Mar 1, 2022

I think you mean Action, but that fails with error CS1618: Cannot create delegate with 'Debug.Assert(bool)' because it or a method it overrides has a Conditional attribute

Correct that should've been Action<bool> (updated). Forgot that Conditional impacts the method group conversion here. So that specific case is broken today. The general scenario though of method groups not having a viable source alternate remains true though.

Because BinaryCompatOnly would affect overload resolution, it would need a C# specification change and should not affect C# language versions that have already been standardized.

To be clear it impacts more than overload resolution. For all intents and purposes the symbol does not exist. The best way to visualize this is that the language ignores the member entirely when importing from metadata (details are likely to be different but practically this is how it will work). That means it impacts any feature which could resolve to that symbol.

@KalleOlaviNiemitalo
Copy link

If metadata were compiled from

public class A {
    public virtual void M() {}
}
public class B : A {
    [Obsolete(BinaryCompatOnly = true)]
    public new void M() {} // not virtual
}

and that were imported to

public class C : B {
    public override void M() {}
}

then, would C.M skip B.M and override A.M, adding a row in the MethodImpl table?

@tannergooding tannergooding added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation untriaged New issue has not been triaged by the area owner labels Mar 1, 2022
@tannergooding
Copy link
Member

Marked as api-ready-for-review. We will end up discussing whether it should modify the existing attribute or be a new attribute in API review.

@jaredpar, is there any pressing need for this from the language for .NET 7/C# 11 or is it just part of the "general backlog"?

@jaredpar
Copy link
Member Author

jaredpar commented Mar 1, 2022

@tannergooding

is there any pressing need for this from the language for .NET 7/C# 11 or is it just part of the "general backlog"?

Not on our side. This proposal is mostly a reaction to roadblocks I've seen runtime / aspnetcore hit recently trying to evolve to modern C# features. Hence there is no direct priority here and could be done in .NET 7 or 8.

The one possible .NET 7 connection is CallerIdenityAttribute. I do not know if that is critical for .NET 7 or fine for a later version. It is hard to see how that could succeed the way we want without a proposal of this form. @jkotas, @agocke can likely speak best to the timeframe of that feature.

@jaredpar
Copy link
Member Author

jaredpar commented Mar 1, 2022

@KalleOlaviNiemitalo

then, would C.M skip B.M and override A.M, adding a row in the MethodImpl table?

This is why I was saying "visualize" vs. "this is how we are actually going to do it 😄

There are a number of details we'd need to work out on the language side. My expectation is that we'd import the member and keep it around for checks like duplicate members, duplicate types, exposing BinaryCompatOnly types in source methods, conflicting signatures, etc ... The member just wouldn't be a candidate for name lookup scenarios.

@jkotas
Copy link
Member

jkotas commented Mar 1, 2022

The one possible .NET 7 connection is CallerIdentityAttribute

CallerIdentityAttribute contributes to our AOT and trimming friendliness .NET 7 theme. I would love to see it happen if possible.

@agocke
Copy link
Member

agocke commented Mar 1, 2022

As an aside, I think the mechanics in this proposal are very similar to the spec language in ECMA around modreqs. If you do not understand a modreq as a compiler, you're supposed to "drop" the member on import. It sounds like BinaryCompatOnly would do something similar.

@tannergooding tannergooding added this to the 7.0.0 milestone Mar 1, 2022
@jaredpar
Copy link
Member Author

jaredpar commented Mar 1, 2022

@agocke

Yes it's conceptually similar to a modreq. The diff being:

  • Adding a modreq here would change the signature which breaks binary compat which defeats the purpose of the proposal
  • Can't use modreq on a type while my expectation is this would be desirable to work on types as well

@svick
Copy link
Contributor

svick commented Mar 2, 2022

Small thing: should the name be the full BinaryCompatibilityOnly, instead of using the abbreviated "compat"? Especially since googling "compat" does not give any useful results, so it might confuse people who don't know the jargon. (On the other hand, Google does understand the whole phrase "binary compat".)

@jaredpar
Copy link
Member Author

jaredpar commented Mar 3, 2022

I'm fine with whatever name API review thinks is appropriate here.

@stephentoub
Copy link
Member

@jaredpar, the Debug.Assert example is a good one. You mention this would help in a few other cases that have come up with the core libraries... got additional examples of things we'd want to update at the same time?

@jaredpar
Copy link
Member Author

@stephentoub

Examples that would be relevant if we went forward with CallerIdentityAttribute

Generally applicable scenarios:

  • Any method today that desires any of the caller info features (file path, line number, expression). There is no way to extend this to get the caller info behavior as that requires a new overload with an optional and compiler will always pick the existing member as it has less optionals (tie breaker in optional scenarios).
  • Anytime you've added an overload to a method with existing optional parameters. General pattern here is remove all optional params from existing overload, copy them to new overload, add new parameter to end with an optional value. The old overload at that point is for binary compat only. Candidate for [BinaryComptabilityOnly]. I'm unsure if runtime allows this type of overload change.

I don't know of any candidates in core libraries that meet these requirements today off hand though.

@terrajobst terrajobst added api-needs-work API needs work before it is approved, it is NOT ready for implementation and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Apr 14, 2022
@terrajobst
Copy link
Member

terrajobst commented Apr 14, 2022

We should probably sit down and list all the places where this would be interesting to use and see what the alternatives would be. For instance, for CallerIdentity one options is to just remove the old members for the ref assembly and ensure that F# and VB and support the new attribute as well.

But we love the idea :-)

@jeffhandley jeffhandley modified the milestones: 7.0.0, 8.0.0 Jul 10, 2022
@ericstj
Copy link
Member

ericstj commented Aug 18, 2022

An alternative design is to change the reference assembly generation tool such that these APIs are just not included. That would have roughly the same impact as this change: on upgrade only the new APIs would be available for compilation hence they would win out.

FWIW this isn't a change, we already have the ability to suppress API from the reference assembly and do in some places today. I do like this proposal though, because it allows folks (presumably) to suppress this and presumably get the old overload resolution and temporarily unblock themselves, as they can with Obsolete.

@tannergooding
Copy link
Member

For instance, for CallerIdentity one options is to just remove the old members for the ref assembly and ensure that F# and VB and support the new attribute as well.

A big thing worth noting is that this applies far beyond just dotnet/runtime and the standard experience for ref assemblies doesn't allow members to be trivially stripped out and/or ignored.

@dakersnar dakersnar modified the milestones: 8.0.0, Future Sep 29, 2022
@dakersnar
Copy link
Contributor

dakersnar commented Sep 29, 2022

If this is something that the language team wants to commit to driving, we can move it back to 8.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Runtime
Projects
None yet
Development

No branches or pull requests