Witnesses made visible in FCS #9510

dsyme · 2020-06-19T18:37:59Z

This is an FCS feature requested by @alfonsogarciacaro for Fable and also relevant to WebSharper and others

It surfaces the information about witness passing through the FCS API

This will make F# transpilers more semantically accurate

The key additions can be seen in the signatrue files, e.g.

type FSharpExpr =
   ....
    /// Like Call but also indicates witness arguments
    val (|CallWithWitnesses|_|) : FSharpExpr -> (FSharpExpr option * FSharpMemberOrFunctionOrValue * FSharpType list * FSharpType list * FSharpExpr list * FSharpExpr list) option 

    /// Indicates a witness argument index from the witness arguments supplied to the enclosing method
    val (|WitnessArg|_|) : FSharpExpr -> int option

type FSharpMemberOrFunctionOrValue = 

    /// Check if this method has an entrpoint that accepts witness arguments and if so return
    /// the name of that entrypoint and the types of the witness arguments
    member GetWitnessPassingInfo: unit -> (string * IList<FSharpType>) option

TODO

testing
hopefully @alfonsogarciacaro can check if the information is sufficient for Fable

src/fsharp/symbols/Symbols.fsi

alfonsogarciacaro · 2020-06-20T13:20:44Z

This is great, @dsyme, thanks a lot! I can PR some tests to this branch next week with SRTP samples that have been reported recently as not being resolved correctly by Fable 👍

KevinRansom · 2020-06-22T18:21:27Z

@dsyme , this is marked as wip, is it still a wip?

alfonsogarciacaro · 2020-06-24T07:09:42Z

@dsyme I've built FCS from this branch and I'm testing it with Fable: https://github.com/fable-compiler/Fable/blob/1ed140e4ffacf232ca5d14e5bee2e48e5b1c3570/src/Fable.Transforms/FSharp2Fable.fs#L548-L562

It's working but I'm facing a couple of problems:

The index from WitnessArg doesn't seem to be reliable. Every time I've found the pattern is always -1, so I've to make a guess and assume it's 0 if the enclosing scope does contain witnesses.
When visiting some expressions the following line is throwing the exception: "The lists had different lengths. list2 is 1 element shorter than list1":

fsharp/src/fsharp/MethodCalls.fs

Line 1864 in ed9b9b7

let convertedArgs = (argExprs, argTypes) ||> List.map2 (fun expr expectedTy -> mkCoerceIfNeeded g expectedTy (tyOfExpr g expr) expr)

For example I get the exception when trying to parse any of the following texts (note the message always says "list2 is 1 element shorter than list1"):

type Point =
    { x: int; y: int }
    static member Zero = { x=0; y=0 }
    static member Neg(p: Point) = { x = -p.x; y = -p.y }
    static member (+) (p1, p2) = { x= p1.x + p2.x; y = p1.y + p2.y }

type MyNumber =
    | MyNumber of int
    static member Zero = MyNumber 0
    static member (+) (MyNumber x, MyNumber y) =
        MyNumber(x + y)
    static member DivideByInt (MyNumber x, i: int) =
        MyNumber(x / i)

type MyNumberWrapper =
    { MyNumber: MyNumber }

module ArrayTests =
    testCase "Array.average works with custom types" <| fun () ->
        [|MyNumber 1; MyNumber 2; MyNumber 3|] |> Array.average |> equal (MyNumber 2)

    testCase "Array.averageBy works with custom types" <| fun () ->
        [|{ MyNumber = MyNumber 5 }; { MyNumber = MyNumber 4 }; { MyNumber = MyNumber 3 }|]
        |> Array.averageBy (fun x -> x.MyNumber) |> equal (MyNumber 4)

    testCase "Array.sum with non numeric types works" <| fun () ->
        let p1 = {x=1; y=10}
        let p2 = {x=2; y=20}
        [|p1; p2|] |> Array.sum |> equal {x=3;y=30}

    testCase "Array.sumBy with non numeric types works" <| fun () ->
        let p1 = {x=1; y=10}
        let p2 = {x=2; y=20}
        [|p1; p2|] |> Array.sumBy Point.Neg |> equal {x = -3; y = -30}

    testCase "Array.sum with non numeric types works II" <| fun () ->
        [|MyNumber 1; MyNumber 2; MyNumber 3|] |> Array.sum |> equal (MyNumber 6)

    testCase "Array.sumBy with non numeric types works II" <| fun () ->
        [|{ MyNumber = MyNumber 5 }; { MyNumber = MyNumber 4 }; { MyNumber = MyNumber 3 }|]
        |> Array.sumBy (fun x -> x.MyNumber) |> equal (MyNumber 12)

