Skip to content
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

Oddities in statically resolved method constraints and method overloading #3814

Open
RealmPlume opened this issue Oct 25, 2017 · 13 comments
Open
Labels
Area-Compiler-SRTP bugs in SRTP inference, resolution, witness passing, code gen Bug Impact-Medium (Internal MS Team use only) Describes an issue with moderate impact on existing code.
Milestone

Comments

@RealmPlume
Copy link

RealmPlume commented Oct 25, 2017

Case 1

type Test() =
     member __.Equals (_: Test) = true

let inline Equals(a: obj) (b: ^t) =
    match a with
    | :? ^t as x -> (^t: (member Equals: ^t -> bool) (b, x))
    | _-> false

let a: Test = Test()
let b:Test = Test()
//b <- null

printfn "%A" (Equals a b)

I expected to call Test.Equals: Test->unit, (Equals a b) return true
but actually it call Object.Equals: obj->unit, return false

Case 2:

type X =
    static member Method (a: obj) = 1
    static member Method (a: int) = 2
    static member Method (a: int64) = 3


let inline Test< ^t, ^a when ^t: (static member Method: ^a -> int)> (value: ^a) =
    ( ^t: (static member Method: ^a -> int)(value))

let inline Test2< ^t> a = Test<X, ^t> a

printfn "%d" (Test2<int> 0)

I expected return 2, but actually it return 1

@dsyme dsyme changed the title Bug: some case , statically resolved will first match object type Oddities in static method resolution and method overloading Oct 25, 2017
@dsyme dsyme changed the title Oddities in static method resolution and method overloading Oddities in statically resolved method constraints and method overloading Oct 25, 2017
@dsyme
Copy link
Contributor

dsyme commented Oct 29, 2017

@greatim Thanks for the clean report, I will be looking into this closely

@dsyme dsyme added Area-Compiler Impact-Medium (Internal MS Team use only) Describes an issue with moderate impact on existing code. Bug labels Oct 29, 2017
@dsyme dsyme added Ready and removed Needs-RFC labels Nov 17, 2017
@dsyme
Copy link
Contributor

dsyme commented Nov 17, 2017

@greatim Please see #3967 for proposed resolution

@realvictorprm
Copy link
Contributor

In case 2 there's certainly the bug that our type constrains either get lost or are ignored.

@realvictorprm
Copy link
Contributor

@dsyme
@jindraivanek and me noticed that if you change Case 2 to this:

type X =
    static member Method (a: int) = 2
    static member Method (a: int64) = 3


let inline Test< ^t, ^a when ^t: (static member Method: ^a -> int)> (value: ^a) =
    ( ^t: (static member Method: ^a -> int)(value))

let inline Test2< ^t> a = Test<X, ^t> a

printfn "%d" (Test2<int> 0)

Now we get super fancy weird compiler errors:

  let inline Test2< ^t> a = Test<X, ^t> a
  --------------------------^^^^

stdin(9,27): error FS0043: No overloads match for method 'Method'. The available overloads are shown below.
Possible overload: 'static member X.Method : a:int64 -> int'. Type constraint mismatch. The type 
    ''t'    
is not compatible with type
    'int64'    
.
Possible overload: 'static member X.Method : a:int -> int'. Type constraint mismatch. The type 
    ''t'    
is not compatible with type
    'int'    
.

This isn't an oddity @dsyme this is a clear bug.

@realvictorprm
Copy link
Contributor

realvictorprm commented Jun 8, 2018

I do have a fix for this consisting mainly of your code with removing 4 lines. It also would fix Case 1.
Do we want to have a fix for this?

To my understanding the problem lies in that the compiler either

  1. stops searching for a correct overload too early
  2. applies constrains on the type parameters too late so that object is chosen as solution.

Please note that this behaviour is reproducable with my branch of your fix https://github.com/Microsoft/visualfsharp/pull/5141/files

@cartermp cartermp added this to the 16.0 milestone Aug 29, 2018
@realvictorprm
Copy link
Contributor

realvictorprm commented Sep 13, 2018

