Update to FCS 38

[baronfel]

This is prep work to update Ionide to FCS 38, and by association the Fantomas integration which breaks due to API differences.

The changes this time mostly related to:

• An new kind of Open declaration (open type),
• a new parameter for xml docs on implicit constructors, and
• a new parameter for Lambdas that captures the original set of patterns and expressions in the lambda before reduction

I'm not saying you should take these changes right now (given that MS is going to push stable FCS shortly), but perhaps this MR can help get a head start on that integration.

There are two things that caused me problems that I wanted to call out for your review specifically:

• The AstTransformer changes for open expressions

  • It appears that unless we report the parentRange in the Node for these declarations, matching of trivia no longer works. This appears to be because the text of the open keyword is no longer included in the range of the SynOpenDeclTarget.ModuleOrNamespace or SynOpenDeclTarget.Type specifically. It seemed fine to use the parentRange here because it encapsulates the entire starting point of the declaration.

• The final failing test, some spacing is still lost in and around #if blocks, 303

  • This test fails with the following output:

Failed some spacing is still lost in and around #if blocks, 303 [48 ms]
  Error Message:
     Expected string length 602 but was 609. Strings differ at index 152.
  Expected: "...sembly.Name\n#if NETCOREAPP2_0\n#else\n    match key with\n   ..."
  But was:  "...sembly.Name\n#if NETCOREAPP2_0\n    do\n#else\n    match key w..."
  ----------------------------------------------^

  Stack Trace:
     at FsUnit.TopLevelOperators.should[a,a](FSharpFunc`2 f, a x, Object y) in /Users/chethusk/oss/fantomas/src/Fantomas.Tests/FsUnit.fs:line 33
   at Fantomas.Tests.CompilerDirectiveTests.some spacing is still lost in and around #if blocks, 303() in /Users/chethusk/oss/fantomas/src/Fantomas.Tests/CompilerDirectivesTests.fs:line 361

  Standard Output Messages:
 seq [Write "let "; Write "internal"; Write " "; Write "UpdateStrongNaming"; ...]
 seq [Write "let "; Write "internal"; Write " "; Write "UpdateStrongNaming"; ...]

It seems to me that this is another trivia/range mismatch, but I haven't yet been able to reason or debug myself into an explanation for it.

In addition, I do not have any tests for the new open type syntax at all.

I would appreciate your help and feedback on these points, as I'm hoping to quickly reach a resolution here and get a prerelease of this into ionide for publishing FCS 38 support. This could of course be done on a fork so that you wouldn't have to merge prerelease dependencies until the stable version is available.

[nojaf]

Hey Chet,

Many thanks for these changes! It is nice to see the impact of FCS 38 before it is released.
Eugene also already prepared some changes in his fork.
Both versions look similar.

For the open expressions, I think the parentRange is the correct call for now. It is a better candidate for Trivia I believe.

Your changes solved some spacing is still lost in and around #if blocks, 303 better than I had. I was going to circle back to this at some point.
I just changed the result of the unit test and it looks better now.
No worries for having no new tests for the new syntax. Could you compose a list with samples of the new syntax maybe?

[baronfel]

Somehow the update to use the FCS stable build has reverted that test (it's the same one with the do statement in the ifdef brackets from before) and now I am very confused.

[nojaf]

@baronfel, I fixed the unit test in a different way and I believe everything is ok.
The short version is that the do keyword can have trivia and is not capture inside the SynExpr.Do or SynModuleDecl.DoExpr range. I've fixed this for our suite of tests.

[baronfel]

Lovely! I was looking at your most recent commit and it does seems like a nice way to go. It's certainly more clean than anything I was contemplating doing to try to solve the issue.

From your perspective, what else should I be looking at to get this merged? Additional test cases around the new 'open type' syntax?

[nojaf]

I'm going through https://devblogs.microsoft.com/dotnet/announcing-f-5/ and I'm adding some extra tests.