Add proposal for BinaryCompatOnlyAttribute

[333fred]

Adds a proposal for an oft-mentioned feature, BinaryCompatOnlyAttribute, that we've never actually gotten around to proposing before. @jaredpar for review

[teo-tsirpanis]

What about type-forwarding a BinaryCompatOnly type?

[333fred]

@teo-tsirpanis as currently proposed, you couldn't do that. May be another reason to look at the alternative version though.

[rickbrew]

If it's implemented as a flag for [Obsolete], as discussed in #7706, that should work. The ability to use [Obsolete] and [assembly: TypeForwardedTo(...)] was fixed with dotnet/roslyn#61264

[333fred]

If it's implemented as a flag for [Obsolete], as discussed in #7706, that should work. The ability to use [Obsolete] and [assembly: TypeForwardedTo(...)] was fixed with dotnet/roslyn#61264

Even if it used Obsolete, as currently worded, it would not work, as the type is literally invisible in the current compilation. We will have to see how hard a line we want to take. I will point out, @rickbrew, that if we did allow types and members to be visible in the current compilation, then your stated use case in #7706 (comment) would no longer be possible, as you would still be beholden to the overloading rules :)

[rickbrew]

Yeah I guess the compiler would also have to special case allowing it to be seen within uses of [assembly: TypeForwardedTo(...)]. I would definitely want to use all of these together though! Hopefully a good solution can be found to enable that.

[RikkiGibson]

Is an error still issued when two members differ only by presence of BinaryCompatOnlyAttribute? Since the runtime can't distinguish which one you meant to call in that case?

class C
{
    [BinaryCompatOnly]
    public void M(int x) { }

    public void M(int x) { } // error?
}

[jaredpar]

Is it inaccessible from within the current compilation or just referencing compilations?

[jaredpar]

Another motivation is that lacking a feature like [BinaryCompatOnly] we are limited in our ability to add new caller info style attributes. These two issues have good discussions on this: caller identity and xunit

[jaredpar]

The interface member part will lead to some interesting situations. Consider the following as a concret example:

// assembly1:
public abstract class C {
  public void Dispose() { }
}

// assembly2:
public class D : C, IDisposable { } 

Now lets say in version 2 the author of assembly1 marks C.Dispose with [BinaryCompatOnly]. Execution of assembly2 will continue to work fine because C.Dispose still satisfies IDisposable.Dispose by CLR rules. The moment the author moves to a new version of assembly1 they will get a compilation error and be forced to re-implement IDisposable.Dispose. That overall may be okay but it seems like a potential pitfall in some cases.

[jaredpar]

How does this work with say unit testing? The primary consumer of this technology is likely to be the .NET libraries / runtime team. The [BinaryCompatOnly] attribute will make the method inaccessible to new code but old code will still use it. The method will exist virtually forever for this reason.

How will the libraries team write unit tests for this method that they need to maintain forever?

[jnm2]

Maybe TestUtils.CreateDelegate<Action<DeclaringType, TParam1, ...>>("MethodName") and similar strategies for invoking the method using reflection with low ceremony at the call site?

[jaredpar]

The interface member part will lead to some interesting situations. Consider the following as a concret example:

// assembly1:
public abstract class C {
  public void Dispose() { }
}

// assembly2:
public class D : C, IDisposable { } 

Now lets say in version 2 the author of assembly1 marks C.Dispose with [BinaryCompatOnly]. Execution of assembly2 will continue to work fine because C.Dispose still satisfies IDisposable.Dispose by CLR rules. The moment the author moves to a new version of assembly1 they will get a compilation error and be forced to re-implement IDisposable.Dispose. That overall may be okay but it seems like a potential pitfall in some cases.

[CyrusNajmabadi]

so can a method not call itself?

what if i have multiple BCOA members, where one calls the other. i feel like that would be totally normal.

[CyrusNajmabadi]

Consequentely, this means they cannot implement interface members

this also seems wrong. Say i have a BCOA interface member, and then the BCOA impl of htat interface member. That seems no longer supported.