This is unrelated to this bug.

I was investigating case 1 further and realized that this oddity is realted to overload resolution within inheritance. This example should print the same as case one but uses different code for reproduction:

type FooBase() =
     member __.Foo(_: obj) = false

type Test() =
     inherit FooBase()
     member __.Foo(_: Test) = true

let inline Foo(a: FooBase) (b: ^t) =
    match a with
    | :? ^t as x -> 
       (^t: (member Foo: ^t -> bool) (b, x))
    | x -> 
       printfn "Not what we want"
       false

let a: Test = Test()
let b:Test = Test()
//b <- null

printfn "%A" (Foo a b)

Expecting that this code prints "true" because Foo with Test as type argument would be expected here.

@dsyme Is this information helpful for finding the root cause?

@dsyme
Copy link
Contributor

dsyme commented Sep 13, 2018

@realvictorprm Yes, helpful, thanks. The question isn't the root cause but whether we can change existing behaviour without busting existing code, and if not then what to do about it.

@dsyme
Copy link
Contributor

dsyme commented Oct 27, 2018

From discussion with @realvictorprm

Both of these bugs are really hard to fix without breaking existing code.

  1. Fundamentally some SRTP constraints are being solved "too early" before the type variables in the signatures are fully solved. This means resolutions are reported involving the obj type (either as object/support type or argument type) and inference proceeds on that basis

  2. The first attempted fixes tried to delay resolution however that broke too much code. Attempts to moderate by delaying only "strong" resolution and proceeding on the old basis for "weak" resolution also didn't help, existing code still broke

One possible way to make progress is as follows:

  1. First, add lots of test cases. We have done some in Additional comments and testing related to SRTP resolution #3967 but we really need to add all of FSharpPlus and the other test cases reported by @gusty.

  2. Second, we could first aim to report warning in the cases where the above behavior is happening. In particular we would store-aside any constraints where the arguments involved unresolved variable types, and resolution proceeded. We would then re-check the resolution of these constraints in PostInferenceChecks.fs. If the resolution of the constraints differed we would report a warning.

  3. With that in place we could consider a /langfix language update for this case. However that may still be too intrusive, we need to have enough test cases to determine this.

@dsyme
Copy link
Contributor

dsyme commented Oct 27, 2018

Notes for @realvictorprm to add tests that would grab and build a known commit of FSharpPlus as part of the test process

In tests\fsharp\tet-frramework.fs

module Commands = 
    let dotnet workDir exec (dotNetExe: FilePath) flags srcFiles =
        let args = (sprintf "%s %s" flags (srcFiles |> Seq.ofList |> String.concat " "))

        ignore workDir 
        exec dotNetExe args
let dotnet cfg arg = Printf.ksprintf (Commands.dotnet cfg.Directory (exec cfg) cfg.DotNetExe) arg

And in tests\fsharp\tests.fs:

    [<Test>]
    let testFSharpPlusBuild () = 

        let cfg = testConfig "repos"

        git cfg "clone http://github.com/fsprojects/FSharpPlus f309032020892r30 repodir"
        
        let cfg2 = testConfig "repos/repodir"

        dotnet cfg2 "build" [ "FSharpPlus.sln" ]

@realvictorprm
Copy link
Contributor

Hi @dsyme I'll work today and tomorrow many hours on this. From my recent work I need to report, that right now there isn't a correct dotnet command implemented neither it's easy to implement it such that the new compiler build will be picked up at least from a slack talk with @cartermp:
Me:

Would it be best to open a PR with my changes and then discuss which steps are required to acquire a dotnet.exe which uses the freshly build fsharp compiler?

Phillip Carter:

It's very tricky, since it requires a build of the CLI itself to work. I recommend using something from FSharpPlus as source as the test case(s)

Relying on this comment I'll work on creating a build script picking the source files of FSharpPlus and invoke the compiler directly on it instead of using the sln file to build the project.

@realvictorprm
Copy link
Contributor

OK another case of trying to delay overload resolution. The case 2 can be "workedaround" through using this code:

type X =
    static member Method (a: int) = 2
    static member Method (a: int64) = 3


let inline Test< ^t, ^a when (^t or ^a): (static member Method: ^a -> int)> (value: ^a) =
    ( (^t or ^a): (static member Method: ^a -> int)(value))
    
let inline Test2< ^t when (X or  ^t) : (static member Method :  ^t -> int)> a = Test<X, ^t> a

printfn "%d" (Test2<int> 0)

@cartermp cartermp modified the milestones: 16.0, Core 3.0 Feb 20, 2019
@cartermp cartermp modified the milestones: .NET Core 3.0, Backlog Jun 17, 2019
@dsyme dsyme added Area-Compiler-SRTP bugs in SRTP inference, resolution, witness passing, code gen and removed Ready Area-Compiler labels Mar 31, 2022
@vzarytovskii vzarytovskii moved this to Not Planned in F# Compiler and Tooling Jun 17, 2022
@vzarytovskii vzarytovskii reopened this Jan 5, 2024
@github-project-automation github-project-automation bot moved this from Done to In Progress in F# Compiler and Tooling Jan 5, 2024
@ingted
Copy link
Contributor

ingted commented Jan 29, 2024

Welcome to F# Interactive for .NET Core in Visual Studio. To execute code, either
  1. Use 'Send to Interactive' (Alt-Enter or right-click) from an F# script. The F# Interactive process will
     use any global.json settings associated with that script.
  2. Press 'Enter' to start. The F# Interactive process will use default settings.
> 
Microsoft (R) F# Interactive version 12.8.0.0 for F# 8.0
Copyright (c) Microsoft Corporation. All Rights Reserved.

For help type #help;;

> val it: unit = ()

> type X =
    static member Method (a: int) = 2
    static member Method (a: int64) = 3


let inline Test< ^t, ^a when ^t: (static member Method: ^a -> int)> (value: ^a) =
    ( ^t: (static member Method: ^a -> int)(value))

let inline Test2< ^t> a = Test<X, ^t> a

printfn "%d" (Test2<int> 0);;

  let inline Test2< ^t> a = Test<X, ^t> a
  --------------------------^^^^

stdin(10,27): error FS0043: No overloads match for method 'Method'.

Known return type: int

Known type parameter: < ^t >

Available overloads:
 - static member X.Method: a: int -> int // Argument 'a' doesn't match
 - static member X.Method: a: int64 -> int // Argument 'a' doesn't match

> 

Still not workable...

@vzarytovskii
Copy link
Member

Welcome to F# Interactive for .NET Core in Visual Studio. To execute code, either
  1. Use 'Send to Interactive' (Alt-Enter or right-click) from an F# script. The F# Interactive process will
     use any global.json settings associated with that script.
  2. Press 'Enter' to start. The F# Interactive process will use default settings.
> 
Microsoft (R) F# Interactive version 12.8.0.0 for F# 8.0
Copyright (c) Microsoft Corporation. All Rights Reserved.

For help type #help;;

> val it: unit = ()

> type X =
    static member Method (a: int) = 2
    static member Method (a: int64) = 3


let inline Test< ^t, ^a when ^t: (static member Method: ^a -> int)> (value: ^a) =
    ( ^t: (static member Method: ^a -> int)(value))

let inline Test2< ^t> a = Test<X, ^t> a

printfn "%d" (Test2<int> 0);;

  let inline Test2< ^t> a = Test<X, ^t> a
  --------------------------^^^^

stdin(10,27): error FS0043: No overloads match for method 'Method'.

Known return type: int

Known type parameter: < ^t >

Available overloads:
 - static member X.Method: a: int -> int // Argument 'a' doesn't match
 - static member X.Method: a: int64 -> int // Argument 'a' doesn't match

> 

Still not workable...

Nothing has been done in this area to my knowledge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compiler-SRTP bugs in SRTP inference, resolution, witness passing, code gen Bug Impact-Medium (Internal MS Team use only) Describes an issue with moderate impact on existing code.
Projects
Archived in project
Development

No branches or pull requests

7 participants