Avoid unnecessary CompilableProvier castings (#84)

[SergeiPavlov]

No description provided.

[alex-kulakov]

Hello, so this is similar to the case in #261, right?

[SergeiPavlov]

Hello, so this is similar to the case in #261, right?

Not exaclty.
The aim of #265 was to remove common part of each Clone() method for simplification.

This PR avoids explicit type casts

[alex-kulakov]

@SergeiPavlov,

With these changes of ProviderVisitor, it is basically the same as CompilableProviderVisitor. No references to ProviderVisitor exist, except for usage of it as base class for CompilableProviderVisitor. This means there is no way to have it as direct base class for something in user code. I'm thinking of merging ProviderVistior and CompilableProviderVisitor.

By the way, some members of CompilableProviderVisitor, for instance, VistitTag, VisitDistinct methods have specific return types instead of general CompilableProvider type. Why? To have advantage of covariant returns we need to call method and work with result directly, without any casts, but I can't see any direct usage of results, moreover, they are casted to CompilableProvider in the Visit method, so cast to base class happens anyway, right?

[SergeiPavlov]

Right now returning TagProvider form VisitTag(TagProvider provider) is unnecessary, but may be useful in future.
Generally returning subtype is more safe.

Casting to base class is compile-time, so it is free.

[alex-kulakov]

But it blocks overriding methods from being able to return something other than provider type of parameter. CompilableProviderVisitor is used in rewriters which may transform tree of providers including change of type of returning instance. If we changed return type of virtual method this opportunity would be closed. It also may be useful for future, and in case of such need of rewriting we would have to change virtual method signature back to base type - CompilableProvider. Not very good for base class. I assume that overriding methods prevented you from declaring all the virtual VisitXxx methods to have exact return types, right?

I would vote for keeping CompilableProvider as return type of virtual methods. I did some benchmarks on dummy providers tree of 200 nodes (only visiting of the tree). Fastest option changes from one to another in some runs but both options are faster than returning Provider.

Method	Mean	Error	StdDev	Rank	Gen 0	Gen 1	Allocated
ReturnCompilableProviders	3.106 us	0.0110 us	0.0092 us	1	1.6899	0.0229	8 KB
RetrunExactProviders	3.139 us	0.0205 us	0.0191 us	1	1.6899	0.0229	8 KB
RerturnProviders	3.367 us	0.0062 us	0.0048 us	2	1.6899	0.0229	8 KB

Method	Mean	Error	StdDev	Rank	Gen 0	Gen 1	Allocated
RetrunExactProviders	3.188 us	0.0302 us	0.0268 us	1	1.6899	0.0229	8 KB
ReturnCompilableProviders	3.272 us	0.0245 us	0.0229 us	2	1.6899	0.0229	8 KB
RerturnProviders	3.501 us	0.0216 us	0.0191 us	3	1.6899	0.0229	8 KB

I believe that tells that returning CompilableProvider and having it across the board is good idea overall.

Also, I'm getting rid of ProviderVisitor as base class of visitors and will make a PR to your branch a bit later today.

[SergeiPavlov]

My guess was that every VisitFoo() method returns the same type as its argument, because it is a kind of filter.
Is this statement wrong?

Ok, I've changed return type to CompilableProvider

[alex-kulakov]

My guess was that every VisitFoo() method returns the same type as its argument, because it is a kind of filter.

This is true but mostly for sealed classes, also might be true for some in-between classes. For instance, ColumnMappingInspector does not change structure of Providers tree, but some others may change the tree, optimize it or shrink it. This is why I vote for methods returning CompilableProvider in ComplilableProviderVisitor, because it does not constraint derived types' methods to have exact return type instead of base type.

This is what I did in my changes to this PR, I agreed with you and made children types take advantage of covariant returns where it is possible.
The PR I did servicetitan#134