Skip to content

Conversation

edgarfgp
Copy link
Contributor

@edgarfgp edgarfgp commented Oct 24, 2023

Fixes #16156 and #14316

@vzarytovskii
Copy link
Member

vzarytovskii commented Oct 31, 2023

Fix can be a simpler one, I think, needs a bunch of testing though, both positive and negative cases (like the case in the linked issues, as well a the opposite - slot is instance, but override is static):

diff --git a/src/Compiler/Checking/CheckExpressions.fs b/src/Compiler/Checking/CheckExpressions.fs
index d232a04e6..11162f71c 100644
--- a/src/Compiler/Checking/CheckExpressions.fs
+++ b/src/Compiler/Checking/CheckExpressions.fs
@@ -7012,7 +7012,7 @@ and TcObjectExpr (cenv: cenv) env tpenv (objTy, realObjTy, argopt, binds, extraI
               let overrides' =
                   [ for overrideMeth in overrides do
                         let overrideInfo, (_, thisVal, methodVars, bindingAttribs, bindingBody) = overrideMeth
-                        let (Override(_, _, id, mtps, _, _, _, isFakeEventProperty, _)) = overrideInfo
+                        let (Override(_, _, id, mtps, _, _, _, isFakeEventProperty, _, _)) = overrideInfo
 
                         if not isFakeEventProperty then
                             let searchForOverride =
diff --git a/src/Compiler/Checking/MethodOverrides.fs b/src/Compiler/Checking/MethodOverrides.fs
index bfa11de4c..221921ac6 100644
--- a/src/Compiler/Checking/MethodOverrides.fs
+++ b/src/Compiler/Checking/MethodOverrides.fs
@@ -44,7 +44,8 @@ type OverrideInfo =
         argTypes: TType list list *
         returnType: TType option *
         isFakeEventProperty: bool *
-        isCompilerGenerated: bool
+        isCompilerGenerated: bool *
+        isInstance: bool
 
     member x.CanImplement = let (Override(canImplement=a)) = x in a
 
@@ -61,6 +62,8 @@ type OverrideInfo =
     member x.ReturnType = let (Override(returnType=b)) = x in b
 
     member x.IsCompilerGenerated = let (Override(isCompilerGenerated=b)) = x in b
+    
+    member x.IsInstance = let (Override(isInstance=b)) = x in b
 
 type RequiredSlot = 
     | RequiredSlot of methodInfo: MethInfo * isOptional: bool
@@ -104,7 +107,7 @@ exception OverrideDoesntOverride of DisplayEnv * OverrideInfo * MethInfo option
 module DispatchSlotChecking =
 
     /// Print the signature of an override to a buffer as part of an error message
-    let PrintOverrideToBuffer denv os (Override(_, _, id, methTypars, memberToParentInst, argTys, retTy, _, _)) = 
+    let PrintOverrideToBuffer denv os (Override(_, _, id, methTypars, memberToParentInst, argTys, retTy, _, _, _)) = 
        let denv = { denv with showTyparBinding = true }
        let retTy = (retTy  |> GetFSharpViewOfReturnType denv.g)
        let argInfos = 
@@ -136,7 +139,7 @@ module DispatchSlotChecking =
         let (CompiledSig (argTys, retTy, fmethTypars, ttpinst)) = CompiledSigOfMeth g amap m minfo
 
         let isFakeEventProperty = minfo.IsFSharpEventPropertyMethod
-        Override(parentType, minfo.ApparentEnclosingTyconRef, mkSynId m nm, fmethTypars, ttpinst, argTys, retTy, isFakeEventProperty, false)
+        Override(parentType, minfo.ApparentEnclosingTyconRef, mkSynId m nm, fmethTypars, ttpinst, argTys, retTy, isFakeEventProperty, false, minfo.IsInstance)
 
     /// Get the override info for a value being used to implement a dispatch slot.
     let GetTypeMemberOverrideInfo g reqdTy (overrideBy: ValRef) = 
@@ -175,7 +178,7 @@ module DispatchSlotChecking =
                 //CanImplementAnySlot  <<----- Change to this to enable implicit interface implementation
 
         let isFakeEventProperty = overrideBy.IsFSharpEventProperty(g)
-        Override(implKind, overrideBy.MemberApparentEntity, mkSynId overrideBy.Range nm, memberMethodTypars, memberToParentInst, argTys, retTy, isFakeEventProperty, overrideBy.IsCompilerGenerated)
+        Override(implKind, overrideBy.MemberApparentEntity, mkSynId overrideBy.Range nm, memberMethodTypars, memberToParentInst, argTys, retTy, isFakeEventProperty, overrideBy.IsCompilerGenerated, overrideBy.IsInstanceMember)
 
     /// Get the override information for an object expression method being used to implement dispatch slots
     let GetObjectExprOverrideInfo g amap (implTy, id: Ident, memberFlags, ty, arityInfo, bindingAttribs, rhsExpr) = 
@@ -200,7 +203,7 @@ module DispatchSlotChecking =
                     CanImplementAnyClassHierarchySlot
                     //CanImplementAnySlot  <<----- Change to this to enable implicit interface implementation
             let isFakeEventProperty = CompileAsEvent g bindingAttribs
-            let overrideByInfo = Override(implKind, tcrefOfAppTy g implTy, id, tps, [], argTys, retTy, isFakeEventProperty, false)
+            let overrideByInfo = Override(implKind, tcrefOfAppTy g implTy, id, tps, [], argTys, retTy, isFakeEventProperty, false, memberFlags.IsInstance)
             overrideByInfo, (baseValOpt, thisv, vs, bindingAttribs, rhsExpr)
         | _ -> 
             error(InternalError("Unexpected shape for object expression override", id.idRange))
@@ -227,7 +230,7 @@ module DispatchSlotChecking =
         
     /// Check if an override is a partial match for the requirements for a dispatch slot except for the name.
     let IsSigPartialMatch g (dispatchSlot: MethInfo) compiledSig overrideBy =
-        let (Override(_, _, _, methTypars, _, argTys, _retTy, _, _)) = overrideBy
+        let (Override(_, _, _, methTypars, _, argTys, _retTy, _, _, _)) = overrideBy
         let (CompiledSig (vargTys, _, fvmethTypars, _)) = compiledSig
         methTypars.Length = fvmethTypars.Length &&
         IsTyparKindMatch compiledSig overrideBy && 
@@ -249,7 +252,7 @@ module DispatchSlotChecking =
      
     /// Check if an override exactly matches the requirements for a dispatch slot except for the name.
     let IsSigExactMatch g amap m dispatchSlot overrideBy =
-        let (Override(_, _, _, methTypars, memberToParentInst, argTys, retTy, _, _)) = overrideBy
+        let (Override(_, _, _, methTypars, memberToParentInst, argTys, retTy, _, _, _)) = overrideBy
         let compiledSig = CompiledSigOfMeth g amap m dispatchSlot
         IsSigPartialMatch g dispatchSlot compiledSig overrideBy &&
         let (CompiledSig (vargTys, vrty, fvmethTypars, ttpinst)) = compiledSig
@@ -300,7 +303,8 @@ module DispatchSlotChecking =
     /// Check if an override exactly matches the requirements for a dispatch slot.
     let IsExactMatch g amap m dispatchSlot overrideBy =
         IsNameMatch dispatchSlot overrideBy &&
-        IsSigExactMatch g amap m dispatchSlot overrideBy
+        IsSigExactMatch g amap m dispatchSlot overrideBy &&
+        dispatchSlot.IsInstance = overrideBy.IsInstance
 
     /// Check if an override implements a dispatch slot 
     let OverrideImplementsDispatchSlot g amap m dispatchSlot availPriorOverride =
@@ -386,7 +390,7 @@ module DispatchSlotChecking =
                             noimpl()
                         | [ overrideBy ] ->
 
-                            let (Override(_, _, _, methTypars, _, argTys, _, _, _)) = overrideBy
+                            let (Override(_, _, _, methTypars, _, argTys, _, _, _, _)) = overrideBy
 
                             let moreThanOnePossibleDispatchSlot =
                                 dispatchSlots
@@ -965,4 +969,3 @@ let GetAbstractPropInfosForSynPropertyDecl(infoReader: InfoReader, ad, memberNam
         
     let dispatchSlots = pinfos |> List.filter (fun pinfo -> pinfo.IsVirtualProperty)
     dispatchSlots
-
diff --git a/src/Compiler/Checking/MethodOverrides.fsi b/src/Compiler/Checking/MethodOverrides.fsi
index d18a8cdc6..ac72aa0ca 100644
--- a/src/Compiler/Checking/MethodOverrides.fsi
+++ b/src/Compiler/Checking/MethodOverrides.fsi
@@ -32,7 +32,8 @@ type OverrideInfo =
         argTypes: TType list list *
         returnType: TType option *
         isFakeEventProperty: bool *
-        isCompilerGenerated: bool
+        isCompilerGenerated: bool *
+        isInstance: bool
 
     member ArgTypes: TType list list
 
@@ -50,6 +51,8 @@ type OverrideInfo =
 
     member ReturnType: TType option
 
+    member IsInstance: bool
+
 type RequiredSlot =
     | RequiredSlot of methodInfo: MethInfo * isOptional: bool
     | DefaultInterfaceImplementationSlot of methodInfo: MethInfo * isOptional: bool * possiblyNoMostSpecific: bool

@vzarytovskii
Copy link
Member

vzarytovskii commented Oct 31, 2023

Maybe also verifying that tooling (service tests) light up the issue too. And maybe do different comparison between two bools, to make sure we don't box.

Maybe also check that it plays well with mixed (static+instance) members on both interface and implementation.

@edgarfgp
Copy link
Contributor Author

edgarfgp commented Oct 31, 2023

Thanks for the detailed analysis, @vzarytovskii. Seems easier fix in my last commit(Lets see what the CI says). I will have look at your implementation suggestion after :)

Edit: Regarding the error message: Reuse FS0855 or create a new error number etc. ?

@vzarytovskii
Copy link
Member

vzarytovskii commented Oct 31, 2023

Thanks for the detailed analysis, @vzarytovskii. Seems easier fix in my last commit(Lets see what the CI says).

It looks like it adds a bunch more. Change in IsExactMatch will also cover object expressions for example (actually one more idea for the test).

I will have look at your implementation suggestion after :)

Edit: Regarding the error message: Reuse FS0855 or create a new error number etc. ?

The change I posted will currently reuse existing error (FS0017), which makes sense, I think, since it's technically not finding the correct override.

So, when we have something like:

type IOperation =
    static abstract member Execute: unit -> unit

type FaultyOperation() =
    interface IOperation with
        member _.Execute() = ()

it will result in

error FS0017: The member 'Execute: unit -> unit' does not have the correct type to override the corresponding abstract method.

Here, when we emit it, we can do additional analysis to see what's not matching (if it's the isInstace flag, then we can produce different warning).