[Youssef1313]

Can a BinaryCompatOnly member reference another BinaryCompatOnly member?
e.g,

[BinaryCompatOnly]
public class C1 { }

[BinaryCompatOnly]
public class C2 : C1 { }

[rickbrew]

I think that should be possible, just like it is with [Obsolete] elements referring to other [Obsolete] elements

[333fred]

As currently proposed, no. This is one of several open questions that I get into.

[hamarb123]

Just adding my thoughts on how the feature can be improved, and ideas to solve some of the unresolved issues / questions.

[hamarb123]

For this, I think we could allow it like so: if an interface member definition is marked BinaryCompatOnly, then its implementation must also be explicit, and marked BinaryCompatOnly. All of the parameters & return type would have to exactly match also.

[hamarb123]

Perhaps we could have something like BinaryCompatOnlyImplementation(Type), where it specifies an interface which should only be considered to be implemented for binary compatibility purposes. This would allow removing interfaces implementations from usage in source, without breaking their usage in binary.

With this setup, you would disallow explicit interface implementations from being marked BinaryCompatOnly, unless their definition is also this as I proposed in my other comment; for a non-BinaryCompatOnly definition, if the implicit one is marked BinaryCompatOnly, then an explicit one would have to be provided.

[hamarb123]

I think a good solution to this is the following: disallow referencing BinaryCompatOnly members (other than types) completely, their implementation can be extracted to a private / internal member as required which the BinaryCompatOnly will stub to. For unit tests, you would either test the stub, or use reflection.

There is a more interesting question with types though, as you still need to be able to use them in signatures probably (even if just other BinaryCompatOnly signatures); I think the solution to this could be: only use that type if there's no other type which matches (i.e., force all BinaryCompatOnly types to match very last, after every other possibility) (or can be done by specifying full name global:: if needed - probably the recommended way to do it), and emit a warning on usage of a BinaryCompatOnly type (default error severity), which can be suppressed if the usage is intentional. Potential (quite reasonable) adjustment could be that: this only applies for already BinaryCompatOnly members/types, it would be disallowed & ignored otherwise.

[hamarb123]

I mentioned a potential way to solve the usability concerns around this section in my comments below.

I don't understand why hiding would be disallowed here - doesn't shadowing have nothing to do with the original member really? Especially since BinaryCompatOnly effecively pre-emptively shadows a member before anyone can even use it, so I don't see why it would be disallowed here.

[hamarb123]

I think this shouldn't be possible, you can always extract a binary compat member's impl to another private / internal member as required, and then just keep the binary compat member as a stub to it. Since the member would not be exposed externally, you can just name it whatever works; this allows effectively calling the compat only member, without having to complicate overload rules and similar to figure out which one of the potentially conflicting overloads you wanted (if we allow overloading by ref/virtual in, return types, etc with this feature, which I assume we're allowing, as long as they're binary different).

[KalleOlaviNiemitalo]

I presume this should be sealed.

[333fred]

After some workshopping with @jaredpar offline, I'm not longer convinced this is quite the right approach. I'll include it in the alternatives section of a followup, but I'm now thinking more along the lines of simply being able to set the relative priority of an overload during the applicability step of overload resolution. I'll get that proposal out sometime soon.

[CyrusNajmabadi]

of simply being able to set the relative priority of an overload during the applicability step of overload resolution.

I'm in favor of this, as long as relative priority is specified in either english terms (like "better, betterest, evenbetterest"), or if it's done with floating point numbers (or qubits).

[hamarb123]

I'm in favor of this, as long as relative priority is specified in either english terms (like "better, betterest, evenbetterest"), or if it's done with floating point numbers (or qubits).

This sounds like a joke to me 😆 (referring to the english terms part)

[Overload("betterest")]
void a();

I'd think we should be able to get away with ints, if not for extension methods, so double or something similar should work probably.

[hamarb123]

Another question is whether we'd be able to change the return type still if we just allow adjusting overload resolution...

