Improper use of NotClientImplementable attribute

[xecrets]

Describe the bug

In commit d757326 recently merged and now part of 11.1.4, the [NotClientImplementable] attribute was added to the IClipboard interface (and a lot of other interfaces).

Considering the fact that the Avalonia implementations of IClipboard are still incomplete and not fully equivalent between the platforms, for example Linux does not present the default data formats, this is very unfortunate.

The addition of this attribute prevents an implementer to add their own decorated implementations to work around the issues present in the Avalonia implementation.

This is not an internal API that is normally unused by implementers, it's a public API accessible as TopLevel.Clipboard . If Avalonia in the future breaks this API in a way that would break a decorating implementation, it'll typically break any normal use of it as well. In this case I do not see any benefit at all in forbidding client implementations (decorators to work around Avalonia limitations typically).

To Reproduce

Have code that implements IClipboard and upgrade to 11.1.4 . Compilation breaks with a message This interface or abstract class is -not- implementable by user code !.

Expected behavior

I would expect a client implementation of IClipboard to be possible just as it was before 11.1.4 .

Avalonia version

11.1.4

OS

Windows, macOS, Linux

Additional context

No response

[stevemonaco]

That was changed over two years ago in preparation for v11. The API still public, so there's no appetite to make breaking changes to it outside of new major versions.

Implementations are intended for platform implementers, not applications. If you are implementing a platform, then the suggested approach is to put <AvaloniaAccessUnstablePrivateApis>true</AvaloniaAccessUnstablePrivateApis> into your csproj.

If you're writing an application and there's missing clipboard functionality, then the best approach is create a new issue with the scope of work, implement it yourself, and upstream it into Avalonia. If the missing clipboard functionality isn't appropriate for Avalonia, then stick it into a separate class library with <AvaloniaAccessUnstablePrivateApis>true</AvaloniaAccessUnstablePrivateApis> so your main app doesn't make unknowingly calls to private APIs. Private APIs do not have a stable API. You still need to work with AvaloniaLocator so your implementation is used instead of Avalonia's platform implementation.

[xecrets]

That was changed over two years ago in preparation for v11.

Yes, but was was only merged very recently for version 11.1.4, so it's a breaking change that just surfaced.

Implementations are intended for platform implementers, not applications.

Decorators are a well-known and very useful pattern, which are now made impossible because of this attribute.

If you're writing an application and there's missing clipboard functionality

Not necessarily missing, but I might have a need to customize and specialize behavior.

then the best approach is create a new issue with the scope of work, implement it yourself, and upstream it into Avalonia

That is of course nice, when realistic. But it's often hard to handle a delay for a year or two before the feature is merged into a public release, and depending on the application, the requirements for a specific app may be much less demanding than for a general purpose feature into the platform. It's also non-trivial to set up a development environment for Avalonia UI to do just a small feature add unfortunately, so it's a little hard to motivate the effort at times as much as I would like. I have contributed previously to other large project.

If the missing clipboard functionality isn't appropriate for Avalonia, then stick it into a separate class library with true

That's a global flag, and thus a little like swatting a fly with a sledgehammer. There's likely to be unwanted side effects. Creating a separate library just for one little decorator seems a little too much.

I realize I probably can't fight city hall on this one, but I honestly don't see the benefit of this attribute on this type of API. It's not an unstable private API. IClipboard is public, globally and easily accessible interface and not something esoteric from within the deep internals of the platform implementation.

If it's really seen as an unstable private API, then perhaps it should be made internal, and a new public interface should be exposed to application developers.

Anyway, thanks for the response, and for Avalonia UI!

[stevemonaco]

Yes, but was was only merged very recently for version 11.1.4, so it's a breaking change that just surfaced.

Ah, I see. This is due to #16626 where being able to build without opting in was mistakenly possible.

Decorators are a well-known and very useful pattern, which are now made impossible because of this attribute.

How are you decorating when the platform-specific implementations are not publicly accessible nor can you replace their use within Avalonia without opting in, AFAIK? Why should run-of-the-mill apps be able to tweak native platform implementations by default?

I realize I probably can't fight city hall on this one, but I honestly don't see the benefit of this attribute on this type of API.

I'm just one community contributor trying to give you the rationale, not a core maintainer. Avalonia is created across multiple packages instead of a monolith. Avalonia reserves space for itself (and its packages), enough space for third-party platform implementers, and less space to application developers. [NotClientImplementable] components like IClipboard are meant for platform implementers to implement and for application devs to consume, not extend.

[timunie]

