In Avalonia.Base, sealed several classes whose constructors aren't available from public API, and which have no inheritors

[Lehonti]

What does the pull request do?

Helps track which classes aren't inherited from, at least internally

What is the updated/expected behavior with this PR?

No behavior changes

Breaking changes

If there is an analysis tool that checks for breaking changes in public API, it may complain, but it really makes no difference

[avaloniaui-team]

You can test this PR using the following package version. 11.0.999-cibuild0040046-beta. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

[robloo]

If there is an analysis tool that checks for breaking changes in public API, it may complain, but it really makes no difference

How would it make no difference? Apps may be using base classes in several places you have now sealed. I really wish you would at least open a discussion before going through and making these types of PRs... I was one that was strongly stating we should allow these types of changes but you are doing some dangerous stuff.

Personally, I am not a fan of sealing too much. In fact I wouldn't accept this PR if it was up to me. If there are some performance critical paths that could be another discussion but I don't think that is how you approached this.

[Lehonti]

Hi @robloo, if the constructors in question are internal how would an app inherit from the classes?

[robloo]

Hi @robloo, if the constructors in question are internal how would an app inherit from the classes?

Yes, I suppose you're right from that standpoint. Why do classes like CubicBezierEasing having an internal constructor to begin with though? Seems plausible to derive from that some new easing in an application. Why would that even be locked down? Doesn't make sense to me. That can be said a few other places as well.

Sealing classes isn't simply a "well it isn't used so let's lock it down" type of discussion either. Is the class intended to be derivable? Is deriving something that should be supported from an API standpoint? That is an important question to answer before sealing anything and it's a question the core team or original author needs to answer in a broad sense.

[Lehonti]

Sealing classes isn't simply a "well it isn't used so let's lock it down" type of discussion either. Is the class intended to be derivable? Is deriving something that should be supported from an API standpoint? That is an important question to answer before sealing anything and it's a question the core team or original author needs to answer in a broad sense.

I understand where you are coming from.

To address your concern, let me ask you a question. What should the default behavior (sealed vs. unsealed) be, and why? In the C# language, classes are unsealed by default (imo an unfortunate design choice); but should the default behavior of the language determine your OOP design choices? If the designers of the language had determined that classes were to be sealed by default (which is the case of Kotlin, for example), would you say the opposite? (I am presuming that this is your line of thought, but correct me if I am wrong).

As you may guess, my choice is 'sealed by default'. If our roles were reversed, I would say "unsealing classes isn't simply a 'well, someone might want to inherit from it so let's open it up' type of discussion either. Is the class intended to be derivable? Is deriving something that should be supported from an API standpoint? That is an important question to answer before unsealing anything and it's a question the core team or original author needs to answer in a broad sense."

Why do I lean towards sealed by default? In short, ensuring as many invariants as possible. It's like the readonly of inheritance. Also, if a class is unsealed, the programmer may get the impression that the class was made to be inherited (and to handle certain scenarios), when in reality no thought was put into making the code inside the class play nicely with its inheritors. Every method that is made virtual or even just protected increases the 'surface area' for security vulnerabilities.

I remember a pull request from some time ago where I made certain private methods static and the reviewer questioned that, saying that they were static by chance, to which, if asked today, I would reply "I'm just adding an assurance at the language level that this method does not depend on instance members, and the ways it can modify them are more restricted; how can that possibly be a bad thing? If you ever need it to belong to an instance, you can remove the static qualifier just as easily".

Sorry if my pull request feels spammy (it may seem like that since, admittedly, my explanation for the PR was too short), or that I didn't give it enough thought; but hopefully you can see that there is a reasoning behind it.

[maxkatz6]

If there is an analysis tool that checks for breaking changes in public API, it may complain, but it really makes no difference

CI script should validate API surface using .NET SDK validators. Even though it is known to have some issues.
In this case, it does, indeed, seem safe.

Why do classes like CubicBezierEasing having an internal constructor to begin with though?

Our easing algorithms/classes are not extendable in this way. You can, though, implement your own IEasing type. Or use configurable SplineEasing and SpringEasing.
Quite possibly CubicBezierEasing should also be one of the configurable ones.

Sealing classes isn't simply a "well it isn't used so let's lock it down" type of discussion either. Is the class intended to be derivable? Is deriving something that should be supported from an API standpoint?

