Skip to content
This repository has been archived by the owner on Sep 13, 2022. It is now read-only.

Require support for default implementations of interfaces #1019

Merged
merged 1 commit into from
Jun 26, 2019

Conversation

terrajobst
Copy link
Member

@terrajobst terrajobst commented Dec 13, 2018

This marker will require all implementations of .NET Standard 2.1 to support default implementations of interfaces. Needless to say that this has runtime impact. The benefit of doing this in the standard is that it allows library authors to use this feature for their interfaces. The downside is that that this is potentially a lot of runtime work. I'm curious to how the board feels about absorbing that change for .NET Standard 2.1, especially @dotnet/nsboard-xamarin and @dotnet/nsboard-unity.

This also includes the new runtime exception AmbiguousImplementationException that will be thrown in cases where the runtime can't decide between conflicting default implementations in the diamond case (https://github.com/dotnet/corefx/issues/34124).

@terrajobst terrajobst added netstandard-api This tracks requests for standardizing APIs. runtime-impact Marks API suggestions that require runtime changes labels Dec 13, 2018
@terrajobst terrajobst added this to the .NET Standard 2.1 milestone Dec 13, 2018
@terrajobst terrajobst requested review from a team as code owners December 13, 2018 19:52
@MichalStrehovsky
Copy link
Member

This change also has impact on tooling and everyone who does something with the file format. Isn't adding this to NetStandard 2.1 rushed? We haven't even field tested the feature yet to demonstrate the value. The feature is highly controversial - e.g. look at the amount of downvotes in dotnet/csharplang#288.

The major motivating factor for default interface methods is interop with Java - as such, libraries that don't need to interoperate with Java have little need to use the feature. Libraries that interop with Java are unlikely to be able to target NetStandard 2.1 anyway.

@joshpeterson
Copy link

@terrajobst: Can you provide more details about what default implementations of interfaces are? Are there any design documents availble?

@MichalStrehovsky
Copy link
Member

@joshpeterson I pushed out a spec in dotnet/coreclr#21564

@cartermp
Copy link

Agreed with @MichalStrehovsky, this feels premature. We don't even have a released version of the C# compiler that uses this feature, thus we don't actually have good real-world feedback on it. Why require it for .NET Standard 2.1?

@terrajobst
Copy link
Member Author

terrajobst commented Dec 17, 2018

[@MichalStrehovsky] Isn't adding this to NetStandard 2.1 rushed?

My understanding is that this feature is committed for .NET Core 3.0. In that sense it's stable and "locked in". There are other aspects, such as language and tooling changes, but I don't think we'd have to wait for them to be fully done, nor do we necessarily want to. Keep in mind that while we can rev .NET Standard faster than before (because we no longer have to wait for .NET Framework) there is still an element of opportunity loss. Whether or not that's high depends on (1) how compelling the feature is (2) how attractive it is for library authors.

[@cartermp] Why require it for .NET Standard 2.1?

I'm generally a bit afraid of holding off on type system features for too long as it potential takes away of bunch of opportunity for library authors and risks bifurcation in the ecosystem. That being said, I'm not up to date with how well/committed the work for default implementations of interface is. My primary motivation for filing this work item is to have a place for that discussion. I'll reach out to a few more folks to gauge where we are with this.

@cartermp
Copy link

Note: when I wrote "released", I meant "released and non-preview" 🙂

Given our very low usage of Preview releases all-up I'm not confident that feedback we get from those channels is representative.

@forki
Copy link

forki commented Dec 17, 2018 via email

@MichalStrehovsky
Copy link
Member

Note: when I wrote "released", I meant "released and non-preview"

The feature didn't make it to the current C# 8 preview so it would be true for both interpretations.

Is there a document about what you meant with "Java interop"?

Projections of e.g. Android APIs in Xamarin tools.

@joshpeterson
Copy link

@MichalStrehovsky: Thanks for the design documents (and related links). They were very helpful.

@terrajobst: If this PR is merged, then will any runtime supporting .NET Standard 2.1 be required to implement default interface implementations? I think this differs from System.Reflection.Emit, which is in .NET Standard 2.1, but might not be implemented in a given runtime.

@terrajobst
Copy link
Member Author

terrajobst commented Dec 18, 2018

If this PR is merged, then will any runtime supporting .NET Standard 2.1 be required to implement default interface implementations?

