Fix #5531

[realvictorprm]

This is a working fix for the example of #5531.
However there are still some points open before I think it's a good idea to merge this:

• Discuss the change of this proposal, is it OK to prefer overrides of methods and so ignore base implementations?
• Extend the check to the complete inheritance graph instead of a single "look-back"
• Only ignore a methinfo if the signature of both match (so respect different overloads)

@dsyme It would be great if you could have a look whether this check is allowed at this location or should appear earlier.

[realvictorprm]

OK as soon as the CI passes this is ready for a review.

[realvictorprm]

Ok this is ready! :)

[dsyme]

I looked at the change and to be honest I don't really know what rule it's implementing.

Also, there are no tests added so it's hard to assess what's being changed and whether we are correctly testing it's impact.

[dsyme]

I think you want to be making appropriate use of ExcludeHiddenOfMethInfos: https://github.com/Microsoft/visualfsharp/blob/master/src/fsharp/InfoReader.fs#L634

It could be we are missing a call to this during SRTP resolution.

[realvictorprm]

OK, I'll add more comments to the code.

The tests would have been the next step as soon as the change itself is the right approach.

The changes filter out any members which are base implementations (parent) of the given member implementation from the derived class (child).

[realvictorprm]

@dsyme thanks for the pointer. Exactly such is what I expected to find in the compiler somewhere but didn't :)

[realvictorprm]

I'll find out where this call can be added to apply the same effect as my current code + add tests for the change.

[dsyme]

You can see it is applied here but not applied here. I'd imagine applying it on the second case would do the trick.

[realvictorprm]

OK I tried your tip but it looks like the method isn't filtering out the methods correctly. Could it be that the method implementation is wrong?

[dsyme]

OK I tried your tip but it looks like the method isn't filtering out the methods correctly. Could it be that the method implementation is wrong?

No, I'm fairly certain it's not wrong, you can test it in normal code (x.Method resolution not SRTP resolution), and the repro in the bug actually tests it that way I believe

Just go through and work out why the filtering is not being applied?

[realvictorprm]

@dsyme OK I found out that the big problem here is that in the example:

type Base() =
    abstract member Foo : unit -> unit
    default this.Foo() = printfn "Base"
    
type Derived() =
    inherit Base()
    member this.Foo() = printfn "Derived"
    
let inline callFoo< ^T when ^T : (member Foo: unit -> unit) > (t: ^T) =
    (^T : (member Foo: unit -> unit) (t))
    
let b = Base()
let d = Derived()
let bd = d :> Base

b.Foo()
bd.Foo()
d.Foo()

callFoo<Base> b
callFoo<Base> bd
callFoo<Base> d
// A unique overload for method 'Foo' could not be determined based on type information prior to this program point.
// A type annotation may be needed. Candidates:
//   abstract member Base.Foo : unit -> unit,
//   member Derived.Foo : unit -> unit
callFoo<Derived> d

the members overriding a base implementation should definitely be written with "override" instead.

[realvictorprm]

@dsyme the problem is that the non explicit override isn't saved anywhere neither it's possible at that location to check for such. As we're displaying warnings that an implementation isn't specified with override we can save this information back in an extra field stating that this member is a non explicit override.

[realvictorprm]

I've applied the new approach with which the bug is resolved + it's saved back that a member non explicitly overrides a member.

[realvictorprm]

Unfortunately I discovered, that the current approach isn't working. I guess something with my test setup yesterday went wrong.

[realvictorprm]

So this "new" approach is simply using the filter predicate from the warning that a member implementation is hidden by another implementation. In my understanding the ExcludeHiddenOfMethInfos isn't working out as it isn't comparing each element with each other as required to find out whether a method is the "hidden" implementation (as there could be the case that "hiding" happens multiple times).

[realvictorprm]

@dotnet-bot test this please

[realvictorprm]

OK this fix is working too however if I try this example:

type Base() =
    abstract member Foo : unit -> unit
    default this.Foo() = printfn "Base"
    
type Derived() =
    inherit Base()
    member this.Foo() = printfn "Derived"
    
type DerivedDerived() =
    inherit Derived()
    member this.Foo() = printfn "DerivedDerived"
    
let inline callFoo< ^T when ^T : (member Foo: unit -> unit) > (t: ^T) =
    (^T : (member Foo: unit -> unit) (t))
    
let b = Base()
let d = Derived()
let dd = DerivedDerived()
let bd = d :> Base
callFoo<Derived> d
callFoo<DerivedDerived> dd

The last line has conflict again, is this OK or shouldn't this be covered too? (Also there isn't any warning that the method of DerivedDerived is hidding the method of Derived).
If so I would return to my initial approach hard-checking inheritance as it worked without breaking the teast and for all the examples (trying to mirror long inheritance hierarchies).

cc @dsyme

PS: Tests come as soon as we've decided on which fix is OK.

[realvictorprm]

Can someone help me identifying what the problem with my new test case is? The error the CI gives is confusing

[realvictorprm]

The ci_build3 fails with this:
error FS2014: A problem occurred writing the binary 'D:\a\1\s\tests\fsharp\regression\5531\test.exe': Error in pass3 for type Test, error: Error in pass3 for type Base, error: Could not load file or assembly 'Microsoft.Build.Tasks.v4.0, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'. The system cannot find the file specified. which makes no sense to me.
@cartermp @KevinRansom do you know more about such failures? I just need a hint so I can fix this

[cartermp]

This is an internal error in the typechecker, likely because you've caused it to hit an illegal state. There is a warning in CI that may be a hint:

est.fs(7,17): warning FS0864: This new member hides the abstract member 'abstract member Base.Foo : unit -> unit'. Rename the member or use 'override' instead.

[KevinRansom]

@realvictorprm so the test is failing to build with the coreclr. It is not a problem with the fix, it's just that the test case isn't correct for coreclr. To build and run tests on coreclr and desktop use SingleTestBuildAndRun. That doesn't really support the diffing, so probably the best thing to do is to add the test within a #if !FSHARP_SUITE_DRIVES_CORECLR_TESTS block and hope we rewrite these test sensibly.

Kevin

[realvictorprm]

Thank you very much!
That makes more sense. I'll apply your suggestion tomorrow and then ping again as soon as the CI passes :)

[realvictorprm]

OK done, CI passes. This is ready again @KevinRansom :)

[KevinRansom]

Thank you for this contribution

Kevin