I will try to convert these into tests for this repository and send a PR tomorrow.

Needs more work ... #9510 (comment)

dsyme · 2020-06-26T19:10:23Z

@alfonsogarciacaro Thank you so much for trialling this, I'll take a look at the problems you mention and get things under test too.

dsyme · 2020-06-26T19:11:27Z

For example I get the exception when trying to parse any of the following texts (note the message always says "list2 is 1 element shorter than list1"):

Do you know if you were using the FSharp.Core from a nuget package (without witness passing) or the freshly built FSharp.Core (with witness passsing)

alfonsogarciacaro · 2020-06-26T19:15:03Z

Ah, I had a feeling it might be related to FSharp.Core. I was indeed using the one from Nuget, let me try with a freshly built one.

dsyme · 2020-06-26T19:36:37Z

Ah, I had a feeling it might be related to FSharp.Core. I was indeed using the one from Nuget, let me try with a freshly built one.

Well, we need to be robust when using the nuget one, I was just more asking for my own information when I look into this

alfonsogarciacaro · 2020-06-26T20:34:33Z

Hmm, I tried using the artifacts/bin/FSharp.Core/Proto/netstandard2.0/FSharp.Core.dll file I got after running fcs/build.sh in this branch, both for the Tests project and Fable itself but still getting the same error :/

As a side note, I must say that building this repo in macOS is a breeze compared to what it used to be. Kudos to everybody involved in improving this!

…fcsw

ncave · 2020-09-17T11:32:00Z

