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

Discussion: Improve F# to C# interoperability #2503

Closed
smoothdeveloper opened this issue Feb 27, 2017 · 11 comments
Closed

Discussion: Improve F# to C# interoperability #2503

smoothdeveloper opened this issue Feb 27, 2017 · 11 comments

Comments

@smoothdeveloper
Copy link
Contributor

smoothdeveloper commented Feb 27, 2017

Issue with overloads and lambdas

I'm trying to make this same call: https://github.com/picoe/Eto/blob/develop/Samples/Tutorials/Tutorial4/Main.cs#L55

textBox.TextBinding.BindDataContext<MyObject>(
    r => r.TextProperty
    , (r, val) => r.TextProperty = val
);

I can write this easily in C#.

To get there in F#, this is what I would initially get working (after trying the same thing as C# code):

textBox.TextBinding.BindDataContext<_>(
            Func<MyObject,_>(fun (r) -> r.TextProperty)
            , Action<MyObject,_>(fun (r) value -> r.TextProperty <- value)
        ) |> ignore

With help of more experienced F# user I attempted this:

textBox.TextBinding.BindDataContext<MyObject>(
    fun (r : MyObject) -> r.TextProperty
    , fun (r: MyObject) value -> r.TextProperty <- value
) |> ignore

which fails due to tuple mishaps (which is easy to troubleshoot), finally settled to

textBox.TextBinding.BindDataContext<_>(
    (fun (r : MyObject) -> r.TextProperty)
    , (fun (r: MyObject) value -> r.TextProperty <- value)
) |> ignore

Is there something we can do to make C# idiomatic and FP oriented code easy to consume from F#?

This is guaranteed to annoy C# developer trying out to call their nice FP enabled API from F#, at least in cases where overload resolution or type inference significantly differs from C#/VB.NET.

Related

@vivainio
Copy link

Naive suggestion without knowing all the type inference details: seems like a function that takes a delegate, when passed a lambda, could infer the types of the lambda arguments (and expected return value) from the generic parameters of the delegate

@smoothdeveloper
Copy link
Contributor Author

Adding some info for that specific use case, this is what is defined in Eto.Forms:

DualBinding<TValue> BindDataContext(IndirectBinding<TValue> dataContextBinding, DualBindingMode mode = DualBindingMode.TwoWay, TValue defaultControlValue = default(TValue), TValue defaultContextValue = default(TValue))
DualBinding<TValue> BindDataContext(string propertyName, DualBindingMode mode = DualBindingMode.TwoWay)
DualBinding<TValue> BindDataContext<TObject>(Expression<Func<TObject, TValue>> propertyExpression, DualBindingMode mode = DualBindingMode.TwoWay)
DualBinding<TValue> BindDataContext<TObject>(Func<TObject, TValue> getValue, Action<TObject, TValue> setValue, Action<TObject, EventHandler<EventArgs>> addChangeEvent = null, Action<TObject, EventHandler<EventArgs>> removeChangeEvent = null, DualBindingMode mode = DualBindingMode.TwoWay, TValue defaultGetValue = default(TValue), TValue defaultSetValue = default(TValue))