A slight problem might be when we have something like:

type IOperation =
    static abstract member Execute: unit -> unit

type FaultyOperation() =
    interface IOperation with
        member _.Execute() = ()
        static member Execute() = ()

It will still produce FS0017 on the first member, which is technically correct, but probably better to produce FS0855: No abstract or interface member was found that corresponds to this override, which again, probably an additional check somewhere for the match of isInstance.

I can look at both if you'd like (probably tomorrow, slightly under the weather today).

Edit: After thinking about it again, I guess this info carried in the Override type is correct way to go. We can then reuse it in nice printing, in object expressions, etc, without too many changes.

@edgarfgp
Copy link
Contributor Author

@vzarytovskii the Last commit uses your suggestion and adds more tests.

I can look at both if you'd like (probably tomorrow, slightly under the weather today).

Hope you feel better tomorrow, Yes, please. I think we are close to having a fix for this. Thanks in advance

@edgarfgp edgarfgp marked this pull request as ready for review October 31, 2023 16:07
@edgarfgp edgarfgp requested a review from a team as a code owner October 31, 2023 16:07
@vzarytovskii
Copy link
Member

@vzarytovskii the Last commit uses your suggestion and adds more tests.

I can look at both if you'd like (probably tomorrow, slightly under the weather today).

Hope you feel better tomorrow, Yes, please. I think we are close to having a fix for this. Thanks in advance