As @stevemonaco said, it would be great to understand the real usecase. If it's a bug, it needs a fix. If it's a missing feature, it needs to be added at some point in time.

So please take your time to carefully explain what's your reason for need to implement this interface. Best with some sample code. Then we can discuss whatever way to go.

Tip

If you need priority on a certain feature or fix, you can request it in case you have a support agreement.

[xecrets]

Hi!

As mentioned, the reason is to decorate the platform implementation of a public interface. There are numerous reasons why this might be useful and legitimate, from adding missing functionality, to work around bugs, to facilitate debugging, adding logging, unit testing and many other reasons. A decorator is a very powerful design pattern, but now made impossible by the marking of a public interface such as IClipboard as [NotClientImplementable] .

It's not about implementing something the platform implements, but even so, I don't see why there's a need to forbid it. A warning, that is possible to disable, maybe... but this causes a compilation error. In fact, I could argue that it kind of destroys most of the benefits of having an interface at all! One of the absolute core benefits of interfaces is the ability to provide alternate implementations, now this possibility is removed for all but the platform developers. I can't even inject a unit test fake or mock with this restriction in place!

To demonstrate, I've put together a self-contained sample app here: https://github.com/xecrets/clipboard-decorator . It's both somewhat elaborate, but also a bare-bones sample thus not demonstrating best practices for production code in some ways. It compiles and works on Windows and Linux Ubuntu 22.04 - I've not tested it elsewhere.

What this sample does is provide simple support for accepting files on the clipboard on Linux, which is not supported otherwise. But, as mentioned, even if if this is or will be supported in the future, there are still numerous reasons why you might want to decorate the platform implementation.

It's now using Avalonia UI 11.1.3 and compiles and works fine. If upgraded to 11.1.4, it no longer compiles because of the added [NotClientImplementable] attribute.

I do not agree that an acceptable solution is to add <AvaloniaAccessUnstablePrivateApis>true</AvaloniaAccessUnstablePrivateApis> to the project file, since it totally sends the wrong signals. It's not about accessing an internal unstable private API! It's about a perfectly legitimate decorator to a public and presumably reasonably stable API!

IMHO it's a bug. The attribute should never have been added. It's also a breaking change, and should be marked and documented as such if the maintainers insist on it.

[efcoyote]

Same problem here with the IClassicDesktopStyleApplicationLifetime. The Avalonia team itself has recommended implementing the interface: 500910
That is not possible anymore without the mentioned AvaloniaAccessUnstablePrivateApis flag.

[stevemonaco]

The approach in your example is problematic:

IClipboard clipboard = mainWindow.Clipboard ?? throw new InvalidOperationException("No clipboard!");
IStorageProvider storageProvider = mainWindow.StorageProvider ?? throw new InvalidOperationException("No storage provider!");
IClipboard myClipboad = OperatingSystem.IsLinux() ?
	new MyLinuxClipboard(clipboard, storageProvider) :
	new MyDefaultClipboard(clipboard);

mainWindow.DataContext = new MainWindowViewModel(myClipboad);

You're consuming Avalonia types directly in the ViewModel. Ideally, the ViewModel should not know Avalonia exists. Instead, you should create your own interface or service class with a public API that matches your app's requirements (not simply a 1:1 IClipboard wrapper) and consume that instead. You can still inject the IClipboard instance into your interface/service implementation for base functionality, but it will be private and non-accessible to the VM.

There's no point in implementing IClipboard unless you're pushing your own implementation (decorated or full implementation) into Avalonia itself via AvaloniaLocator.

I don't see any problem with requiring an opt-in for such advanced functionality to keep a happy path for the bulk of the user base, but the team decides that.

[timunie]

Made this a discussion for now as we need to discuss the API here.

[xecrets]

You're consuming Avalonia types directly in the ViewModel.

Right. As I said it's a "sample thus not demonstrating best practices for production code". But somewhere I would like to consume IClipboard, and right now I can't decorate it regardless of where I consume it.

Instead, you should create your own interface or service class with a public API that matches your app's requirements

Of course I can do that when I have control over all of the code, which is essentially what I did to work around the current situation and get on with my app.

However, that is only possible when I have full control over all consumers if the clipboard. If I am using a third party component, or even framework library code that expects a IClipboard instance, I no longer have the option to provide a decorated instance if there is such need with the current [NotClientImplementable] restriction.

There's no point in implementing IClipboard unless you're pushing your own implementation

