Skip to content

Conversation

@dsyme
Copy link
Contributor

@dsyme dsyme commented Sep 20, 2020

Now that we have decent diagnostics while editing scripts, it quickly becomes apparent that VS tooling assumes that scripts are .NET Framework, e.g. see #10160

Motivated this I put together a prototype implementation of explicit target framework specifications in scripts. The mini-spec is below.

I also added an option in VS to use F# Interactive for .NET Core. .

image

image

This works and is practical for use and I'll be using it myself.

Limitations:

  • There is no cancellation of F# Interactive execution for .NET Core. You have to restart. This is because of dotnet fsi doesn't support Ctrl-C Interrupt - results in unhandled PlatformNotSupportedException #9397
  • There is no debugging of execution for .NET Core. This is because of Produce debug information in .NET Core fsi #5457
  • The order of the options in the Visual Studio options panel could be improved. I'm not sure what controls the ordering
  • The VS setting doesn't control whether scripts are analysed as .NET Core scripts by default. You currently need to add #targetfx "netcore" to the script
  • The F# Interactive started uses dotnet fsi and doesn't use either the FSI installed by the VSIX (there is no .NET COre FSI installed by the VSIX)
  • dotnet is found using a hardwired path to c:\Program Files\dotnet\dotnet.exe

Mini-spec: Explicit target framework specifications in scripts

Scripts may include the declarations

    #targetfx "netfx"
    #targetfx "netcore"

It is not possible to declare scripts as #netstandard2.0 even if they are neutral, though that would be relatively easy to add.

If present, an explicit framework declaration must be the first non-whitespace, non // comment token in the script.

Editing

When editing (in Visual Studio):

  • a script with an explicit target framework declaration is analysed with respect to the latest tooling-supported version of the corresponding target framework. This will typically be the same .NET SDK used to execute scripts with dotnet fsi

  • a script without an explicit target framework declaration is by default analysed as .NET Framework. If the option "Use .NET Core Scripting" is selected in the Visual Studio options then .NET Core is assumed.

General rules for all script analysis:

  • If a script being edited does #load on another script that has an incompatible target framework declaration then a warning is reported at the point of the #load

  • If a script being edited does #load on another script that has no target framework declaration then no warning is reported. The overall contents are analysed with respect to the explicit or default target framework of the script being edited.

Compiling

As background, F# fsc.exe supports compiling scripts. This is the "script compilation" feature that has always been present in the F# compiler - it allows you to include scripts on the command line or in your project and during compilation you get the load closure and references from them. This applies even if the script is in a project (with 'Compile' action), for example. Any DLL references implied by package references are also retrieved from the script. When script compilation is invoked, the outputs are not necessarily a functioning application - the referenced DLLs are not copied to the output folder, for example (except perhaps FSharp.Core.dll).

Wxisting compilation arguments determine the target framework (--targetprofile:(mscorlib|netcore|netstandard)). The default for fsc.exe is always --targetprofile:mscorlib. This is unchanged.

If scripts being compiled have #netfx and #netcore declarations they must be consistent with the --targetprofile flag of the compilation else a warning is emitted. If the compilation uses --targetprofile:netstandard, then scripts on the command line must not have either #netfx or #netcore.

Script Execution (fsi and .NET Interactive)

The FSI instance is either netfx or netcore. If a script with an incompatible target framework declaration is loaded for execution a warning is reported.

FSharp.Compiler.Service

FSharpChecker.GetProjectOptionsFromScript accepts an optional parameter defaultToDotNetFramework that says whether scripts should be assumed to be .NET Framework in the absence of any other declarations. In the absence of a declaration the default is assumed to be .NET Framework scripting when FCS is running in .NET Framework, and .NET Core when running in .NET Core.

As today FSharpChecker.Compile defaults to --targetprofile:mscorlib so --targetprofile:netcore must be given if a #netcore script is being compiled. The presence of a #netcore declaration in the main script being compiled can be computed by first calling GetProjectOptionsFromScript.

Script Execution in Visual Studio

By default scripts are executed using F# Interactive for .NET Framework.

If you select ".NET Core Scripting" from the Visual Studio F# Tool options then F# Interactive for .NET Core will be used on next restart of F# Interactive.

The feature is not labelled as "preview" since it requires explicit opt-in either via #targetfx or selecting ".NET Core Scripting" form the Visual Studio F# Tool options.


@KevinRansom
Copy link
Contributor

@dsyme -- please ... don't. netfx is going to be deprecated eventually and only legacy tooling will target it.

@dsyme
Copy link
Contributor Author

dsyme commented Sep 20, 2020