It's a good point. Especially coming from UWP, where everything is sealed by default (IIRC, WinRT IDL by itself seals everything by default).
This one was previously discussed, and we believed that everything should be sealed by default in the project as well. But each of these instances should be well discussed. Sometimes it's easy - controls should not be sealed, sometimes it's not so much.
In this PR it doesn't seem to be a difficult question, since these types can't be inherited by the developer anyway. At least in a conventional way.

[MrJul]

To focus on CubicBezierEasing only and not on the general "sealed vs unsealed" debate, I don't think that implementation was supposed to be public yet: it's only used internally and the public setters do nothing. I believe the rationale is that it doesn't derive from Easing and can't be converted easily through the EasingTypeConverter: a special syntax would have to be invented here to pass the two control points (I think that's the cubic-bezier(0.25, 0.1, 0.25, 1.0) in comment). Pinging @kekekeks for confirmation.

Since it's already public and can be useful, I believe it should be modified to be usable publicly, like other easings:

1. Remove the constructor completely
2. Inherit from Easing
3. Create the private CubicBezier implementation when necessary / reset if if the control points change
4. Optionally, handle the cubic-bezier(x1, x2, y1, y2) syntax to Easing.Parse

With 1-3 it's at least usable explicitly from C# and XAML, but not as a string, which requires point 4.

All that is completely out of scope of this PR. For now, I'd say to leave the class unsealed.

[avaloniaui-team]

You can test this PR using the following package version. 11.0.999-cibuild0040195-beta. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

[kekekeks]

Setters doing nothing look like an oversight/bug. That class should lazily create the CubicBezier instance and reset it when properties change.

[kekekeks]

Actually, it also seems to be duplicating the KeySpline thingy, except CubicBezier class is a line-by-line port from Chrome.

[maxkatz6]

I am going to merge this PR, as it doesn't break any API. It wasn't possible to inherit CubicBezier before or after this PR.
Please consider creating an issue or another PR changing this easing class independently from this PR.

[robloo]

Well, I do hope someone on the core team went through each and it makes sense to seal the types that were sealed from an API standpoint. There was at least one other: SpanVector that had a virtual method implying someone at one point thought it was a good idea to support inheritance. That virtual method was removed and the class sealed.

What should the default behavior (sealed vs. unsealed) be, and why? In the C# language, classes are unsealed by default

I didn't want to get into the weeds with the discussion. But you answered your own question -- unsealed is the default in C# and is most widely expected for app developers. Avalonia is also most useful because it is extensible and customizable. In 11.0 however a lot of that power has been lost and I'm not sure its for the best. I would NOT go around sealing things unless you know from a design standpoint it shouldn't be derived.

That means while in this case this PR doesn't change the API surface area I would not make a habit of sealing things. It locks things down too much for app developers and is usually unnecessary.

[MrJul]

SpanVector is internal so we can change it back if needed. It's likely ported from WPF (imo it should deleted in the future, we can do much better than the ported FrugalStructList nowadays - low priority though).

Internal and private classes are better sealed in .NET (Core), since the JIT can optimize them (for example is/as becomes a single int comparison instead of a hierarchy walk loop, virtual calls can be inlined, and probably more). There's even an analyzer for it, CA1852.

Unsealed by default is considered by some C# team members a mistake, I've seen it mentioned a couple of times in the C# language design repository. (Sorry, I can't find a relevant discussion right now, it's probably buried somewhere in an issue's comments.) Here's an old SO answer by Jon Skeet about it. Of course this position isn't universal and is highly debatable.

An unsealed class should be designed with inheritance in mind. Most often, unsealed without virtual is pretty useless: simply leaving a class unsealed won't make it easily replaceable by an inherited type if the system hasn't been designed for it. Sealed public classes can be unsealed later if necessary, whereas the opposite can't be done without making breaking changes.

That being said, I personally do agree that some things were made less extensible a bit too harshly, as I've commented before. My concerns are mostly about "pubternal" vs public, than sealed (at least now there's a way to opt-in to these API). But I can understand the rationale behind it: it's very hard to make non breaking changes, sometimes even for simple improvements, when everything is public and unsealed. I really hope some types can become public again in the future, once they've been well designed with that in mind. The same should be true for hierarchies.