Remove unnecessary casting from Clone() methods

[NightOwl888]

In Java, overridden methods in subclasses can return a different type than the base class. Therefore, it is common in Java to declare the clone() method with the subclass type so there is no need for the consumer to cast the return value of the method.

In .NET, this only works if a class is sealed, since it would otherwise constrain the subclass to that of the base class type. While we don't use the ICloneable interface in .NET per Microsoft's recommendation, we left the return type as object for compatibility and provide an option for 3rd parties to create a custom compile that implements ICloneable in all of the appropriate places. While this affects usability somewhat by requiring a cast, the fact of the matter is object return type is the only thing we can do consistently across the API even if we took out the ICloneable option.

Although the return type of Clone() is always object, many of the original casts to a specific type were carried over from Java and they can now be removed. In particular, there are some calls to MemberwiseClone() that are cast to a more specific type even though the return type is object and there is no need to set any of its members.

        public virtual object Clone()
        {
            return (MergeScheduler)base.MemberwiseClone();
        }

[olivier-spinelli]

Hello,

Covariant return type is now available (as of C#9). See here: https://anthonygiretti.com/2020/10/12/introducing-c-9-covariant-returns/

It seems to me that the type safe code (no more need of the "general" object return) can be reintegrated.

[rclabo]

@olivier-spinelli Thank you for contributing that information. I was not previously aware of this new c#9 feature. Pretty cool. Unfortunately, for now I don't believe that we can leverage the features of C#9 since Lucene.Net currently targets netstandard2.0, netstandard2.1 and net45. The language versions these frameworks support are 7.3, 8.0 and 7.3 respectively see target framework language versions It looks like c#9 is only available when targeting .Net 5 currently. At some point in the future, though we may be able to leverage this c#9 feature. It's great to see this addition to the c# language has been recently added.

[NightOwl888]

@rclabo

We have already upgraded the project to C# 9.0, however so far we haven't taken advantage of any of its new features.

@olivier-spinelli

Thanks for pointing out this new feature. It looks like C# is finally catching up with Java in this regard.

So, with that revelation my thought is now to change the return type to match Lucene instead of using the (obsolete) .NET convention for the Clone() method. We keep the cast, but no longer force the end user to contend with casting when cloning. This is a non-breaking change for most projects.

The only question is, do we really need the feature of allowing the end user to custom compile with ICloneable? My vote is no, we don't need it anymore. A comment on why we diverged and not implemented ICloneable in .NET will suffice.

In addition to Clone() there are a lot of classes where the subclass in Java returns a covariant (subclass) of the base type, which we can now do to eliminate even more casts. That being said, it will be hard to find them. Any thoughts on how we might track these down without doing a method by method comparison of virtual/abstract methods?

[eladmarg]

I think we don't need the ICloneable, it was only a patch for the .net port my main concern is if this truly solves the the performance penalty of boxing / unboxing issues behind the scenes.

[NightOwl888]

Great. Without further ado we will remove the ICloneable feature (which was only a non-default option).

However, unfortunately covariant returns are only supported on .NET 5+ target frameworks. I have done a test locally to confirm this is the case.

That said, I am on board with getting rid of casting (by end users) on the Clone() methods in every case where we can at the expense of API consistency.

1. In .NET 5 we will use covariant returns (which will be supported when the target is added to Lucene.NET)
2. In sealed classes, we will return the exact type.
3. In non-sealed classes, we will also return the exact type.
4. In any built-in subclasses, we will return the base class type, which is a step above object, but may still require a cast.

Again, this will probably not break anyone's code, they will just no longer have to cast in some cases. Projects that are upgraded from prior targets of .NET to .NET 5+ will have no casting to do at all with Clone() methods.

All other methods we will have consider on a case by case basis whether supporting covariant returns is worth the cost of maintaining diverging code and diverging APIs, since this feature is only supported on .NET 5 targets.

[rclabo]

@NightOwl888 I think that approach makes good sense. I'm totally onboard with that. That let's the project leverage covariant returns when targeting .Net 5 and gives other targets the best experience they can have. Perfect.

[paulirwin]

@NightOwl888 Can you please review the state of this issue and make a list of what is remaining? Thanks!