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

Add F# support #1670

Merged
merged 19 commits into from Mar 29, 2017
Merged

Add F# support #1670

merged 19 commits into from Mar 29, 2017

Conversation

srivatsn
Copy link
Contributor

@srivatsn srivatsn commented Mar 2, 2017

This PR sets up the skeleton for what's needed to add basic F# support.

This change adds a new project system with a new project type guid. A IVsProjectSelector needs to be implemented to select this guid for a given project based on some condition of the project. To test though, the .sln file can be modified with this new guid and the project will open with the new project system.

Designtime targets: There's a new file called Microsoft.FSharp.DesignTime.targets that needs to be pulled in by the FSharp targets similar to how it's done for C# here. This is so that the right CPS rules and capabilities get added to the project.

F# capability: The Designtime.targets adds a new FSharp capability. However all of the project system components today are marked as applicable for CSharp or VisualBasic. We need to go through them all and mark them as applicable to F# as well where it makes sense.

Language-service-hookup. There's three pieces of work here:

  • The way the LS hookup works in roslyn project system is that kicks off a designtime build of a target called CompileDesignTime and expects to get the commandline string that would have been passed to the compiler. To do that the Fsc task needs to return the FscCommandLineArgs similar to how the csc task does that here
  • Once the commandline is obtained, it needs to be parsed and FSharpCommandLineParserProvider needs to be filled out by calling into the F# compiler.
  • Finally, we need to create a ProjectContext. There's common code that does that today. However it looks like Roslyn has a check for C#\VB in that code path and a different overload was added for F#. We should just fix that and standardize on calling with an error prefix.

Then there's the big workitem to show the items ordered by fileorder in the tree and provide gestures to move them around.

I believe @brettfo @KevinRansom are going to fill out some of the pieces here.

@dotnet/project-system @Pilchie

@srivatsn
Copy link
Contributor Author

srivatsn commented Mar 3, 2017

fs

@forki
Copy link

forki commented Mar 3, 2017

Kiss

@forki
Copy link

forki commented Mar 3, 2017

I'm really excited for this. Thanks for doing this.

One question after seeing the screenshot: will the package references work like in C#? I mean will it be possible to fill these in memory? This is important in order to keep paket dependency manager support. @davkean I assume yes, but would be nice to hear that everything will be the same.
Thx

@davkean
Copy link
Member

davkean commented Mar 3, 2017

Package references will work exactly like C#.

@forki
Copy link

forki commented Mar 3, 2017

Awesome thanks for confirmation.

@enricosada
Copy link

enricosada commented Mar 3, 2017

To do that the Fsc task needs to return the FscCommandLineArgs similar to how the csc task does that here

Already done that with dotnet/netcorecli-fsc#88 (FSharp.NET.Sdk, the target file) and dotnet/fsharp#2523 (Fsc task)

@srivatsn if you want, you can use test it with the fsproj and nuget.config in https://github.com/enricosada/dotnet-proj-info/tree/master/examples as example.
The nuget.config contains the dev feed with new fsctask and fsharp.net.sdk
the fsproj is the new tempalte for 1.0.0 rtm, so without dotnet-compile-fsc

i use that for a [dotnetcli tool dotnet-proj-info](ref https://github.com/enricosada/dotnet-proj-info/tree/master/examples) who will get fsc args

dotnet proj-info -p ..\c1\c1.fsproj --fsc-args

@srivatsn
Copy link
Contributor Author

srivatsn commented Mar 3, 2017

@enricosada is there a plan to add that back to the regular desktop Fsc task as well? This project system should eventually work for all F# projects.

@enricosada
Copy link

@srivatsn the package Fsharp.Net.Sdk version 1.0.0 already use only Fsc task (not dotnet compile fsc). Is the one from vf# repo.

And also the new fsproj already works to create lib/console target framework net451 etc

Works on mono too :)

@enricosada
Copy link

And net45 are under integration test suite in https://github.com/dotnet/netcorecli-fsc/blob/master/test/dotnet-new.Tests/CommonScenarioTests.cs#L55

@srivatsn or you meant something different?

@srivatsn
Copy link
Contributor Author

srivatsn commented Mar 4, 2017