Adding --langversion:preview (see #10140) seems to be breaking FCS tests here.

…fcsw

cartermp · 2020-10-24T18:55:16Z

FYI @dsyme @alfonsogarciacaro we are planning on release FCS 38 alongside F# 5, which is just in a few short weeks. If this is important to get in by then, we should prioritize addressing all outstanding issues with this one so that it's not a mad rush to publish.

alfonsogarciacaro · 2020-10-25T01:02:46Z

For what it concerns Fable, right now we are using a custom build of FCS so we don't depend on the package, so please don't rush things just to let this in, we can build from this repo later (we can even build from the branch). Although other projects like Websharper may do be waiting for this to be included in the FCS package, see dotnet-websharper/core#1113

alfonsogarciacaro · 2020-10-30T01:35:37Z

In contrast, in the FSharpExprConvert.GetWitnessArgs call stack there's a point where the constraints of the type parameters are set. Maybe something like that is missing in .GetWitnessPassingInfo()?

FSharpExprConvert.GetWitnessArgs 
> ConstraintSolver.CodegenWitnessesForTyparInst
> ConstraintSolver.FreshenTypeInst
> ConstraintSolver.FreshenAndFixupTypars
> ConstraintSolver.FixupNewTypars

fsharp/src/fsharp/infos.fs

Line 381 in 886205d

    
           (tpsorig, tps) ||> List.iter2 (fun tporig tp -> tp.SetConstraints (CopyTyparConstraints  m tprefInst tporig))

dsyme · 2020-10-30T11:56:59Z

@alfonsogarciacaro Thanks, I'm setting things up to repro your situation

I assume the README in lib\fcs in the Fable repro is accurate?

C:\GitHub\dsyme\Fable>more lib\fcs\README.md
# FSharp.Compiler.Service

This binary is generated by the following steps:

- Clone https://github.com/ncave/fsharp
- Check out `service_slim` branch
- Run `bash fcs/build.sh`
- Copy `artifacts/bin/FSharp.Compiler.Service/Release/netstandard2.0/FSharp.Compiler.Service.*`
- Copy `artifacts/bin/FSharp.Compiler.Service/Release/netstandard2.0/FSharp.Core.*`

dsyme · 2020-10-30T11:59:49Z

Actually I guess I want this branch

service_slim_fcsw -> ncave/service_slim_fcsw

dsyme · 2020-10-30T12:07:56Z

@ncave I'm have some trouble building ncave/service_slim_fcsw. I get

C:\GitHub\dsyme\ncave\src\buildtools\buildtools.targets(30,5): error MSB3073: The command ""C:\Program Files\dotnet\dotnet.exe" "C:\GitHub\dsyme\ncave\artifacts\\bin\fslex\Release\netcoreapp3.1\fslex.dll" -o "C:\GitHub\dsyme\ncave\artifacts\obj\FSharp.Compiler.Private\Proto\net472\illex.fs" --unicode --lexlib Internal.Utilities.Text.Lexing ..\absil\illex.fsl" exited with code 1. [C:\GitHub\dsyme\ncave\src\fsharp\FSharp.Compiler.Private\FSharp.Compiler.Private.fsproj]

after running

.\build.cmd

in that branch. I'll try various techniques to unblock

alfonsogarciacaro · 2020-10-30T12:39:13Z

@dsyme Yes, the instructions are correct. And for the witnesses you are correct it is ncave/fsharp:service_slim_fcsw branch. I think you get the error if you run ./build.cm, you must run bash fcs/build.sh (that is, the script in the "fcs" directory) instead. I assume in Windows you can use git bash to run the script, but its contents are just:

dotnet build -c Release src/buildtools/buildtools.proj
dotnet build -c Release src/fsharp/FSharp.Compiler.Service

Actually , the fable:witnesses-repro already contains the FCS binaries, so you only need to build again if you want to make a change and test it.

alfonsogarciacaro · 2020-10-30T12:49:53Z

A full script to recompile FCS and test it with Fable, assuming you're in Fable directory and ncave/fsharp:service_slim_fcsw is in the "ncave" sibling directory, would be:

cd ..\ncave
dotnet build -c Release ../ncave/src/buildtools/buildtools.proj
dotnet build src/fsharp/FSharp.Compiler.Service -c Release
cd ..\fable
copy /y ..\ncave\artifacts\bin\FSharp.Compiler.Service\Release\netstandard2.0\FSharp.Compiler.Service.* lib\fcs\
copy /y ..\ncave\artifacts\bin\FSharp.Compiler.Service\Release\netstandard2.0\FSharp.Core.* lib\fcs\
dotnet run -p src/Fable.Cli -- src/quicktest

dsyme · 2020-10-30T13:39:01Z

OK, the problem with List.sum repros in main as a regression when taking quotations of code involving List.sum (when trying to generate the quotation data).

@cartermp This is a regression, I presume we may see this in the wild. I will add a separate issue for this.

type Point =
    { x: int; y: int }
    static member Zero = { x=0; y=0 }
    static member (+) (p1, p2) = { x= p1.x + p2.x; y = p1.y + p2.y }
let p1 = {x=1; y=10}
let p2 = {x=2; y=20}

<@ List.sum [p1] @>

dsyme · 2020-10-30T13:48:39Z

Tracked here: #10364

dsyme · 2020-10-30T15:24:14Z

@alfonsogarciacaro I think I'm doing this right, that QuickTest now seems to pass:

C:\GitHub\dsyme\Fable>dotnet run -p src/Fable.Cli -- src/quicktest
Fable: F# to JS compiler 3.0.0-nagareyama-beta-004
Thanks to the contributor! @vbfox
src\quicktest> cmd /C "dotnet" restore QuickTest.fsproj
  Determining projects to restore...
  Restored C:\GitHub\dsyme\Fable\src\quicktest\QuickTest.fsproj (in 406 ms).
Parsing src\quicktest\QuickTest.fsproj...
Initializing F# compiler...
Compiling src\quicktest\QuickTest.fsproj...
F# compilation finished in 2454ms
Compiled src\quicktest\QuickTest.fs
Fable compilation finished in 904ms

dsyme · 2020-10-30T23:16:31Z

This is ready once @alfonsogarciacaro and @ncave confirm it works for Fable without additional issues

alfonsogarciacaro · 2020-10-31T00:14:46Z

This is awesome @dsyme, thanks a lot for all your help! I've merged your latest changes and all Fable tests are passing now 🎉 🎉 🎉

GetWitnessPassingInfo still doesn't seem to work, but the cases where BasicPatterns.TraitCall appears without inlining can be resolved by simply selecting the appropriate method using the trait info.

The performance fix is also very welcome! So far when we were building active patterns by composing BasicPatterns (so we were visiting expressions more than once) there was always a performance hit, and this was probably the reason.

The trial by fire will now be to compile FSharpPlus with Fable ;)