(Looking at the signatures, this gives back some faith in F# 🙂)

I can understand that the compiler has work picking the correct overload, although C# handles it gracefully with less information.

The issue I intend to discuss is what can we do in F# to smoothen the interop with Func / Action / delegates as lambda and make the F# code look closer to the idiomatic C# call?

@dsyme
Copy link
Contributor

dsyme commented Feb 27, 2017

Can you do this?

textBox.TextBinding.BindDataContext<MyObject>(
    (fun r -> r.TextProperty), (fun r value -> r.TextProperty <- value)
) |> ignore

If so this boils down to existing suggestions to allow fun x -> y to be x => y with different precedence (i.e. no parentheses)?

@dsyme
Copy link
Contributor

dsyme commented Feb 27, 2017

(Note language design should really be discussed in fslang-suggestions, slack, stackoverflow etc please, though some discussion threads are used in this repo)

@smoothdeveloper
Copy link
Contributor Author

@dsyme I can't do that, it was my initial attempt, it fails to infer MyObject on .TextProperty

Lookup on object of indeterminate type based on information prior to this program point. A type annotation may be needed prior to this program point to constrain the type of the object. This may allow the lookup to be resolved.

For both call sites.

Sorry I'm using this repository as I intended to have discussion before we can identify if a plausible language suggestion can be drafted.

@ReedCopsey
Copy link
Contributor

ReedCopsey commented Feb 27, 2017

@dsyme That works (even without specifying ) in a simple case - I'm not sure if it's one of the overloads, or the optional arguments, that breaks it.

For example, this works fine:

open System

type Test<'b> () =
    member __.BindDataContext<'a> (getValue : Func<'a,'b>, setValue : Action<'a,'b>) = ()
    member __.BindDataContext (setValue : 'b) = ()
    
type R = { mutable Foo : string }
let r = { Foo = "Foo" }
let t = Test<string> ()

t.BindDataContext( (fun f -> f.Foo), (fun f v -> f.Foo <- v) )

@smoothdeveloper
Copy link
Contributor Author

I confirm the case I'm facing is a bit ill due to presence of complex overloads, but I've seen similar cases where lambda syntax won't work as I'd expect (with C# background), requiring more annotations.

It would be great if we could identify some ways to make inference and lambda work in more cases (when overload resolution is involved), if we have good certainty this won't be breaking change, which I understand this could be tricky.

In this case, we have only 2 overloads with a generic parameter, and only one can accept 2 delegates, so we might be able to get it work in this specific case.

@smoothdeveloper
Copy link
Contributor Author

@dsyme regarding the extra parens around fun x -> y, I don't consider it a big issue (although for C# users, this is a bit odd), I guess it would be breaking change if there was an overload taking a Tuple<Func<TObject,TValue>, Action<TObject,TValue>> so I don't think it is needed.

I'll search the suggestion and put a 👎 on it 😄

@cartermp
Copy link
Contributor

cartermp commented Dec 6, 2017

Closing old discussion

@cartermp cartermp closed this as completed Dec 6, 2017
@smoothdeveloper
Copy link
Contributor Author

Another head scratching overload resolution situation:

open System
open System.Threading.Tasks
let makeTask () = new Task<_>(fun () -> 1)
let task = makeTask()
//let runningTask = Task.Run(fun () -> task) // can't find overload
//let runningTask = Task.Run(Func<_>(fun () -> task)) // can't find overload
//let runningTask = Task.Run<_>(Func<_>(fun () -> task)) // can't find overload
//let runningTask : Task<_> = Task.Run<_>(Func<_>(fun () -> task)) // can't find overload
//let runningTask = Task.Run(Func<Task<_>>(fun () -> task)) // can't find overload
//let runningTask = Task.Run(Func<Task<int>>(fun () -> task)) // can't find overload
//let runningTask = Task.Run<int>(Func<Task<int>>(fun () -> task)) // can find overload!
//let runningTask = Task.Run<int>(Func<Task<_>>(fun () -> task)) // can find overload!
//let runningTask = Task.Run<int>(Func<_>(fun () -> task)) // can find overload!
let runningTask = Task.Run<int>(fun () -> task) // can find overload!

there is no way to use type inference to omit the return type of the task in this example.

@smoothdeveloper
Copy link
Contributor Author

The equivalent C#:

using System;
using System.Threading.Tasks;
Task<int> makeTask => new Task<int>(() => 1);
var task = makeTask();
var runningTask = Task.Run(() => task);

I got the code checking on first attempt and without annotating the call to Task.Run.

@dsyme is there any plan to make the overload resolution work better in such cases? Many C# APIs rely on similar overloads and calling it from F# is a tedious exercise in fighting with the compiler to make it pick the correct one.

smoothdeveloper added a commit to smoothdeveloper/visualfsharp that referenced this issue Apr 25, 2019
…, almost like I want in VS2019, this is for the simple non generic method overload case.

I want to check if user experience changes wrt dotnet#2503 and put some time to add tests.
smoothdeveloper added a commit to smoothdeveloper/visualfsharp that referenced this issue Apr 30, 2019
…, almost like I want in VS2019, this is for the simple non generic method overload case.

I want to check if user experience changes wrt dotnet#2503 and put some time to add tests.
smoothdeveloper added a commit to smoothdeveloper/visualfsharp that referenced this issue May 16, 2019
…, almost like I want in VS2019, this is for the simple non generic method overload case.

I want to check if user experience changes wrt dotnet#2503 and put some time to add tests.
smoothdeveloper added a commit to smoothdeveloper/visualfsharp that referenced this issue Aug 26, 2019
…, almost like I want in VS2019, this is for the simple non generic method overload case.

I want to check if user experience changes wrt dotnet#2503 and put some time to add tests.
smoothdeveloper added a commit to smoothdeveloper/visualfsharp that referenced this issue Aug 27, 2019
…, almost like I want in VS2019, this is for the simple non generic method overload case.

I want to check if user experience changes wrt dotnet#2503 and put some time to add tests.
smoothdeveloper added a commit to smoothdeveloper/visualfsharp that referenced this issue Aug 29, 2019
…, almost like I want in VS2019, this is for the simple non generic method overload case.

I want to check if user experience changes wrt dotnet#2503 and put some time to add tests.
smoothdeveloper added a commit to smoothdeveloper/visualfsharp that referenced this issue Sep 20, 2019
…, almost like I want in VS2019, this is for the simple non generic method overload case.

I want to check if user experience changes wrt dotnet#2503 and put some time to add tests.
smoothdeveloper added a commit to smoothdeveloper/visualfsharp that referenced this issue Dec 15, 2019
…, almost like I want in VS2019, this is for the simple non generic method overload case.

I want to check if user experience changes wrt dotnet#2503 and put some time to add tests.
smoothdeveloper added a commit to smoothdeveloper/visualfsharp that referenced this issue Dec 27, 2019
…, almost like I want in VS2019, this is for the simple non generic method overload case.

I want to check if user experience changes wrt dotnet#2503 and put some time to add tests.
smoothdeveloper added a commit to smoothdeveloper/visualfsharp that referenced this issue Jan 13, 2020
…, almost like I want in VS2019, this is for the simple non generic method overload case.

I want to check if user experience changes wrt dotnet#2503 and put some time to add tests.
smoothdeveloper added a commit to smoothdeveloper/visualfsharp that referenced this issue Jan 16, 2020
…, almost like I want in VS2019, this is for the simple non generic method overload case.

I want to check if user experience changes wrt dotnet#2503 and put some time to add tests.
smoothdeveloper added a commit to smoothdeveloper/visualfsharp that referenced this issue Jan 19, 2020
…, almost like I want in VS2019, this is for the simple non generic method overload case.

I want to check if user experience changes wrt dotnet#2503 and put some time to add tests.
smoothdeveloper added a commit to smoothdeveloper/visualfsharp that referenced this issue Jan 22, 2020
…, almost like I want in VS2019, this is for the simple non generic method overload case.

I want to check if user experience changes wrt dotnet#2503 and put some time to add tests.
smoothdeveloper added a commit to smoothdeveloper/visualfsharp that referenced this issue Jan 22, 2020
…, almost like I want in VS2019, this is for the simple non generic method overload case.

I want to check if user experience changes wrt dotnet#2503 and put some time to add tests.
smoothdeveloper added a commit to smoothdeveloper/visualfsharp that referenced this issue Jan 22, 2020
…, almost like I want in VS2019, this is for the simple non generic method overload case.

I want to check if user experience changes wrt dotnet#2503 and put some time to add tests.
smoothdeveloper added a commit to smoothdeveloper/visualfsharp that referenced this issue Jan 31, 2020
…, almost like I want in VS2019, this is for the simple non generic method overload case.

I want to check if user experience changes wrt dotnet#2503 and put some time to add tests.
smoothdeveloper added a commit to smoothdeveloper/visualfsharp that referenced this issue Feb 15, 2020
…, almost like I want in VS2019, this is for the simple non generic method overload case.

I want to check if user experience changes wrt dotnet#2503 and put some time to add tests.
KevinRansom pushed a commit that referenced this issue Feb 19, 2020
* migrate (as a separate copy) fsharpqa tests that are going to be impacted by #6596

the baseline files are empty on purpose, expecting CI failure reporting those tests, I intend to update the baseline and clean up the comments / xml tags from fsharpqa syntax in later commit, and then remove those specific tests altogether from fsharpqa if this is OK with the maintainers.

* update the .bsl files

* remove comments that are handled by baseline files, update baseline files

* remove the migrated tests from fsharpqa tests

* need to be more careful when migrating those

* testing if running the test with .fs instead of .fsx makes them work on .netcore

* exclude migrated fsharpqa from dotnet core run

* sample test in fsharpqa (can't run locally for now)

* trying to make it green now.

* checking if this path is covered by a test, trying to identify how to trigger this other overload related error message

* * [MethodCalls.fs] Defining CallerArgs<'T> in, this replaces passing of callerArgsCount, uncurriedCallerArgs and other variants in the overload resolution logic happening in ConstraintSolver & TypeChecker
* [TypeChecker.fs] pass CallerArgs instace at call sites for overload resolution + some commented code as we'll be building a list of given argument types (probably moving as a CallerArgs method or property)
* [ConstraintSolver.fs/fsi] pipe the overload resolution traced callback to `trace.CollectThenUndoOrCommit` as that expression is long and more important in that context

* bit of refactoring of error message building logic during failed overload resolution

[ConstraintSolver.fsi]

* define OverloadInformation and OverloadResolutionFailure, those replace workflow where `string * ((CalledMeth<_> * exn) list)` values were created at call site mixed with message building and matched later in non obvious fashion in `failOverloading` closure
* adjust UnresolvedOverloading and PossibleOverload exceptions

[ConstraintSolver.fs]

* get rid of `GetPossibleOverloads`, this was used only in `failOverloading` closure, and just being passed the same parameters at call site
* give `failOverloading` a more meaningful signature and consolidate the prefix message building logic there
* fix `failOverloading` call sites

[CompilerOps.fs] adjust pattern matching

Expecting behaviour of this code to be identical as before PR

`

* (buildfix) harmonizing .fsi/.fs right before commit doesn't always work with simple copy paste.

* trying to check what kind of things break loose when I change this

I'm trying to identify why we get `(CalledMeth<Expr> * exn list)`, I'm making a cartesian product through getMethodSlotsAndErrors for now, trying to figure out if we can get other than 0 or 1 exception in the list for a given method slot.

I may revert that one if it doesn't make sense make from a reviewer standpoint and/or breaks fsharpqa tests surounding overload resolution error messages.

* (minor) [ConstraintSolver.fs] revert space mishapps to reduce diff, put back comments where they were

* toward displaying the argument names properly (may fail some fsharpqa tests for now)

* missing Resharper's Ctrl+Alt+V "introduce variable" refactoring @auduchinok :)

* pretty print unresolved overloads without all the type submsumption per overload verbose messages

* Overload resolution error messages: things display like I want in fsi, almost like I want in VS2019, this is for the simple non generic method overload case.

I want to check if user experience changes wrt #2503 and put some time to add tests.

* adjust message for candidates overload

* Hijack the split phase for UnresolvedOverloading, remove the match on PossibleOverload.

It consolidates some more of the string building logic, and it now shows as a single compact and exhaustive error message in VS

Thinking I'll change PossibleOverload to not be an exception if going this way is OK

* updating existing failing baseline files that looks correct to me

* quickfix for update.base.line.with.actuals.fsx so that it skips missing base line files in the loop

* (minor) minimize diff, typos, comments

* fix vsintegration tests affected by overload error message changes

* merge issue unused variable warning

* update the 12 fsharpqa migrated baseline with new error messages

* (minor) baseline update script

[update.base.line.with.actuals.fsx]
* add few directories
* error handling (when tests are running, the .err files are locked

* move System.Convert.ToString and System.Threading.Tasks.Task.Run tests from QA

* * moving 3 fsharpqa tests to new test suite
* bring back neg_System.Convert.ToString.OverloadList.fsx which I mistakenly removed during merge

* consolidate all string building logic in CompileOps.fs, fix remaining cases with missing \n (the end result code is less whack-a-mole-ish)

* update base lines

* remove the migrated tests from fsharpqa

* fix vstest error message

* fix env.lst, removed wrong one...

* update baselines of remaining tests

* adding one simple test with many overloads

* appropriate /// comments on `CalledMeth` constructor arguments

* minimize diff / formatting

* trim unused code

* add simple test, one message is interesting, mentioning parameter count for possible overloads

* comment flaky test for now

* code review on the `coerceExpr` function from @TIHan, we can discard first `isOutArg` argument as the only hardcoded usage was guarded by `error` when the value is true, and passing an explicit false which is now guaranteed equivalent to `callerArg.IsOptional`

* code formatting remarks on ConstraintSolver.fs/fsi

* (minor) formatting

* format known argument type / updating .bsl

* update missing baseline

* update missing baselines

* update missing baseline

* minor: make TTrait better in debugger display

* [wip] pass TraitConstraintInfo around failed overload resolution error, and try to display some of it's info

* minimize diff

* signature file mismatch

* removing duplicate in fscomp.txt

* surfacing initial bits of TraitConstraintInfo, roughly for now

* formatting types of known argument in one go, the formatting is still wrong:

* unamed type arguments aren't relabelled (still show their numerical ids)
* SRTP constraints are dismissed, not sure how they should be lined up to pass to SimplifyTypes.CollectInfo

* rework of overload failure message to prettify *ALL* types in a single go, not only argument types, but return types and known generic argument types (this info was never displayed originally but is useful for scenario similar to FSharpPlus implementation)

added `PrettifyDiscriminantAndTypePairs` in TastOps, this allow to weave extra information with the complete list of types, to help reconstituting the several types from their origin (in the usage spot: argument types, return type, generic parameter types)

* fixup the tests and add two tests

* one checking on the new information of known return type and generic parameter types
* one checking we don't leak internal unammed Typar and that they don't all get named 'a despite being different

* updating baselines that got tighter.

* simplify handling of TraitConstraintInfo

* naming couple of fields and turning a property to a methods as it triggers an assert in debugger when inspecting variables

* comments in the assembling of overload resolution error message

* Add information about which argument doesn't match
Add couple of tests
Updating bunch of baselines

* minor updates to testguide and devguide

* fix PrimitiveConstraints.``Invalid object constructor`` test

* fix(?) salsa tests

* minimize diff

* put back tests under !FSHARP_SUITE_DRIVES_CORECLR_TESTS

* missing updated message

* minor adjustments to TESTGUIDE.md

* return type was missing prettifying in prettyLayoutsOfUnresolvedOverloading

* address code review nits / minimize diff / add comment on PrettifyDiscriminantAndTypePairs

* minimize diff

* proposed work around the flaky error message until #6725 has a fix

we keep the fsharpqa test around (but removing the overload error messages from what is asserted out of it) in the meantime

* fixing baselines of new tests from master

* sisyphus round of baseline update

* removing type which isn't in use and popped back up after rebase

* minimize diff

* tidy inconsistent tuple literal

* * removing TTrait properties that end up not being used
* renaming tys and returnTy fields to better match convention (`tys` is used, so no underscore prefix)
* minimizing diff

* minimize diff

* minimize diff

* minimize diff

* link to usage example in same file

* revert converting CallerArg single cased DU into Record

* minimize diff

* minimize diff

* minimize diff

* fix rebase glitches

* fix rebase glitch

* update baseline

* fix base lines / new tests base lines

* edge case: needs a new line after "A unique overload for method '%s' could not be determined based on type information prior to this program point. A type annotation may be needed." if there are no optional parts.

* updating base line for edge case of missing new line

* missing baseline

* removing comment
baronfel pushed a commit to baronfel/FSharp.Compiler.Service that referenced this issue Feb 19, 2020
* migrate (as a separate copy) fsharpqa tests that are going to be impacted by #6596

the baseline files are empty on purpose, expecting CI failure reporting those tests, I intend to update the baseline and clean up the comments / xml tags from fsharpqa syntax in later commit, and then remove those specific tests altogether from fsharpqa if this is OK with the maintainers.

* update the .bsl files

* remove comments that are handled by baseline files, update baseline files

* remove the migrated tests from fsharpqa tests

* need to be more careful when migrating those

* testing if running the test with .fs instead of .fsx makes them work on .netcore

* exclude migrated fsharpqa from dotnet core run

* sample test in fsharpqa (can't run locally for now)

* trying to make it green now.

* checking if this path is covered by a test, trying to identify how to trigger this other overload related error message

* * [MethodCalls.fs] Defining CallerArgs<'T> in, this replaces passing of callerArgsCount, uncurriedCallerArgs and other variants in the overload resolution logic happening in ConstraintSolver & TypeChecker
* [TypeChecker.fs] pass CallerArgs instace at call sites for overload resolution + some commented code as we'll be building a list of given argument types (probably moving as a CallerArgs method or property)
* [ConstraintSolver.fs/fsi] pipe the overload resolution traced callback to `trace.CollectThenUndoOrCommit` as that expression is long and more important in that context

* bit of refactoring of error message building logic during failed overload resolution

[ConstraintSolver.fsi]

* define OverloadInformation and OverloadResolutionFailure, those replace workflow where `string * ((CalledMeth<_> * exn) list)` values were created at call site mixed with message building and matched later in non obvious fashion in `failOverloading` closure
* adjust UnresolvedOverloading and PossibleOverload exceptions

[ConstraintSolver.fs]

* get rid of `GetPossibleOverloads`, this was used only in `failOverloading` closure, and just being passed the same parameters at call site
* give `failOverloading` a more meaningful signature and consolidate the prefix message building logic there
* fix `failOverloading` call sites

[CompilerOps.fs] adjust pattern matching

Expecting behaviour of this code to be identical as before PR

`

* (buildfix) harmonizing .fsi/.fs right before commit doesn't always work with simple copy paste.

* trying to check what kind of things break loose when I change this

I'm trying to identify why we get `(CalledMeth<Expr> * exn list)`, I'm making a cartesian product through getMethodSlotsAndErrors for now, trying to figure out if we can get other than 0 or 1 exception in the list for a given method slot.

I may revert that one if it doesn't make sense make from a reviewer standpoint and/or breaks fsharpqa tests surounding overload resolution error messages.

* (minor) [ConstraintSolver.fs] revert space mishapps to reduce diff, put back comments where they were

* toward displaying the argument names properly (may fail some fsharpqa tests for now)

* missing Resharper's Ctrl+Alt+V "introduce variable" refactoring @auduchinok :)

* pretty print unresolved overloads without all the type submsumption per overload verbose messages

* Overload resolution error messages: things display like I want in fsi, almost like I want in VS2019, this is for the simple non generic method overload case.

I want to check if user experience changes wrt dotnet/fsharp#2503 and put some time to add tests.

* adjust message for candidates overload

* Hijack the split phase for UnresolvedOverloading, remove the match on PossibleOverload.

It consolidates some more of the string building logic, and it now shows as a single compact and exhaustive error message in VS

Thinking I'll change PossibleOverload to not be an exception if going this way is OK

* updating existing failing baseline files that looks correct to me

* quickfix for update.base.line.with.actuals.fsx so that it skips missing base line files in the loop

* (minor) minimize diff, typos, comments

* fix vsintegration tests affected by overload error message changes

* merge issue unused variable warning

* update the 12 fsharpqa migrated baseline with new error messages

* (minor) baseline update script

[update.base.line.with.actuals.fsx]
* add few directories
* error handling (when tests are running, the .err files are locked

* move System.Convert.ToString and System.Threading.Tasks.Task.Run tests from QA

* * moving 3 fsharpqa tests to new test suite
* bring back neg_System.Convert.ToString.OverloadList.fsx which I mistakenly removed during merge

* consolidate all string building logic in CompileOps.fs, fix remaining cases with missing \n (the end result code is less whack-a-mole-ish)

* update base lines

* remove the migrated tests from fsharpqa

* fix vstest error message

* fix env.lst, removed wrong one...

* update baselines of remaining tests

* adding one simple test with many overloads

* appropriate /// comments on `CalledMeth` constructor arguments

* minimize diff / formatting

* trim unused code

* add simple test, one message is interesting, mentioning parameter count for possible overloads

* comment flaky test for now

* code review on the `coerceExpr` function from @TIHan, we can discard first `isOutArg` argument as the only hardcoded usage was guarded by `error` when the value is true, and passing an explicit false which is now guaranteed equivalent to `callerArg.IsOptional`

* code formatting remarks on ConstraintSolver.fs/fsi

* (minor) formatting

* format known argument type / updating .bsl

* update missing baseline

* update missing baselines

* update missing baseline

* minor: make TTrait better in debugger display

* [wip] pass TraitConstraintInfo around failed overload resolution error, and try to display some of it's info

* minimize diff

* signature file mismatch

* removing duplicate in fscomp.txt

* surfacing initial bits of TraitConstraintInfo, roughly for now

* formatting types of known argument in one go, the formatting is still wrong:

* unamed type arguments aren't relabelled (still show their numerical ids)
* SRTP constraints are dismissed, not sure how they should be lined up to pass to SimplifyTypes.CollectInfo

* rework of overload failure message to prettify *ALL* types in a single go, not only argument types, but return types and known generic argument types (this info was never displayed originally but is useful for scenario similar to FSharpPlus implementation)

added `PrettifyDiscriminantAndTypePairs` in TastOps, this allow to weave extra information with the complete list of types, to help reconstituting the several types from their origin (in the usage spot: argument types, return type, generic parameter types)

* fixup the tests and add two tests

* one checking on the new information of known return type and generic parameter types
* one checking we don't leak internal unammed Typar and that they don't all get named 'a despite being different

* updating baselines that got tighter.

* simplify handling of TraitConstraintInfo

* naming couple of fields and turning a property to a methods as it triggers an assert in debugger when inspecting variables

* comments in the assembling of overload resolution error message

* Add information about which argument doesn't match
Add couple of tests
Updating bunch of baselines

* minor updates to testguide and devguide

* fix PrimitiveConstraints.``Invalid object constructor`` test

* fix(?) salsa tests

* minimize diff

* put back tests under !FSHARP_SUITE_DRIVES_CORECLR_TESTS

* missing updated message

* minor adjustments to TESTGUIDE.md

* return type was missing prettifying in prettyLayoutsOfUnresolvedOverloading

* address code review nits / minimize diff / add comment on PrettifyDiscriminantAndTypePairs

* minimize diff

* proposed work around the flaky error message until dotnet/fsharp#6725 has a fix

we keep the fsharpqa test around (but removing the overload error messages from what is asserted out of it) in the meantime

* fixing baselines of new tests from master

* sisyphus round of baseline update

* removing type which isn't in use and popped back up after rebase

* minimize diff

* tidy inconsistent tuple literal

* * removing TTrait properties that end up not being used
* renaming tys and returnTy fields to better match convention (`tys` is used, so no underscore prefix)
* minimizing diff

* minimize diff

* minimize diff

* minimize diff

* link to usage example in same file

* revert converting CallerArg single cased DU into Record

* minimize diff

* minimize diff

* minimize diff

* fix rebase glitches

* fix rebase glitch

* update baseline

* fix base lines / new tests base lines

* edge case: needs a new line after "A unique overload for method '%s' could not be determined based on type information prior to this program point. A type annotation may be needed." if there are no optional parts.

* updating base line for edge case of missing new line

* missing baseline

* removing comment
nosami pushed a commit to xamarin/visualfsharp that referenced this issue Feb 23, 2021
…#6596)

* migrate (as a separate copy) fsharpqa tests that are going to be impacted by dotnet#6596

the baseline files are empty on purpose, expecting CI failure reporting those tests, I intend to update the baseline and clean up the comments / xml tags from fsharpqa syntax in later commit, and then remove those specific tests altogether from fsharpqa if this is OK with the maintainers.

* update the .bsl files

* remove comments that are handled by baseline files, update baseline files

* remove the migrated tests from fsharpqa tests

* need to be more careful when migrating those

* testing if running the test with .fs instead of .fsx makes them work on .netcore

* exclude migrated fsharpqa from dotnet core run

* sample test in fsharpqa (can't run locally for now)

* trying to make it green now.

* checking if this path is covered by a test, trying to identify how to trigger this other overload related error message

* * [MethodCalls.fs] Defining CallerArgs<'T> in, this replaces passing of callerArgsCount, uncurriedCallerArgs and other variants in the overload resolution logic happening in ConstraintSolver & TypeChecker
* [TypeChecker.fs] pass CallerArgs instace at call sites for overload resolution + some commented code as we'll be building a list of given argument types (probably moving as a CallerArgs method or property)
* [ConstraintSolver.fs/fsi] pipe the overload resolution traced callback to `trace.CollectThenUndoOrCommit` as that expression is long and more important in that context

* bit of refactoring of error message building logic during failed overload resolution

[ConstraintSolver.fsi]

* define OverloadInformation and OverloadResolutionFailure, those replace workflow where `string * ((CalledMeth<_> * exn) list)` values were created at call site mixed with message building and matched later in non obvious fashion in `failOverloading` closure
* adjust UnresolvedOverloading and PossibleOverload exceptions

[ConstraintSolver.fs]

* get rid of `GetPossibleOverloads`, this was used only in `failOverloading` closure, and just being passed the same parameters at call site
* give `failOverloading` a more meaningful signature and consolidate the prefix message building logic there
* fix `failOverloading` call sites

[CompilerOps.fs] adjust pattern matching

Expecting behaviour of this code to be identical as before PR

`

* (buildfix) harmonizing .fsi/.fs right before commit doesn't always work with simple copy paste.

* trying to check what kind of things break loose when I change this

I'm trying to identify why we get `(CalledMeth<Expr> * exn list)`, I'm making a cartesian product through getMethodSlotsAndErrors for now, trying to figure out if we can get other than 0 or 1 exception in the list for a given method slot.

I may revert that one if it doesn't make sense make from a reviewer standpoint and/or breaks fsharpqa tests surounding overload resolution error messages.

* (minor) [ConstraintSolver.fs] revert space mishapps to reduce diff, put back comments where they were

* toward displaying the argument names properly (may fail some fsharpqa tests for now)

* missing Resharper's Ctrl+Alt+V "introduce variable" refactoring @auduchinok :)

* pretty print unresolved overloads without all the type submsumption per overload verbose messages

* Overload resolution error messages: things display like I want in fsi, almost like I want in VS2019, this is for the simple non generic method overload case.

I want to check if user experience changes wrt dotnet#2503 and put some time to add tests.

* adjust message for candidates overload

* Hijack the split phase for UnresolvedOverloading, remove the match on PossibleOverload.

It consolidates some more of the string building logic, and it now shows as a single compact and exhaustive error message in VS

Thinking I'll change PossibleOverload to not be an exception if going this way is OK

* updating existing failing baseline files that looks correct to me

* quickfix for update.base.line.with.actuals.fsx so that it skips missing base line files in the loop

* (minor) minimize diff, typos, comments

* fix vsintegration tests affected by overload error message changes

* merge issue unused variable warning

* update the 12 fsharpqa migrated baseline with new error messages

* (minor) baseline update script

[update.base.line.with.actuals.fsx]
* add few directories
* error handling (when tests are running, the .err files are locked

* move System.Convert.ToString and System.Threading.Tasks.Task.Run tests from QA

* * moving 3 fsharpqa tests to new test suite
* bring back neg_System.Convert.ToString.OverloadList.fsx which I mistakenly removed during merge

* consolidate all string building logic in CompileOps.fs, fix remaining cases with missing \n (the end result code is less whack-a-mole-ish)

* update base lines

* remove the migrated tests from fsharpqa

* fix vstest error message

* fix env.lst, removed wrong one...

* update baselines of remaining tests

* adding one simple test with many overloads

* appropriate /// comments on `CalledMeth` constructor arguments

* minimize diff / formatting

* trim unused code

* add simple test, one message is interesting, mentioning parameter count for possible overloads

* comment flaky test for now

* code review on the `coerceExpr` function from @TIHan, we can discard first `isOutArg` argument as the only hardcoded usage was guarded by `error` when the value is true, and passing an explicit false which is now guaranteed equivalent to `callerArg.IsOptional`

* code formatting remarks on ConstraintSolver.fs/fsi

* (minor) formatting

* format known argument type / updating .bsl

* update missing baseline

* update missing baselines

* update missing baseline

* minor: make TTrait better in debugger display

* [wip] pass TraitConstraintInfo around failed overload resolution error, and try to display some of it's info

* minimize diff

* signature file mismatch

* removing duplicate in fscomp.txt

* surfacing initial bits of TraitConstraintInfo, roughly for now

* formatting types of known argument in one go, the formatting is still wrong:

* unamed type arguments aren't relabelled (still show their numerical ids)
* SRTP constraints are dismissed, not sure how they should be lined up to pass to SimplifyTypes.CollectInfo

* rework of overload failure message to prettify *ALL* types in a single go, not only argument types, but return types and known generic argument types (this info was never displayed originally but is useful for scenario similar to FSharpPlus implementation)

added `PrettifyDiscriminantAndTypePairs` in TastOps, this allow to weave extra information with the complete list of types, to help reconstituting the several types from their origin (in the usage spot: argument types, return type, generic parameter types)

* fixup the tests and add two tests

* one checking on the new information of known return type and generic parameter types
* one checking we don't leak internal unammed Typar and that they don't all get named 'a despite being different

* updating baselines that got tighter.

* simplify handling of TraitConstraintInfo

* naming couple of fields and turning a property to a methods as it triggers an assert in debugger when inspecting variables

* comments in the assembling of overload resolution error message

* Add information about which argument doesn't match
Add couple of tests
Updating bunch of baselines

* minor updates to testguide and devguide

* fix PrimitiveConstraints.``Invalid object constructor`` test

* fix(?) salsa tests

* minimize diff

* put back tests under !FSHARP_SUITE_DRIVES_CORECLR_TESTS

* missing updated message

* minor adjustments to TESTGUIDE.md

* return type was missing prettifying in prettyLayoutsOfUnresolvedOverloading

* address code review nits / minimize diff / add comment on PrettifyDiscriminantAndTypePairs

* minimize diff

* proposed work around the flaky error message until dotnet#6725 has a fix

we keep the fsharpqa test around (but removing the overload error messages from what is asserted out of it) in the meantime

* fixing baselines of new tests from master

* sisyphus round of baseline update

* removing type which isn't in use and popped back up after rebase

* minimize diff

* tidy inconsistent tuple literal

* * removing TTrait properties that end up not being used
* renaming tys and returnTy fields to better match convention (`tys` is used, so no underscore prefix)
* minimizing diff

* minimize diff

* minimize diff

* minimize diff

* link to usage example in same file

* revert converting CallerArg single cased DU into Record

* minimize diff

* minimize diff

* minimize diff

* fix rebase glitches

* fix rebase glitch

* update baseline

* fix base lines / new tests base lines

* edge case: needs a new line after "A unique overload for method '%s' could not be determined based on type information prior to this program point. A type annotation may be needed." if there are no optional parts.

* updating base line for edge case of missing new line

* missing baseline

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

No branches or pull requests

5 participants