@dsyme -- please ... don't. netfx is going to be deprecated eventually and only legacy tooling will target it.

Well, we're not exactly going to wait until then to support script editing in Visual Studio, are we? The current status quo is just weird.

The only other possibility is a global setting, which we can have but is broken as the only mechanism.

@KevinRansom
Copy link
Contributor

@dsyme - a switch is fine and manageable. A directive creates a difficult comparability story.

@dsyme
Copy link
Contributor Author

dsyme commented Sep 20, 2020

@dsyme - a switch is fine and manageable.

Nah, it's not fine. I edit both .NET Core and .NET Framework scripts, and we do that even in-repo. It's not a solution.

A directive creates a difficult comparability story.

Why? To be honest it's the opposite - it's being explicit about something scripts should always have been explicit about since .NET Core scripting came into existence.

If the problem is the netcore --> net5 naming then that's just naming and/or compat can be thrashed out. But fundamentally we need this metadata until there is no more default assumption that scripts are .NET Framework.

Anyway this is just a spike, we can discuss this in a RFC. Your concerns are legitimate but we need a solution.

@dsyme
Copy link
Contributor Author

dsyme commented Sep 22, 2020

Here is a shot of .NET Core scripting editing working in Visual Studio:

image

  • There is one hack where I hardwire c:\Program Files\dotnet\sdk as the home for .NET SDKs.
  • The logic for deciding which .NET SDK to use for reference assemblies will need to be discussed at length
  • There are obv. other possible refinements in the design (e.g. allowing "netstandard2.0", "netstandard2.1" and "net5" scripts)

@baronfel
Copy link
Member

We had to do similar in FSAC, where I implemented probing logic to enumerate the versions of the sdk and runtime installed, using the pack references whenever possible. I also implemented a varying dotnet root via environment variables because of course the sdks can be located anywhere. It makes me feel good that you're hitting some of these same issues, validates that I wasn't going crazy.

@dsyme
Copy link
Contributor Author

dsyme commented Sep 22, 2020

Based on a discussion with @KevinRansom

Given the code in this PR there are a number of simple variations in overall design we can now implement for the F# scripting model and its defaults in Visual Studio

Design 1 (@KevinRansom ):

  • Support declaration #targetfx "netfx" (or #netfx) for editing legacy scripts
  • Otherwise assume .NET Core scripting everywhere in all Visual Studio tools, including for scripts with no declaration
  • Support a global VS setting for which F# Interactive we start up, defaulting to .NET Core

Design 2 (current spike):

  • Support both #targetfx "netfx" and #targetfx "netcore" for editing scripts
  • Assume netfx for unannotated scripts.
  • F# Interactive defaults to .NET Framework
  • Future: support a global VS setting for which F# Interactive we start up, defaulting to .NET Core
  • Future: generalise #targetfx to allow minimal framework specifications, e.g. #targetfx "net5"

I think design 1 is better.

  • Pro: simple for .NET Core scripting, which is the future
  • Pro: it matches what's in Ionide and .NET Notebooks
  • Con: It sort of counts as a breaking change because to get editing for .NET Framework scripts you have to add a declaration, and to execute them you have to set a global setting in VS.
  • Con: I quite like the general ability to specify a minimal framework for scripts, I think we should keep that bit.

Anyway option 1 is a relatively small tweak from the PR now the mechanisms are in place

@dsyme
Copy link
Contributor Author

dsyme commented Sep 22, 2020

I busted .NET Framework scripting in some way - will work out what's up tomorrow or later in the week.

image

Copy link
Contributor

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

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

This looks good so far. I especially like the FxResolver for DotnetFrameworkReferences refactor.

// Copyright (c) Microsoft Corporation. All Rights Reserved. See License.txt in the project root for license information.

// Functions to retrieve framework dependencies
module internal FSharp.Compiler.DotNetFrameworkDependencies
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably we can lose the DotNetFrameworkDependencies

None, None
else
// Running on .NET Framework 32 bit windows (e.g. devenv.exe), need to find a .NET SDK
let sdks = @"C:\Program Files\dotnet\sdk" // TODO - correct this technique assuming this is devenv.exe
Copy link
Contributor

Choose a reason for hiding this comment

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

This is kind of tough, the ide may run on the desktop ... for sure it is at the moment. I will try and find an API, I think there may be one somewhere ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is kind of tough, the ide may run on the desktop ... for sure it is at the moment. I will try and find an API, I think there may be one somewhere ...

All the searches say "run dotnet --info or dotnet--list-sdks". But how to find dotnet.exe? There must be a way for VS to probe it's corresponding dotnet install, and we can pass that through