Cool, do you mind if I add to this PR?

@edgarfgp
Copy link
Contributor Author

Cool, do you mind if I add to this PR?

Not at all. I will be able to say that I worked with you in a fix ;)

@smoothdeveloper
Copy link
Contributor

should the message be either:

FS0855: No abstract or interface instance member was found that corresponds to this override

or

FS0855: No abstract or interface static member was found that corresponds to this override

@vzarytovskii
Copy link
Member

should the message be either:

FS0855: No abstract or interface instance member was found that corresponds to this override

or

FS0855: No abstract or interface static member was found that corresponds to this override

That's why I mentioned why we can adjust it in place we throw/create it.

@vzarytovskii
Copy link
Member

vzarytovskii commented Nov 1, 2023

should the message be either:

FS0855: No abstract or interface instance member was found that corresponds to this override

or

FS0855: No abstract or interface static member was found that corresponds to this override

Thinking about these messages again. These are not overrides we're talking about here. It's method implementations, so FS0017 sounds reasonable with some adjustments to messaging.

Will play with it a little, to see where's the easiest we can produce that message.

@vzarytovskii
Copy link
Member

vzarytovskii commented Nov 1, 2023

Fixed a bunch of stuff, I will probably need some help with figuring out tests, and gaps in the fix.

Now the behaviour is the following (NOTE: all of those are from FSI, and not compiler, so will show only 1st error):