As mentioned, there are many scenarios when there's a good reason to implement for example IClipboard or any similar public interface exposed by the framework. Note that when I say implement I mean "implement as in implementing an interface", not "implement as in implementing for a platform". I am not interested here in making my own platform implementation of a clipboard, I am interested in tweaking, or inspecting, how it works in various ways. Decorating is useful in many scenarios as I mentioned.

This is not the only affected interface either, IClipboard just happened to be where my code blew up. I still don't see the danger or harm in allowing app-specific decorators, fakes, mocks and similar implementations. I would much appreciate an explanation why that is bad thing, so bad it must be disallowed with prejudice!

The interface API in question is a stable, public, API. It's not an internal, or private, or unstable API. What could break by allowing an app developer to 'implement' it, for whatever reason?

[kekekeks]

What could break by allowing an app developer to 'implement' it, for whatever reason?

• Avalonia 11.1.4 - exists
• 3rd party library implements IClipboard and gets published to nuget.org
• Avalonia 11.1.5 adds a new member to IClipboard
• TypeLoadException happens on usage attempt of said 3rd party library

[kekekeks]

Note that we are still targeting netstandard2.0/netfx that don't have "default interface implementations"

[xecrets]

3rd party library implements IClipboard and gets published to nuget.org

The scenario was rather 3rd party library consumes IClipboard, not implements .

Avalonia 11.1.5 adds a new member to IClipboard
TypeLoadException happens on usage attempt of said 3rd party library

It still blows up if you make a breaking change to IClipboard, just by having a library consume it. The addition of the [NotClientImplementable] doesn't change that.

From where I see it, a breaking change to IClipboard (as the example, reasoning applies to all similar), is, well... a breaking change for consumers as well as implementors. Adding the [NotClientImplementable] doesn't mitigate or change that, assuming that anyone wishing to implement IClipboard for example via a decorator, also has a consumer. So regardless, if you have a consumer of IClipboard and there's a breaking change, it breaks.

[grokys]

It still blows up if you make a breaking change to IClipboard, just by having a library consume it.

I may be missing something here, but how does adding an interface member break consumers?

[xecrets]

I may be missing something here, but how does adding an interface member break consumers?

No, you're right, it doesn't really break the consumer. My bad. But it's still considered a breaking change, and using the [NotClientImplementable] attribute as silver bullet solution to allow such breaking changes is just wrong.

My reasoning with the 3rd party library as a consumer was to illustrate the need to actually decorate for example IClipboard and introducing my own private IMyClipboard doesn't help in that case.

[grokys]

I still don't see the danger or harm in allowing app-specific decorators, fakes, mocks and similar implementations. I would much appreciate an explanation why that is bad thing, so bad it must be disallowed with prejudice!

The problem is that even adding a member to an interface is considered a breaking change, so allowing clients to implement e.g. IClipboard would mean that we are unable to do any further work on the clipboard implementation. This wouldn't be an issue if we didn't support netstandard2.0 as default interface implementations would be available. Unfortunately though, we do support netstandard2.0 as some of our users are still targeting .NET framework. This means that we're unable to use default interface implementations, and by extension any change to an interface is a breaking change. To avoid breaking user code we simply disallow implementing the interface unless a user explicitly opts-in by setting AvaloniaAccessUnstablePrivateApis.

[xecrets]

As a short-stop quick-fix, although I really don't want to see it as a permanent solution, create a property called AllowNotClientImplementable that opt-ins specifically to [NotClientImplementable] attributed types

I'm not sure how this would work on a technical level? Could you expand on how it would be implemented?

Ok, I was unclear. I meant as a project property, pretty much functionally synonymous with AvaloniaAccessUnstablePrivateApis and used in the same way - but signaling the correct intent with the usage in the project file.

[xecrets]

Perhaps the simplest solution would be for Avalonia to include a ClipboardBase abstract class which implements IClipboard; this would allow us to add new IClipboard members without breaking user code which derives from ClipboardBase.

Since this discussion is not really about IClipboard but rather about all similarly attributed interfaces with the [NotClientImplementable] attribute, that would be complicated and overly convoluted I think.

Why is there a need to keep it so "simple" to add members to existing stable public interfaces? In reality it doesn't happen often, at least not for the majority of the [NotClientImplementable] interfaces I think. And, as I mentioned in another comment, in my opinion, a better option is to create a new interface when the need does arise, either as a disjoint separate interface (that can still share the same implementation), or as an interface inheriting the original and extending it with the new member.

[xecrets]

We considered this, but it also marks all usages of the member as obsolete in the Avalonia codebase.

The would apply to [Experimental] no? Also, it's just one editing session to disable the warnings internally. If I can get a full build environment working I'd be happy to do it. It really shouldn't be that much work, and once it's done, it's done, and it'll also be easy to find the usages.