Copy link
Contributor

Choose a reason for hiding this comment

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

Is dotnet.exe always on PATH? This is how the official MSBuildLocator does it: https://github.com/microsoft/MSBuildLocator/blob/master/src/MSBuildLocator/DotNetSdkLocationHelper.cs

@smoothdeveloper
Copy link
Contributor

@dsyme can this be handled in tooling first (with an option to pick FSI version) without making the breaking change if there are no directives, in which case it shouldn't assume either runtime but match with the tooling selected FSI?

Adding the directive is fine, but forcing a new default runtime breaks editing majority of existing scripts in VS tooling.

Other editors seems to enable picking which version of FSI it uses, VS is the exception there.

@abelbraaksma
Copy link
Contributor

Otherwise assume .NET Core scripting everywhere in all Visual Studio tools, including for scripts with no declaration

As long as we cannot run Dotnet FSI from VS, I think the default should be netfx and not netcore (as much as I regret to say this), at least as long as you cannot run the script you're editing from within VS.

NetFx as default also matches the current default. If we suddenly change the default in absence of a #targetfx it'll mean that any person that's used to currently testing/running/debugging scripts from within VS will suddenly come to see red squiggles in previously "clean" scripts.

Regardless the choice of default, there should always be the possibility to explicitly state such default. That's a self-documenting feature. If we do choose #targetfx netcore as the default, we should be able to spell it out.

Support a global VS setting for which F# Interactive we start up, defaulting to .NET Core

As far as I know it is non-trivial to change the FSI inside VS to run as .NET Core. I've come to understand that it requires out-of-process execution because VS itself is .NET Framework (though I hope I'm wrong here ;) ).

@dsyme
Copy link
Contributor Author

dsyme commented Sep 29, 2020

As long as we cannot run Dotnet FSI from VS, I think the default should be netfx and not netcore (as much as I regret to say this), at least as long as you cannot run the script you're editing from within VS.

We should simply next extend this feature work to make it possible (and the default) to run .NET Core FSI from Visual Studio.

First thing however is to get the build green, there are some issues with the default settings

@runfoapp runfoapp bot mentioned this pull request Sep 29, 2020
@dsyme
Copy link
Contributor Author

dsyme commented Dec 16, 2020

@KevinRansom and I talked this through and come to the following spec

  • .NET Core scripting will initially be in "preview" (opt-in) and become the default (opt-out) when taken out of preview

  • The overall guiding principle is script editing and execution aligns with the behaviour of “dotnet fsi script.fsx” when started within the directory where the script resides

    This has significant positives and ramifications. Specifically, “dotnet fsi script.fsx” already respects the global.json within that directory structure. This affects both the runtime used and the .NET SDK chosen. This is actually a really, really good thing as it allows the .NET SDK and other settings used by a script to be pinned down precisely via global.json, a key step in more professional, sound, reliable script programming support and something we can already advertise as a positive of “dotnet fsi” over .NET Framework “fsi.exe”.

  • There will be no language-level declaration

  • Editing/analysis: The choice of the .NET SDK (e.g. global.json) affects the references that should, logically speaking, be used for analysis on that script.

    As a result, when editing, we will attempt to query “dotnet fsi” by running it in the directory of the script, to “ask” it for the relevant references for a script.

    • This will likely be done by a new flag dotnet fsi --info which writes the relevant references to stdout or a temp file.

    • If dotnet fsi --info fails to start (e.g. the SDK in global.json is not installed) we will report a warning in the editor and fall back to existing .NET Framework references

    • There will be a VS option to say “always use the desktop references for editing” except

    • This behaviour will be implemented in GetProjectOptionsFromScript so will be relevant to all FCS users as well that implement support for the F# scripting model

    • There should likely be an override argument for FCS, e.g. where the .NET SDK is already known or should be determined from the currently running process, e.g. for notebooks

  • Execution: For execution from VS, the logical startup directory for “dotnet fsi” for the F# Interactive window in VS will be the directory of the currently opened script.

    • This is a slightly awkward – there may not be a script open. If not we will use % USERPROFILE%\source\repos

    • The F# Interactive window in VS will print both the startup directory, the reason why it was chosen (“chosen from open script c:\misc\abc.fsx”) and the .NET SDK version used.

    • The dotnet.exe to use is determined by some apphost logic that Kevin knows about

  • Debugging of scripts is permitted for .NET Core scripting, but with a warning emitted about lack of symbols.

    • It is useful for scripts that use debuggable libraries. However no debug symbols are emitted for scripting code.

    • Because of this, when debugging is activated a warning will be printed in the VFSI window that breakpoints will not be respected.

  • Interrupt cancellation is currently deactivated in the menus for .NET Core scripting

    We may add it back once we have some form of cancellation, e.g. for debuggable code we could emit some checks, even if imperfect

  • There is a switch in the VS tools to go back to .Net Framework-based scripts

    • For editing, changing the setting requires you to close/reopen the script

    • For execution, changing the setting requires a restart of the VFSI process

