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

initial support for F# #237

Merged
merged 9 commits into from
Jun 6, 2019
Merged

initial support for F# #237

merged 9 commits into from
Jun 6, 2019

Conversation

brettfo
Copy link
Member

@brettfo brettfo commented May 25, 2019

tl;dr - Editing/running F# snippets in Try .NET and displaying diagnostics.

Oh boy, is this a giant pile of hacks.

I'm creating this PR so early in the process to get feedback on how to better architect these changes, because right now I'm rather ashamed of what I've done.

List of sins and why I committed them (pun intended):

  1. Workspace servers: Rather than try to factor out all the calls to RoslynWorkspaceServer and properly abstract them, I created WorkspaceServerMultiplexer which simply looks for the suffix .fsproj on the workspace type to redirect to FSharpWorkspaceServer; otherwise it defaults to RoslynWorkspaceServer. As I understand it, this will only work for on-disk workspaces which just happen to set their type to the project file path (see item 1 below). I've mostly plumbed through a Language property, and otherwise auto-detected it from the .fsproj suffix for the workspace servers.
  2. In an attempt to reuse as much as possible (meaning less plumbing), I hijacked various functions to explicitly look for F#-specific file extensions and then bail in a different direction. See BufferInliningTransformer.cs, FileExtensions.cs, and SourceTextExtensions.cs. A method in BufferInliningTransformer.cs has been made virtual to allow for F#-specific overriding.
  3. Markdig doesn't understand code fences of the type fsharp, so to get it to appropriately classify F# blocks as AnnotatedCodeBlocks, I had to lie and say it was C#. The proper fix for this should be adding F# support to Markdig. Turns out it's not Markdig, but this repo; I found and updated the language parsing.
  4. F# can't perform compilations and analysis on purely in-memory data structures like Roslyn can, so I introduced RedirectedPackage which creates a directory under %TEMP%, copies everything from the parent Package, and does the text substitution and all work there. (This also explains why I do source path mapping for F# diagnostics in Library.fs.) After compilation the bin/$(Configuration) directory is then copied back to the 'real' location to allow execution. See item 6.A below.

Things that still need to be done:

  1. Allow a workspace to properly state it's preferred server: RoslynWorkspaceServer or FSharpWorkspaceServer.
  2. Parameterize the hijacked methods and call sites to make the separation cleaner. See item 2 above. Cleaned up as much as possible for this PR. Will be cleaned up even more when enable language as property of workspaces #254 is integrated.
  3. Implement completion in FSharpWorkspaceServer.cs (future PR).
  4. Implement signature help in FSharpWorkspaceServer.cs (future PR).
  5. Add F# tests and fix whatever other tests I most certainly broke.
  6. (Long lead) update the F# libraries to allow in-memory compilation and analysis.
    A. In the short term we could likely hijack the call to dotnet build and pass an additional property, /p:OutputPath=path\to\original\project\bin.

Uninteresting note: The C# runner uses #region/#endregion tags to designate which parts of the file to display in the editor. F# doesn't have the concept of regions but other tools cheat and use the specially-formatted comments, //#region and //#endregion, so I also opted for that approach.

@brettfo brettfo force-pushed the fsharp branch 2 times, most recently from 810ce36 to 9747690 Compare May 25, 2019 20:25
@jonsequitur jonsequitur mentioned this pull request May 26, 2019
{
internal class FSharpMethods
{
private const string FSharpRegionStart = "//#region";
Copy link
Contributor

Choose a reason for hiding this comment

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

"//#region" [](start = 49, length = 11)

Is this ideal? Any suggestions for making it more natural in F#?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really; F# doesn't have the equivalent of #region or any concept of a code block. The closest thing we have is computation expressions, but that would require the writer of the documentation/sample to define a new builder for each span of code. E.g., the backing code could look like this:

type DemoRegionBuilder() =
    member __.Zero() = ()

// ... elsewhere
let demo_region_1 = new DemoRegionBuilder()
let demo_region_2 = new DemoRegionBuilder()
// .. repeat for every equivalent of `#region demo_region_X` that would be found in C#

[<EntryPoint>]
let main(args: string[]) =
    let _ =
        demo_region_1 {
            printfn "hello, world" // this line appears in the web page
        }
    0

With the markdown looking like this:

` ` ` fsharp --project ./MySampleProject.fsproj --source-file ./MySampleFile.fs --region demo_region_1
printfn "hello, world" // this line appears in the web page
` ` `

Adding @KevinRansom to see if he has any additional insight, but I'm still leaning towards specially-formatted comments, because that's essentially what C#/VB #regions are anyways.

@brettfo brettfo changed the base branch from master to feature/fsharp May 30, 2019 20:52
@brettfo brettfo closed this May 31, 2019
@brettfo brettfo reopened this May 31, 2019
@brettfo brettfo force-pushed the fsharp branch 4 times, most recently from 5fd1765 to 9a07403 Compare May 31, 2019 04:01
i <- i + delta
buf.ToString()

let private newlineifyErrorString (message:string) = message.Replace(newlineProxy, Environment.NewLine)
Copy link
Contributor

Choose a reason for hiding this comment

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

"newlineify" 😆

Copy link
Member Author

Choose a reason for hiding this comment

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

Slightly shorter than newlineificize.

@@ -113,7 +113,7 @@ private static void AddSourceFileOption(Command csharp)
var projectFiles = directoryAccessor.GetAllFilesRecursively()
.Where(file =>
{
return directoryAccessor.GetFullyQualifiedPath(file.Directory).FullName == rootDirectory.FullName && file.Extension == ".csproj";
return directoryAccessor.GetFullyQualifiedPath(file.Directory).FullName == rootDirectory.FullName && (file.Extension == ".csproj" || file.Extension == ".fsproj");
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a potential inconsistency here in that I can do this and it will be valid:

```c# --project ./something.fsproj
```

Copy link
Member Author

Choose a reason for hiding this comment

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

My plan (hope?) was to merge as-is to feature/fsharp (where this PR is targeted) and then do a merge in from master to get @colombod's changes that add a language parameter to the workspace object and clean up the various hacks I added around detecting the language.

@brettfo brettfo changed the base branch from feature/fsharp to master June 6, 2019 17:55
@brettfo brettfo changed the title [WIP] initial support for F# initial support for F# Jun 6, 2019
@brettfo brettfo merged commit 80e77b6 into dotnet:master Jun 6, 2019
@brettfo brettfo deleted the fsharp branch June 6, 2019 23:15
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

Successfully merging this pull request may close these issues.

None yet

4 participants