Sorry I should have been clearer - the fact that the FscTask supports this now is great. However, I was referring to dotnet/netcorecli-fsc#88. My understanding is that that PR is updating the FSharp.targets that are used in the FSharp.NET.Sdk which means any project using the SDK attribute and pulling that SDK will get this. However, old projects that exist today (created using previous versions of VS targeting desktop) will not create the commandline args. I think we should replace the existing project system with this effort and get all F# projects to open up with this new project system which would mean fixing up the targets in the VF# repo - correct? So I was asking if you plan to PR those changes into the VF# repo.

To put this differently, I see that the F# SDK enables a bunch more things and i'm wondering if we should consolidate that with the targets in the VF# repo so that we don't have two styles of projects. For eg, one of the other workitems I mention above - adding an import the FSharp.DesignTime.targets that I'm adding in this PR needs to be done in the F# SDK and in the F# targets in the VF# repo before importing common.targets.

@enricosada
Copy link

An update:

FSharp.Compiler.Tools 4.1.1 (used by FSharp.NET.Sdk 1.0.2) add the new properties to embedded Fsc task.
Eveything is on nuget.org, and the dotnet new templates (of .net core tools 1.0.1) resolve that now.

Ready to use examples also in https://github.com/enricosada/DotnetNewFsprojTestingSamples to test p2p and packageref

@srivatsn
Copy link
Contributor Author

Thanks @enricosada - I just tried the FSharp.NET.Sdk 1.0.2. There's still a problem. It looks like the new properties have been added to the netcoreapp1.0 version of the Fsc task in FSharp.Build.dll but not to the desktop version. Since VS uses desktop msbuild, it picks up the Fsharp.Build.dll in the tools folder in that package and fails design time builds fail saying it can't find the SkipCompilerExecution property.

…e hack in that targets that was adding CSharp capability. I didnt find anything that's common to C# and VB that didn't apply to F# - probably the analyzer handler but that'll be a noop if there are no analyzers.
…hemselves don't work because of missing rules for properties that need to be added.
@srivatsn srivatsn changed the title [WIP] Add F# support Add F# support Mar 26, 2017
@saul
Copy link

saul commented Mar 28, 2017

Thanks @srivatsn.

What's the easy way to get setup and working on these tickets? I'm happy to do some of the work, especially as I just spent a few days working on the file ordering in the old project system.

How can I get a project loaded using the project system on my machine?

cc @brettfo - are these tickets open to be worked on by the community at this point, or just MS?

@srivatsn
Copy link
Contributor Author

@saul - sure feel free to pick up any of this work. Let @brettfo know since I believe he plans to tackle them otherwise. https://github.com/dotnet/roslyn-project-system/blob/master/docs/repo/getting-started.md has instructions to get started.

…added ProjectProperty COM aggregation provider doesn't apply for F#.
@brettfo
Copy link
Member

brettfo commented Mar 28, 2017

Also to @saul (and @srivatsn), one of the steps will be to provide an F# command line parser. I've created #1876 which starts this work, but that's ultimately dependent upon some changes in Roslyn. Once both of these are merged, I'll start work on an F# command line parser that can be used here.

@brettfo
Copy link
Member

brettfo commented Mar 28, 2017