module StaticAbstractBug =
    type IOperation =
        static abstract member Execute: unit -> unit
        abstract member Execute2: unit -> unit
        static abstract member Property: int
        abstract member Property2: int
        static abstract Property3 : int with get, set

    type FaultyOperation() =
        interface IOperation with
            member _.Execute() = ()
            member _.Execute2() = ()
            member this.Property = 0
            member this.Property2 = 0
            member this.Property3 = 0
            member this.Property3 with set value = ()

will yield


              member _.Execute() = ()
  ---------------------^^^^^^^

/Users/u/code/fsharp2/src/stdin(16,22): error FS0855: No abstract or interface member was found that corresponds to this override

module StaticAbstractBug =
    type IOperation =
        static abstract member Execute: unit -> unit
        abstract member Execute: unit -> bool
        static abstract member Property: int
        abstract member Property: int

    type FaultyOperation() =
        interface IOperation with
            member _.Execute() = ()
            member _.Execute() = false
            member this.Property = 0
            member this.Property = false

will yield

             member _.Execute() = ()
  ---------------------------------^^
/Users/u/code/fsharp2/src/stdin(31,34): error FS0001: This expression was expected to have type
    'bool'    
but here has type
    'unit'   

Need a sanity check, that this one is correct. It sees a non-static slot, which has bool as return type, but sees an implementation with unit.


module StaticAbstractBug =
    type IFoo<'T> =
       abstract DoIt: unit -> string
       static abstract Other : int -> int
       static abstract member Property: int
       abstract member Property2: int
       static abstract Property3 : int with get, set
    type MyFoo = {
       Value : int
    } with
      interface IFoo<MyFoo> with
        member me.DoIt() = string me.Value
        member _.Other(value) = value + 1
        member this.Property = 0
        member this.Property2 = 0
        member this.Property3 = 0
        member this.Property3 with set value = ()

yields

          member _.Other(value) = value + 1
  -----------------^^^^^

/Users/u/code/fsharp2/src/stdin(47,18): error FS0855: No abstract or interface member was found that corresponds to this override

I will check the rest and fix tests tomorrow.

@vzarytovskii
Copy link
Member

vzarytovskii commented Nov 1, 2023

Updated tests. Need some sanity check on diagnostics.

@vzarytovskii
Copy link
Member