e.g., if I wrote

IEnumerator<int> GetEnumerator() => ...;

some years ago

ideally I would be able to replace it with

[Overload(-1)] IEnumerator<int> GetEnumerator() => ...;
MyStructEnumerator GetEnumerator() => ...; //assume 0 is default, and higher number takes precedence

at some point, but this would obviously be illegal to write currently (without special treatment for overload checking in presence of [Overload]).

And then there's the question of what should I expect the following to call

//should one of these ignore the overload precedences and call the -1 api? if not, it will be hard to call for testing and similar, but if so then it will be hard to ensure the new API is actually called
IEnumerable<int> a = GetEnumerator();
IEnumerable<int> a = (IEnumerable<int>)GetEnumerator();

I am generally in favour of this idea though. Perhaps we could specially assign [Overload(double.NaN)] for "never, ever call this overload".

[333fred]

Another question is whether we'd be able to change the return type still if we just allow adjusting overload resolution

I don't expect so, but I also think the true BinaryCompatOnly option that removed it from the ref assembly has unworkable problems. For example, you'd never be able to test your binary compat only methods, so things like the BCL would simply never use it.

[hamarb123]

I don't expect so, but I also think the true BinaryCompatOnly option that removed it from the ref assembly has unworkable problems. For example, you'd never be able to test your binary compat only methods, so things like the BCL would simply never use it.

That's a shame. Also, why did BinaryCompatOnly need to remove from the ref assembly? I don't personally remember reading about that - it seems unnecessary to me.

In terms of testing: it's easy enough to move the contents of a BinaryCompatOnly member to a helper method with e.g., internal visibility, and we could have an optional analyser for if any BinaryCompatOnly member's impl wasn't only calling a helper member, which would effectively ensure testability of the binary compat member itself.

[CyrusNajmabadi]

This sounds like a joke to me

It would be impossible for me to be any more seriouser here (or 'cyrusier' if you prefer).

[333fred]

Also, why did BinaryCompatOnly need to remove from the ref assembly

That was the model it was going for, and what my proposed rules effectively did.

[jaredpar]

I'm very much convinced adjusting priority is a more promising approach. At the same time I feel bad closing this design without storing it somewhere. The design is fairly complete, captures the big ideas and has a good detailing of the problems that comes with it. Seems like it may be valuable for future reference. Should we just move it to rejected and merge?

[333fred]

I'm going to include it in the alternative section of my followup

[Neme12]

Why was this scrapped :( this is much more versatile than adjusting priority of overloads. This could even allow changing the return type of a property. This could allow outright fixing mistakes, not just trying to hide them or deprioritize them, and it would cut down on all the bloat from multiple overloads added over time.

[Neme12]

This would cut down on complexity, and meanwhile the other approach only increases complexity - and complexity in overload resolution by adding yet more rules - the one place that really doesn't need any more complexity.

[Neme12]

For example, you'd never be able to test your binary compat only methods, so things like the BCL would simply never use it.

You could allow calling those methods via something like UnsafeAccessor.

[HaloFour]

This could even allow changing the return type of a property.

Theoretically so could overload priority. Currently the C# compiler is capable of understanding a type that has overloads that only differ by return type, it just can't resolve the ambiguity so it becomes a compiler error. That attribute could be used to note which overload has priority, so the compiler could call the prioritized overload. The lower priority overload could also then be marked as obsolete.

You would then need to couple it with a language proposal to lift the restriction on being able to declare overloads that differ only by return type.

I guess it gets more complicated if the overload differs due to slightly different parameter types, although I think the overload priority would take precedence over the current type coercion rules.

[333fred]

Why was this scrapped

Because I don't believe there's a viable path forward for this proposal.

You could allow calling those methods via something like UnsafeAccessor.

Even if this was a palatable solution (I don't think it would be, as it would require large code changes in swaths of APIs that would want to be removed), calling these methods from tests are only the tip of the iceberg. The issues around OHI are where this really gets impossible.