-
Notifications
You must be signed in to change notification settings - Fork 789
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes #14613 #14617
Fixes #14613 #14617
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the tests pass it's probably good 👍
@vzarytovskii We could add a tests where the virtual method was hidden ? public class A
{
public virtual void M4(string x) { }
}
public class B : A
{
public new void M4() { }
}
type C =
inherit B()
override this.M4(x: string) = () |
Shouldn't this be covered by the following existing one? M3 in B is hiding the one in A. [<Fact>]
let ``Virtual member was not found that corresponds to this override nested base class with lang version preview`` () =
let CSLib =
CSharp """
public class A
{
public virtual void M1() { }
public virtual void M2() { }
public virtual void M3() { }
public virtual void M4() { }
}
public class B : A
{
public override void M1() { }
public void M2() { }
public new void M3() { }
public new virtual void M4() { }
}
""" |> withName "CSLib"
let app =
FSharp """
module ClassTests
type C() =
inherit B()
override this.M1() = ()
override this.M2() = () // error expected
override this.M3() = () // error expected
override this.M4() = ()
member this.M5() = ()
""" |> withReferences [CSLib]
app
|> withLangVersionPreview
|> compile
|> shouldFail
|> withDiagnostics [
(Error 855, Line 7, Col 19, Line 7, Col 21, "No abstract or interface member was found that corresponds to this override")
(Error 855, Line 8, Col 19, Line 8, Col 21, "No abstract or interface member was found that corresponds to this override")
] |
I think it will but I did not that add a test for the cases when the overload has one or more parameters . But Yeah in theory it will be covered with the previous tests |
I've added couple of tests around overloads. |
I am a bit worried that I've missed some obvious (probably generic-related) overloads resolution for the new path |
class C
{
public virtual void M<T1, T2, T3>(T1? a, T2 b, T1? c, T3? d) {}
}
class D : C
{
public override void M<T1, T2, T3>(T1? a, T2 b, T1? c, T3? d)
where T1 : default
where T3 : default
{
base.M(a, b, c, d);
}
} @vzarytovskii We can add a tests that consumes this generic overload ? |
Sure. What do w expect overload to be in F#? Just same generic method? |
Yes type X =
inherit C
override this.M(a, b, c, d) = () |
I will merge this fix and add more tests in the separate PR |
Fixes #14613 (an issue in preview version of F# which was introduced in #14263)
Adds another
FindMemberFlag
"strategy", which folds over type hierarchy bottom-to-top (from parent up) and discards all members if it detects that a non-virtual member with same signature hides virtual one.@kerams @auduchinok if you have any more tests in mind, I'll gladly add it.
cc @edgarfgp