[grokys]

The would apply to [Experimental] no?

Honestly, I've never used [Experimental] (as we can't use it here in 11.x for reasons already mentioned). I had assumed that usages of [Experimental] APIs in the same assembly wouldn't be marked as warnings, but I may be wrong.

[xecrets]

I had assumed that usages of [Experimental] APIs in the same assembly wouldn't be marked as warnings,

I haven't used it either, but for future reference, I read up on it now, and also tried it out. The specification says:

The diagnostic is reported for any reference to a type or member that is either:

• marked with the attribute,
• in an assembly or module marked with the attribute,

except when the reference occurs within [Experimental] members (automatic suppression).

In a small test, it will flag all uses (except the automatic suppression case above), even within the same assembly.

[stevemonaco]

Perhaps the simplest solution would be for Avalonia to include a ClipboardBase abstract class which implements IClipboard; this would allow us to add new IClipboard members without breaking user code which derives from ClipboardBase.

There are dozens of types with [NotClientImplementable], so it's a large cost to expose these via more stable concrete types.

Why is there a need to keep it so "simple" to add members to existing stable public interfaces?

These are clearly not intended to be stable, public interfaces in the same way that an API like TextBlock.Text is. If they were, they wouldn't have the attribute. They're primarily meant to be consumed by Avalonia components, so if these interfaces cannot change, then Avalonia itself is in a worse spot and harder to improve/maintain. They're for platform implementers (including Avalonia) or well-maintained apps that can afford to take breaking changes.

As a short-stop quick-fix, although I really don't want to see it as a permanent solution, create a property called AllowNotClientImplementable that opt-ins specifically to [NotClientImplementable] attributed types.

As project property? Can't. [NotClientImplementable], [PrivateApi], and interfaces ending with Impl have their members removed in the patched assembly that Avalonia distributes to NuGet. AvaloniaAccessUnstablePrivateApis lets you use the original assembly. I'm not sure if it's possible to ship more assemblies with granular patching. Avalonia perhaps could instead ship an analyzer to enforce it instead. Might be some issues with IntelliSense exposing too much if it were extended into [PrivateApi] though.

[xecrets]

These are clearly not intended to be stable, public interfaces in the same way that an API like TextBlock.Text is. If they were, they wouldn't have the attribute. They're primarily meant to be consumed by Avalonia components, so if these interfaces cannot change, then Avalonia itself is in a worse spot and harder to improve/maintain. They're for platform implementers (including Avalonia) or well-maintained apps that can afford to take breaking changes.

But they are stable, public interfaces (at least a lot of them)! They are publically exposed, and must be used to gain access to functionality essential for most apps.

And if I'm allowed or even forced to consume them, I really want the option to decorate them, and to create test fakes etc.

Examples, in addition to IClipboard are: IAnimation IBinding IDataContextProvider IPointer IBrush IGradientBrush IBitmap IStorageFile IStorageFolder IStorageItem IStorageProvider IClassicDesktopStyleApplicationLifetime . In total about 85 interfaces have this attribute, but these are ones that I immediately recognize and have used in user level app code.

If they really are unstable, fine.

But although history is no sure guide to the future, let's have a look at IClipboard.

It was created 9/6/2015.
It was changed on 5/29/2020 - both adding and changing members, thus breaking in any case.
It was slightly changed on 2/6/2023 - adding nullability to some arguments, but no additions, changes or removals of members.

So, the only change in 9 years was a hard breaking change 4 years ago. There have been no non-breaking changes for consumers since then. Pretty stable, and it should be, it's a pretty standard interface to pretty standard functionality.

[xecrets]

As project property? Can't. [NotClientImplementable], [PrivateApi], and interfaces ending with Impl have their members removed in the patched assembly that Avalonia distributes to NuGet. AvaloniaAccessUnstablePrivateApis lets you use the original assembly.

Now I'm curious! How does this work? How can I consume IClipboard if its members are removed during a patch-step prior to nuget-distribution? I guess it's a little off-topic, but I'd love to understand and learn how this works!

[stevemonaco]

Good point. You're correct that [NotClientImplementable] doesn't hide its members. The patching code appears to add an internal (Assembly) method that can't be implemented externally (besides the opt-in):

Avalonia/nukebuild/RefAssemblyGenerator.cs

Line 71 in 0bd90d8

static void ProcessType(TypeDefinition type, MethodReference obsoleteCtor)

The method name is also serving as the error message itself.

[kekekeks]