@dsyme
Copy link
Contributor Author

dsyme commented Dec 17, 2020

@KevinRansom I've updated this PR

  • no language-level declaration
  • Editing/analysis: when editing, we will attempt to query “dotnet --version” by running it in the directory of the script, to work out the relevant SDK
  • a VS option to say “always use the desktop references for editing” except
  • This behaviour will be implemented in GetProjectOptionsFromScript so will be relevant to all FCS users as well that implement support for the F# scripting model
  • an override argument for FCS for .NET SDK location
  • Execution: not done
  • Debugging: not done
  • Interrupt cancellation deactivated: not done

I'll test the editing and do the rest tomorrow/Friday

@KevinRansom
Copy link
Contributor

KevinRansom commented Dec 17, 2020 via email

Copy link
Contributor

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

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

Wow, there is a lot here.
There are some queries, perhaps a rename my preference is that the spawning of a process to get the tfm was elsewhere but I can see why you put it here.

Take care of the feedback you think impactful, otherwise I am good with this.

let primaryReference = tcConfig.PrimaryAssemblyDllReference()

let assumeDotNetFramework = primaryReference.SimpleAssemblyNameIs("mscorlib")
let useDotNetFramework = primaryReference.SimpleAssemblyNameIs("mscorlib")
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are renaming, might as well go with DesktopFramework, DotnetFramework is a bit ambiguous.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll just undo this renaming to minmise the diff, we can rename later

pdbDirPath = None
tryGetMetadataSnapshot = (fun _ -> None) (* tryGetMetadataSnapshot *) }

let reader = OpenILModuleReader path opts
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice.

static member ClearStaticCaches() =
desiredDotNetSdkVersionForDirectoryCache.Clear()

member _.GetFrameworkRefsPackDirectory() = tryGetSdkRefsPackDirectory()
Copy link
Contributor

Choose a reason for hiding this comment

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

How is this to be used? I would rather not spread this throughout the compiler ... it's bad enough that this file has to know about it.


member _.GetSystemAssemblies() = systemAssemblies

member _.IsInReferenceAssemblyPackDirectory filename =
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this used elsewhere in the compiler, if so, lets not. We already have SystemTypes that covers this.

member _.TryGetSdkDir() = tryGetSdkDir()

/// Gets the selected target framework moniker, e.g netcore3.0, net472, and the running rid of the current machine
member _.GetTfmAndRid() =
Copy link
Contributor

Choose a reason for hiding this comment

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

So, how is this intended to be used?


try
let result = fsiOptions.DependencyProvider.Resolve(dependencyManager, ".fsx", packageManagerTextLines, reportError m, executionTfm, executionRid, tcConfigB.implicitIncludeDir, "stdin.fsx", "stdin.fsx")
let tfm, rid = tcConfigB.fxResolver.GetTfmAndRid()
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason not have fxResolver on fsiOptions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done thanks

let DefaultReferencesForOrphanSources useDotNetFramework =
let currentDirectory = Directory.GetCurrentDirectory()
let fxResolver = FxResolver(Some useDotNetFramework, currentDirectory, range0)
let references, _ = fxResolver.GetDefaultReferences (useFsiAuxLib=false, useDotNetFramework=useDotNetFramework, useSdkRefs=false)
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe useSdkRefs is default true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup I'll change this. Though this entry point DefaultReferencesForOrphanSources isn't actually used anymore at least by VS, and it's not under test. We should probably mark it as obsolete. It seems that FSharp.Editor relies on using the "empty" command-line arguments without --noframework etc. for out of project non-script sources, which is I believe equivalent (i.e. equivalent to this entry with useDotNetFramework=true)

// Use the location of this dll
location

let getDefaultFSharpCoreLocation() = Path.Combine(getFSharpCompilerLocation(), getFSharpCoreLibraryName + ".dll")
Copy link
Contributor

Choose a reason for hiding this comment

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

Noice


#blaaaaaa // blaaaaaa is not a known command;;
^
^^^^^^^^^
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did this change, not that it's not a good change it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes bug fix in pars.fsy I noticed while doing the #netfx thing