Yes. Compilers (such as C#) are expected to use the presence of this field to decide whether or not to allow default interface implementations. If the field is present, the runtime is expected to be able to load & execute the resulting code. For .NET Standard this means any runtime implementing it.

I think this differs from System.Reflection.Emit, which is in .NET Standard 2.1, but might not be implemented in a given runtime.

Correct, which is why those aren't string fields but Boolean properties.

@joshpeterson
Copy link

@terrajobst: Thanks for clarifying.

@YairHalberstadt
Copy link

One of the main motivating factors for default interface methods is to allow library authors to evolve existing interfaces.

Given that MSDN guidelines are to target .Net Standard rather than .Net Core for libraries, this feature will be of only limited use unless .Net Standard includes it.

@terrajobst
Copy link
Member Author

One of the main motivating factors for default interface methods is to allow library authors to evolve existing interfaces.

Given that MSDN guidelines are to target .Net Standard rather than .Net Core for libraries, this feature will be of only limited use unless .Net Standard includes it.

Agreed.

@terrajobst
Copy link
Member Author

@joshpeterson

So what are your thoughts on supporting default interface methods in Unity? Do you think you'll be able to?

@joshpeterson
Copy link

@terrajobst

It will be possible, although not trivial. We will use Mono's implementation once it is ready, then implement it in IL2CPP as well.

My biggest concern is that we don't get ahead of ourselves and require a feature that won't be in C# 8. But I'm fine with this, I'll approve this PR.

@MichalStrehovsky
Copy link
Member

MichalStrehovsky commented Jan 14, 2019

As for implementing these interfaces, the language would likely require implementing all members, including those with default implementations.

@terrajobst this pertains to the discussion about doing fundamental changes in CoreFX that would leverage default interface methods (such as making IList<T> extend IReadOnlyList<T> and providing default implementations of the members to make it not breaking) - this is going to be a source breaking change for all .NET languages (and tools) not aware of default interface methods: people who use old version of the C# compiler, or users of VB or F# would be equally broken as if we added IReadOnlyList<T> to IList<T>'s interface list without providing the default implementation at all.

@YairHalberstadt
Copy link

users of VB or F# would be equally broken as if we added IReadOnlyList to IList's interface list without providing the default implementation at all.

I believe F# intends to support consuming default inteface methods, just not declaring them. I don't know about VB, but I imagine it intends to do the same.

@YairHalberstadt
Copy link

people who use old version of the C# compiler

I imagine the number of people who are prepared to upgrade to the latest version of .Net Core, but not the latest version of the language are few and far between, seeing as the first has far greater capacity for breaking changes than the second.

@cartermp
Copy link

We intend on allowing F# to consume DIMs, but there aren't plants to produce them. But if there's a source breaking change involved with CoreFX adoption then this feels like a huge nonstarter.

@Grauenwolf
Copy link

users of VB or F# would be equally broken as if we added IReadOnlyList to IList's interface list without providing the default implementation at all.

That's not the change that concerns me. In another thread they are talking about adding entirely new concepts such as ICollection<T>.AddRange.

@KathleenDollard
Copy link

VB expects to support DIM. The extent of that implication will be determined after details of the C# implementation are complete. Consumption will be the lower bar. It may not be in the same release that C# implements DIM, but in the following minor release.

I think this is a feature that needs to be available for a bit before it can be used. It seems that CoreFx will need to wait until there is confidence that programmers will be using the new compiler and C# 8. VB will basically be in the same position as older compilers for a (hopefully short) time.

I have mixed feelings about including this in .NET Standard 2.1. I don't feel strongly against it, it just feels a bit premature.

@terrajobst
Copy link
Member Author

@Grauenwolf

I'm also thinking about the semantic versioning aspect. If we are following those rules,
then breaking changes between 2.0 and 2.1 are not permitted.

It's not a breaking change. It's a new feature that has the potential to be source breaking, depending on use. .NET Standard 2.1 isn't introducing any interfaces that have default implementations. But it allows developers to code against .NET Standard to author them.

Which means even if this capability is offered, nothing in the .NET Standard should actually use it yet.

That's the plan.

@cartermp

But if there's a source breaking change involved with CoreFX adoption then this feels like a huge nonstarter.

Agreed. At the same time, I think when VB and F# offer full support for them, we'll leverage them in the BCL. Yes, it might break some other language that doesn't support DIMs but

  1. It's not binary breaking change, only a source breaking change.
  2. I don't think many (if any) 3rd party language support .NET Core yet
  3. There is a work around (implement the newly added members or compile against the old version)

@KathleenDollard

I have mixed feelings about including this in .NET Standard 2.1. I don't feel strongly against it, it just feels a bit premature.

I don't feel super strongly that we need to add it; however I think all languages planning on adding support for it are better off if their customers can upgrade to RTM quality framework version to use the feature. Without this PR, library authors targeting the standard won't be able to use DIMs as a conforming compiler should disallow this without the field in RuntimeFeatures. In other words, adding it now makes it easier to ship previews of the compiler as it doesn't need to be paired with runtime/framework previews as this work is already done & shipped.

@terrajobst
Copy link
Member Author

terrajobst commented Jan 16, 2019

Before merging, we should review @MichalStrehovsky issue on the exception that is thrown in the diamond case (https://github.com/dotnet/corefx/issues/34124). If we decide to take it we should update this PR to add this exception type as well.

@terrajobst terrajobst added the * NO MERGE * Applied to pull-requests that aren't ready to be merged yet. label Jan 16, 2019
@Grauenwolf
Copy link

Regarding point no. 3, it won't be so easy for non-DIM supporting languages to implement the missing methods. Developers using a non-DIM supporting language will have to implement the new methods from scratch.

What this means is that if we add an interface method using DIM, we need to provide a reference implementation. I was originally thinking that this would be in the documentation, but that isn't ideal because it would be in the wrong language.

So what's the solution? Probably we'll need to provide all DIM methods as extension methods first. This way non-DIM supporting methods won't be forced to reinvent the wheel.

@terrajobst
Copy link
Member Author

terrajobst commented Feb 21, 2019

@Grauenwolf

Regarding point no. 3, it won't be so easy for non-DIM supporting languages to implement the missing methods. Developers using a non-DIM supporting language will have to implement the new methods from scratch.

That's a fair point that we need to consider. However, I doubt that many interface methods will have non-trivial implementations. For the most part, they will likely dispatch to other methods, return simple values (false, null) or throw NotSupportedException.

I was originally thinking that this would be in the documentation, but that isn't ideal because it would be in the wrong language.

The docs could trivially include a link to the .NET Core implementation (linking to GitHub or source.dot.net). Yes, it's not ideal because they will likely be in C#, but it will show the expected behavior that a 3rd party should match. Given that the implementation will likely be a few lines at best, that's probably not too bad.

@YairHalberstadt
Copy link

If .Net Core 3 is released before C# 8.0, as seems likely, we're going to have a large number of developers writing .Net Core 3 code in C# 7. Once C# 8.0 is released, and libraries start using DIMs, there's going to be a period of pain until developers upgrade to C# 8.

This will be the case whether or not .Net Standard 2.1 supports DIMs, but doing so would exacerbate such an issue.

@terrajobst
Copy link
Member Author

If .Net Core 3 is released before C# 8.0

C# 8 will be released with .NET Core 3.0 and .NET Standard 2.1 so I don't think this will be an issue. And even if so, it's not something that can be easily avoided.

@terrajobst terrajobst self-assigned this Mar 8, 2019
@terrajobst terrajobst force-pushed the default-impl-of-ifaces branch 2 times, most recently from b8ab0b8 to 0e56fca Compare March 15, 2019 19:27
@terrajobst terrajobst removed the * NO MERGE * Applied to pull-requests that aren't ready to be merged yet. label Mar 15, 2019
@terrajobst
Copy link
Member Author

I've rebased this PR on master and added the AmbiguousImplementationException. Last chance to raise any concerns. Otherwise it's going in :-)

@MichalStrehovsky @jkotas @stephentoub

@MichalStrehovsky
Copy link
Member

MichalStrehovsky commented Mar 15, 2019

There might be another exception type that we'll need to throw for the reabstraction case (overriding a default interface method with an abstract method). This is a new request coming out of this week's C# LDM meeting and it's still under disussions.

The runtime spec for default interface methods is not final yet. I'll leave it up to the NS board to figure out the implications of that on the Standard (whether we want to merge now, or we need to wait).

@terrajobst terrajobst added the * NO MERGE * Applied to pull-requests that aren't ready to be merged yet. label Mar 15, 2019
@terrajobst
Copy link
Member Author

Sounds like we should still wait then.

@terrajobst terrajobst removed the * NO MERGE * Applied to pull-requests that aren't ready to be merged yet. label Jun 26, 2019
@terrajobst
Copy link
Member Author

As discussed offline we agree there is nothing preventing this from being merged for 2.1, so here we go.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
netstandard-api This tracks requests for standardizing APIs. runtime-impact Marks API suggestions that require runtime changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.