tl;dr it's for sure not breaking and doesn't (shouldn't) introduce any new/changed messaging to not-static members, but will fix the original issue + give (imo) a bit more precise issues when we have mixed static and instance members in interface and implementation.

@edgarfgp
Copy link
Contributor Author

edgarfgp commented Nov 2, 2023

Thanks. I don't think I did anything with Properties, though, so that might be the gap.
The main change now is pretty much we distinguish not only name, arity and types but also instance-ness.

@vzarytovskii Updated and added more testing, and yeah we are not reporting Properties both in positive and negative tests

Do you want to look into this, or should we create a separate issue to address this (Arguably our fix already fixed the linked issues)?

@vzarytovskii
Copy link
Member

not reporting Properties

How can you give an example of what's expected and what's happening?

@edgarfgp
Copy link
Contributor Author

edgarfgp commented Nov 2, 2023

not reporting Properties

How can you give an example of what's expected and what's happening?

Actual: Only reporting on instance members

Expected: Report on instance members and Properties

    [<Fact>]
    let ``Produce an error for interface with static abstract member that is implemented as instance member`` () =
        Fsx """
module StaticAbstractBug =
    type IFoo<'T> =
       static abstract member Property: int
       abstract member Property2: int
    type MyFoo = {
       Value : int
    } with
      interface IFoo<MyFoo> with
        member this.Property = 0
        member this.Property2 = 0
        """
         |> withOptions [ "--nowarn:3535" ]
         |> withLangVersion80
         |> compile
         |> shouldFail

Actual: Only reporting on instance members

Expected: Report on instance members and Properties

    [<Fact>]
    let ``Produces errors when includes keyword "static" when implementing a non generic interface in a type`` () =
        Fsx """
module StaticAbstractBug =
    type IFoo<'T> =
       abstract member Property: int
    type MyFoo = {
       Value : int
    } with
      interface IFoo<MyFoo> with
        static member Property = 0
        """
         |> withLangVersion80
         |> typecheck
         |> shouldFail
         

@vzarytovskii
Copy link
Member

So, the minimum repro would be:
Interface: have 1 static method, 1 static property
Implementation: have 1 instance method, 1 instance property

We expect both to be reported as missing/non-matching?

@edgarfgp
Copy link
Contributor Author

edgarfgp commented Nov 2, 2023

Ok. I should easy based on the fix. Should be easy to extend and check for the properties using ie :

3856,tcNoStaticPropertyFoundForOverride,"No static abstract property was found that corresponds to this override"

let instanceExpected = memberFlags.IsInstance
if instanceExpected then
    errorR(Error(FSComp.SR.tcNoPropertyFoundForOverride(), memberId.idRange))
else
    errorR (Error(FSComp.SR.tcNoStaticPropertyFoundForOverride (), memberId.idRange))     

@T-Gro
Copy link
Member

T-Gro commented Nov 2, 2023

Before this gets merged, @edgarfgp could you please also rename names used in the code (e.g. keys used in FsComp.txt) to call them implements and not overrides?

@edgarfgp
Copy link
Contributor Author

edgarfgp commented Nov 2, 2023

Before this gets merged, could you please also rename names used in the code (e.g. keys used in FsComp.txt) to call them implements and not overrides?

Sure

@vzarytovskii
Copy link
Member

Ok. I should easy based on the fix. Should be easy to extend and check for the properties using ie :

3856,tcNoStaticPropertyFoundForOverride,"No static abstract property was found that corresponds to this override"

let instanceExpected = memberFlags.IsInstance

if instanceExpected then

    errorR(Error(FSComp.SR.tcNoPropertyFoundForOverride(), memberId.idRange))

else

    errorR (Error(FSComp.SR.tcNoStaticPropertyFoundForOverride (), memberId.idRange))     

It's not only about adding message. It needs a change in logic when searching for properties' slots.

@edgarfgp
Copy link
Contributor Author

edgarfgp commented Nov 2, 2023

It's not only about adding message. It needs a change in logic when searching for properties' slots.

Yeah I figured after giving is try 😅.

@vzarytovskii
Copy link
Member

It's not only about adding message. It needs a change in logic when searching for properties' slots.

Yeah I figured after giving is try 😅.

I'll fix it. Is my previous comment true? Is it only thing we should do?

@edgarfgp
Copy link
Contributor Author

edgarfgp commented Nov 2, 2023

We expect both to be reported as missing/non-matching?

Sorry I missed this . Yea that would be ideal to report in both scenarios

@vzarytovskii
Copy link
Member

I will get back to it tomorrow, if that's ok.

@edgarfgp
Copy link
Contributor Author

edgarfgp commented Nov 6, 2023

I will get back to it tomorrow, if that's ok.

No worries. Thanks for taking the time Vlad

Copy link
Member

@vzarytovskii vzarytovskii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

type check internal error for interface with static abstract member that is implemented as instance member
5 participants