// Otherwise give up
raise (SessionError (VFSIstrings.SR.couldNotFindFsiExe fsiRegistryPath))
if SessionsProperties.fsiUseNetCore then
let exe = @"c:\Program Files\dotnet\dotnet.exe"
Copy link
Contributor

Choose a reason for hiding this comment

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

Combine the ProgramFiles Environment variable with dotnet\dotnet.exe
Set ProgramFiles
ProgramFiles=C:\Program Files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup will do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

[<InlineData("""#i " """, // Single quote
"input.fsx (1,4)-(1,5) parse error End of file in string begun at or before here",
"input.fsx (1,1)-(1,6) interactive warning Invalid directive '#i '")>]
"input.fsx (1,1)-(1,3) interactive warning Invalid directive '#i '")>]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a bug fix (see pars.fsy) for the range reported for bad hash errors. Could be separated

@auduchinok
Copy link
Member

auduchinok commented Dec 17, 2020

Editing/analysis: when editing, we will attempt to query “dotnet --version” by running it in the directory of the script, to work out the relevant SDK

Could we have an option to set the directory dotnet via FSharpChecker/GetProjectOptionsFromScript, please? We provide a way to choose different fsi versions, and it would be great to align it with the in-editor tooling.

@baronfel
Copy link
Member

I'd second @auduchinok's remark. Having a way to set this sort of internally-probed state is a great escape hatch. Right now in FSAC we completely overwrite the refs derived from FCS for scripts based on custom logic due to some assumptions that didn't hold in our specific scenario, and one of those assumptions used to be that the runtime of the process hosting FCS (.netcoreapp2.0 at the time) was the same as the TFM that scripts would run as (which was often 3.0/3.1). This mismatch led us to have to replace the ref list entirely. Having a way to provide that as a more structured input would mean we could delete lots of probing code and get back on a supported/shared mechanism.

@dsyme
Copy link
Contributor Author

dsyme commented Dec 18, 2020

@auduchinok @baronfel Yes in principle, we should discuss in a separate PR I think.

We provide a way to choose different fsi versions, and it would be great to align it with the in-editor tooling.

I think the appropriate thing is to pass an optional SDK directory to GetProjectOptionsFromScript

So I have added an sdkDirOverride argument to GetProjectOptionsFromScript for this purpose. It must be an actual SDK directory not a lower bound on one (if you need to resolve a lower bound+rollforward policy then please create a fake directory, with global.json with the right entries, and run dotnet --version in it).

Note that the sdkDirOverride override flag will always override any existing global.json next to the script. That may be too strong and you may prefer to respecting a global.json in preference to any user setting about FSI version.

Note also global.json next to a script are only respected if you start dotnet fsi in the same directory as the script. It's the startup directory that matters.

one of those assumptions used to be that the runtime of the process hosting FCS (.netcoreapp2.0 at the time) was the same as the TFM that scripts would run as (which was often 3.0/3.1)

Yes I was just sorting through a similar problem. My understanding is that GetProjectOptionsFromScript will now work for explicit SDKs specs (specified via an sdkDirOverride argument, or a global.json next to or above the script, else the latest installed SDK on the machine). The packs, TFM etc. are computed from the selected SDK.

@baronfel I think you're also saying you want to specify a SDK directory in some situations, e.g. where there is no global.json for the script and you know your execution tooling (eg F# Interactive) is designed to execute scripts for one particular SDK version.

@dsyme
Copy link
Contributor Author

dsyme commented Dec 18, 2020

Random unrelated test failure, could indicate a race condition

[xUnit.net 00:01:22.34]     FSharp.Core.UnitTests.Control.MailboxProcessorType.Receive handles cancellation token [FAIL]
  Failed FSharp.Core.UnitTests.Control.MailboxProcessorType.Receive handles cancellation token [5 s]
  Error Message:
   System.NullReferenceException : Object reference not set to an instance of an object.
  Stack Trace:
     at FSharp.Core.UnitTests.Control.MailboxProcessorType.Receive handles cancellation token() in /Users/runner/work/1/s/tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Control/MailboxProcessorType.fs:line 102

@dsyme dsyme merged commit cee7bb4 into main Dec 18, 2020
@dsyme dsyme changed the title Support .NET Core F# Interactive scripting in VS, also explicit framework specifications in scripts Support .NET Core F# Interactive scripting in VS Dec 18, 2020
@cartermp cartermp added this to the 16.9 milestone Dec 18, 2020
@KevinRansom KevinRansom deleted the feature/fxspec branch June 30, 2021 19:09
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Jan 26, 2022
Support .NET Core F# Interactive scripting in VS, also explicit framework specifications in scripts
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.

9 participants