Please understand that pre-11.0 Avalonia didn't really have a stable public API and we were following "make everything extendable in case we need to extend it later" rule. When stable ABI got introduced with 11.0, we've had a choice of either spending extra months of work on redesigning all APIs (e. g. IClipboard would have become sealed class Clipboard{ internal Clipboard(IClipboardImpl)}), introduce even more breaking changes than we've already had, or, since we have limited resources, doing what we did and introducing private and non-user implementable API concepts.

In retrospect, it following "make everything extendable" was a huge mistake and we should have made everything internal in the first place (I now have a strong belief that not having automatic sealed class modifier is the most major design flaw of the C# language).

This decision will not get revisited now since we don't want to break most common usage scenarios that are not trying to implement private APIs in favor of some extra extensibility that can be achieved on app level by introducing an extra level of abstraction.

As of the "shipping an analyzer" idea, I'd like to remind that .NET is not C#, is not owned by C# devs and has other languages compiled without Roslyn. The reference assembly way of restricting access to APIs works for other languages too.

[xecrets]

Please understand that pre-11.0 Avalonia didn't really have a stable public API and we were following "make everything extendable in case we need to extend it later" rule. When stable ABI got introduced with 11.0, we've had a choice of either spending extra months of work on redesigning all APIs (e. g. IClipboard would have become sealed class Clipboard{ internal Clipboard(IClipboardImpl)}), introduce even more breaking changes than we've already had, or, since we have limited resources, doing what we did and introducing private and non-user implementable API concepts.

I get the transition from pre-11.0 unstable to 11.x stable was complicated. I still don't see why you need to forbid app developers to use and decorate etc. the public interfaces as they are. As mentioned, you shouldn't be changing the interfaces anyway, and you shouldn't need to either in most cases, since in fact they've been around for quite some time.

Instead, if you need to add members, create a new interface. That separates concerns nicely and maintains full binary compatibility all the way. ...and in actual fact, for most of the interfaces in question, it won't be a common occurrence. If some are known to be still experimental, either mark them as [Obsolete] (I volunteer, although I might need some guidance to setup a full development environment), or keep the [NotClientImplementable] attribute for those that really need it.

[xecrets]

This decision will not get revisited now since we don't want to break most common usage scenarios that are not trying to implement private APIs in favor of some extra extensibility that can be achieved on app level by introducing an extra level of abstraction.

Unfortunately instead you did break all the scenarios where someone did implement one of these API's, which is not perfect, and it happened quietly in a minor update from 11.1.3 to 11.1.4.

I don't see that you'd break anything by not having the [NotClientImplementable] attribute on these interfaces. For the most common usage scenarios, it works fine with or without the attribute, or am I missing something?

[kekekeks]

the public interfaces as they are

We've already explained why. We reserve the right to add new interface members in patch releases. This is not going to change.
We are not dropping netfx support at least until V12 either.

[kekekeks]

Unfortunately instead you did break all the scenarios where someone did implement one of these API's

For your applications (not public nuget libraries) it's perfectly safe to use private APIs, you can enable the relevant property, that's not going to create an unsolvable TypeLoadException problem

[xecrets]

We've already explained why. We reserve the right to add new interface members in patch releases. This is not going to change.
We are not dropping netfx support at least until V12 either

Understood. In the end the maintainers decide, I can only put forward my views as a consumer of the framework and when relevant offer to contribute, which I've done. Thanks anyway for listening and for Avalonia!

[kekekeks]

I'm closing this discussion since we won't be changing the way NonClientImplementable works

[grokys]

*For 11.x

[edgarfgp]

This also breaks Fabulous.Avalonia mobile support fabulous-dev/Fabulous.Avalonia#261.

We relied on ISingleViewApplicationLifetime to initialise the iOS view and lifecycle.

So this is essentially game over for F# Fabulous Avalonia mobile support. Would you consider removing NotClientImplementable attribute from ISingleViewApplicationLifetime @kekekeks ?

cc @TimLariviere

[maxkatz6]

What exactly are you trying to achieve with a custom lifetime and custom iOS initialization? Of what can't be done with existing backend. I would focus on this problem instead.

Would you consider removing NotClientImplementable attribute from ISingleViewApplicationLifetime @kekekeks ?

Unlikely. Lifetimes are not supposed to be implemented by user code.

[edgarfgp]

@maxkatz6 we were supporting SceneDelegate(multi-window) uspport, hence we had our own custom lifetime and custom iOS initialization. But I think for now we will rely on AvaloniaAppDelegate for now which will unblock us for now.