dsyme · 2020-11-02T12:42:22Z

This is awesome @dsyme, thanks a lot for all your help! I've merged your latest changes and all Fable tests are passing now 🎉 🎉 🎉

This is great. This PR is now ready to merge

The trial by fire will now be to compile FSharpPlus with Fable ;)

Just a word on this: that library is complex and relies on baroque aspects of SRTP resolution that use the feature in ways that have never been recommended F# practice, and which to be honest I believe make for a poor user experience (e.g. in error messages, simplicity and compiler performance, let alone the problem I have with throwing the Functor/Applicative/... computational abstractions at beginners who deeply don't need them). I occasionally use it as a stress test, and use it as a way to isolate out test cases that capture the exact nature of those baroque corners, and it's useful for flushing those out. It's those individual extracted tests - the existing ones are already in this repo - that I consider the acceptance criteria. If the library does cause problems, please extract out a minimal test case, and we'll fix it as a separate bug.

dsyme · 2020-11-02T23:25:40Z

@KevinRansom @TIHan @cartermp Can I have a review on this please? It's ready to go in.

KevinRansom

Thanks for this Don

cartermp

Thanks - been following along as this progressed. Lots of formatting changes but they're all for the better.

alfonsogarciacaro · 2020-11-07T08:09:53Z

The release candidate of Fable 3 has been published with these changes and it's working flawlessly (at least in my tests. Thanks again for all your work @dsyme!

Also, not entirely sure if it's all due to the performance fix in this PR but I made another simple benchmark of the compiler and the Fable part compilation (after FCS ParseAndCheck) is now around 25% faster! 🚀 This is in the addition to the performance improvements that were already presented at F# eXchange.

forki · 2020-11-07T08:22:32Z

Awesome! So exciting

Co-authored-by: Don Syme <[email protected]>

witnesses made visible in FCS

de9923d

auduchinok reviewed Jun 19, 2020

View reviewed changes

src/fsharp/symbols/Symbols.fsi Outdated Show resolved Hide resolved

KevinRansom previously approved these changes Jun 20, 2020

View reviewed changes

alfonsogarciacaro mentioned this pull request Jun 23, 2020

Witnesses fable-compiler/Fable#2104

Closed

alfonsogarciacaro mentioned this pull request Jun 25, 2020

[WIP] Witnesses for STRP resolution fable-compiler/Fable#2108

Closed

alfonsogarciacaro mentioned this pull request Jul 30, 2020

Including F#+ in the repl fable-compiler/repl#116

Open

brettfo changed the base branch from master to main August 19, 2020 20:02

Merge branch 'main' of https://github.com/dotnet/fsharp into feature/…

46ba75a

…fcsw

This was referenced Sep 12, 2020

Cleanup build artifacts in Fable 3 fable-compiler/Fable#2161

Closed

Witnesses ncave/fsharp#4

Closed

Jand42 mentioned this pull request Sep 16, 2020

Use trait call resolution provided by FCS when it becomes available dotnet-websharper/core#1113

Open

dsyme and others added 5 commits September 22, 2020 14:45

Merge branch 'main' of https://github.com/dotnet/fsharp into feature/…

0e81647

…fcsw

update baselines

9ff1bfb

merge main

8640a8d

fix build

fd6a290

add testing

fa405cc

dsyme changed the title ~~[WIP] Witnesses made visible in FCS~~ Witnesses made visible in FCS Oct 24, 2020

add testing

1a8bb47

dsyme mentioned this pull request Oct 30, 2020

Regression with quotations of List.sum on non-primitive type #10364

Closed

dsyme force-pushed the feature/fcsw branch from 316af3b to eb2f282 Compare October 30, 2020 14:57

Don Syme added 2 commits October 30, 2020 15:16

Fix Regression with quotations of List.sum on non-primitive type

c4b28a1

further testing for FCS on List.sum on non-primitive type

c8d5dfd

dsyme force-pushed the feature/fcsw branch from eb2f282 to c8d5dfd Compare October 30, 2020 15:16