@cloudRoutine, Speaking with @KevinRansom he suggested factoring out this method. My plan was to add a type FSharpCommandLineArguments that inherts from the Roslyn type CommandLineArguments, but first some APIs need to be opened up (see here: dotnet/roslyn#18258).

Once that's been done I need to consume the F# command line parser here.

@saul
Copy link

saul commented Mar 28, 2017

@brettfo @srivatsn do you have an example project file?

I've followed the instructions (using RoslynDev suffix) and I'm using one of @enricosada's example F# .Net Core projects, but I'm getting the following error:

image

@srivatsn
Copy link
Contributor Author

@saul Until #1857 is fixed, you'll need to make those changes in your machine. That is, you need to:

  • Copy the FSharp.NET.Sdk folder from %Program Files%\Dotnet\SDK\sdks to %vsdir%\msbuild\Sdks
  • Add the snippet of xml mentioned in F# SDK needs to import design-time targets and authored into VS #1857 to the two FSharp.NET.Sdk (one in the VSDIR and the other one in your nuget packages directory).
  • Then you can open a F# netcore project by either renaming to msbuildproj as you seem to be doing or by changing the project type guid in the .sln file from F2A71F9B-5D33-465A-A702-920D77279786 (Existing project system's guid) to 6EC3EE1D-3C4E-46DD-8F32-0CC8E7565705 (new project system's guid)

{
[Guid(PackageGuid)]
[PackageRegistration(AllowsBackgroundLoading = true, RegisterUsing = RegistrationMethod.CodeBase, UseManagedResourcesOnly = true)]
[RemoteCodeGeneratorRegistration(SingleFileGenerators.ResXGuid, SingleFileGenerators.ResXGeneratorName,
Copy link
Member

Choose a reason for hiding this comment

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

It looks like you got these off C#, are these the same ones that are enabled for existing F# proejct system?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right - the old F# PS doesn't seem to register any singlefilegenerators. These likely produce cs files and are incorrect. I've removed them.

internal static class FSharpProjectDesignerPage
{
public static readonly ProjectDesignerPageMetadata Application = new ProjectDesignerPageMetadata(new Guid("{6D2D9B56-2691-4624-A1BF-D07A14594748}"), pageOrder: 0, hasConfigurationCondition: false);
public static readonly ProjectDesignerPageMetadata Build = new ProjectDesignerPageMetadata(new Guid("{FAC0A17E-2E70-4211-916A-0D34FB708BFF}"), pageOrder: 1, hasConfigurationCondition: false);
Copy link
Member

@davkean davkean Mar 29, 2017

Choose a reason for hiding this comment

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

unify these across all langauges? Did we do the same thing here for VB?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unify the pages you mean? I think the build page is different for F#.

Choose a reason for hiding this comment

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

Because of this?
image

Choose a reason for hiding this comment

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

Is it XAML? Could you make this page shrinking horizontally?
Textboxes can become wider without any problem so that all the content can be visible.
This with limitation is very inconvenient!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha - is that the only difference? I just meant that the implementation is different (a separate class with a separate guid in the F# assembly but didn't realize it's that close to C#). Ya it might be nice to consolidate these.

@@ -14,7 +14,7 @@
namespace Microsoft.VisualStudio.ProjectSystem.VS.LanguageServices
{
[Export(typeof(IVsContainedLanguageComponentsFactory))]
[AppliesTo(ProjectCapability.CSharpOrVisualBasic)]
[AppliesTo(ProjectCapability.CSharpOrVisualBasicOrFSharp)]
Copy link
Member

Choose a reason for hiding this comment

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

I reckon we should mark these as "Managed".

@@ -33,6 +37,9 @@
<Compile Update="ProjectSystem\Rules\AdditionalFiles.cs">
<DependentUpon>AdditionalFiles.xaml</DependentUpon>
</Compile>
<Compile Update="ProjectSystem\Rules\FSharp.cs">
Copy link
Member

Choose a reason for hiding this comment

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

We get a bug to pull these into appropriate language-specific projects.

@@ -0,0 +1,99 @@
<?xml version="1.0" encoding="utf-8"?>
<!--Copyright, Microsoft Corporation, All rights reserved.-->
Copy link
Member

Choose a reason for hiding this comment

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

Should we just unify these between C#/VB and F#? Do they have any different properties?

Copy link
Member

Choose a reason for hiding this comment

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

Actually, why can we just use the C# one? Don't we do that for VB? And just rename the thing to "Compile"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do as a separate PR since it touches too many files.

@enricosada
Copy link

as a note:

@enricosada
Copy link

enricosada commented Mar 29, 2017

@srivatsn there are <Capability Include="?" /> i need to add to FSharp.NET.Sdk? if i understand right how that works..

namespace Microsoft.VisualStudio.ProjectSystem.VS
{
/// <summary>
/// Provides the Visual Basic implementation of <see cref="IItemTypeGuidProvider"/>.
Copy link
Contributor

Choose a reason for hiding this comment

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

Visual Basic -> FSharp

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess there are lot of places where Visual Basic or C# will be present instead of F#. As we implement the component we should update these files.

@srivatsn
Copy link
Contributor Author

@enricosada the capabilities are included in the FSharp.DesignTime.targets that I've added in this PR. This targets file needs to be imported by the FSharp SDK. More details in #1857

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

10 participants