run Expr tests in independent checkers to get consistent names

812e67e

dsyme requested a review from KevinRansom November 2, 2020 23:25

KevinRansom approved these changes Nov 3, 2020

View reviewed changes

cartermp approved these changes Nov 3, 2020

View reviewed changes

cartermp merged commit 05b0569 into main Nov 3, 2020

cartermp deleted the feature/fcsw branch November 3, 2020 01:32

This was referenced Nov 3, 2020

Update service_slim_fcsw to latest ncave/fsharp#5

Closed

Added service_slim ncave/fsharp#2

Draft

alfonsogarciacaro mentioned this pull request Nov 17, 2020

Added --fableLib arg fable-compiler/Fable#2274

Merged

nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Feb 23, 2021

Witnesses made visible in FCS (dotnet#9510)

d84b3d6

Co-authored-by: Don Syme <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Witnesses made visible in FCS #9510

Witnesses made visible in FCS #9510

dsyme commented Jun 19, 2020 •

edited

Loading

alfonsogarciacaro commented Jun 20, 2020

KevinRansom commented Jun 22, 2020

alfonsogarciacaro commented Jun 24, 2020

dsyme commented Jun 26, 2020

dsyme commented Jun 26, 2020

alfonsogarciacaro commented Jun 26, 2020

dsyme commented Jun 26, 2020

alfonsogarciacaro commented Jun 26, 2020

ncave commented Sep 17, 2020

cartermp commented Oct 24, 2020

alfonsogarciacaro commented Oct 25, 2020

alfonsogarciacaro commented Oct 30, 2020

dsyme commented Oct 30, 2020

dsyme commented Oct 30, 2020

dsyme commented Oct 30, 2020

alfonsogarciacaro commented Oct 30, 2020 •

edited

Loading

alfonsogarciacaro commented Oct 30, 2020 •

edited by dsyme

Loading

dsyme commented Oct 30, 2020 •

edited

Loading

dsyme commented Oct 30, 2020

dsyme commented Oct 30, 2020

dsyme commented Oct 30, 2020

alfonsogarciacaro commented Oct 31, 2020

dsyme commented Nov 2, 2020 •

edited

Loading

dsyme commented Nov 2, 2020

KevinRansom left a comment

cartermp left a comment

alfonsogarciacaro commented Nov 7, 2020

forki commented Nov 7, 2020

Witnesses made visible in FCS #9510

Witnesses made visible in FCS #9510

Conversation

dsyme commented Jun 19, 2020 • edited Loading

alfonsogarciacaro commented Jun 20, 2020

KevinRansom commented Jun 22, 2020

alfonsogarciacaro commented Jun 24, 2020

dsyme commented Jun 26, 2020

dsyme commented Jun 26, 2020

alfonsogarciacaro commented Jun 26, 2020

dsyme commented Jun 26, 2020

alfonsogarciacaro commented Jun 26, 2020

ncave commented Sep 17, 2020

cartermp commented Oct 24, 2020

alfonsogarciacaro commented Oct 25, 2020

alfonsogarciacaro commented Oct 30, 2020

dsyme commented Oct 30, 2020

dsyme commented Oct 30, 2020

dsyme commented Oct 30, 2020

alfonsogarciacaro commented Oct 30, 2020 • edited Loading

alfonsogarciacaro commented Oct 30, 2020 • edited by dsyme Loading

dsyme commented Oct 30, 2020 • edited Loading

dsyme commented Oct 30, 2020

dsyme commented Oct 30, 2020

dsyme commented Oct 30, 2020

alfonsogarciacaro commented Oct 31, 2020

dsyme commented Nov 2, 2020 • edited Loading

dsyme commented Nov 2, 2020

KevinRansom left a comment

Choose a reason for hiding this comment

cartermp left a comment

Choose a reason for hiding this comment

alfonsogarciacaro commented Nov 7, 2020

forki commented Nov 7, 2020

dsyme commented Jun 19, 2020 •

edited

Loading

alfonsogarciacaro commented Oct 30, 2020 •

edited

Loading

alfonsogarciacaro commented Oct 30, 2020 •

edited by dsyme

Loading

dsyme commented Oct 30, 2020 •

edited

Loading

dsyme commented Nov 2, 2020 •

edited

Loading