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.

Sign up for GitHub

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

Jump to bottom

Add use refassemblies switch to fsi #6585

Merged

KevinRansom merged 4 commits into dotnet:master from KevinRansom:userefassemblies

Apr 23, 2019

Member

KevinRansom commented Apr 19, 2019

With DotNet Core 3.0, we will be shipping a set of reference assemblies that matches the default target framework for the shipped dotnet cli. The NetSdk team are updating the compile time build to make use of these.

This PR adds an additional switch to FSI, that will have fsi compile against this set of reference assemblies by default rather than the implementation assemblies.

This is important because with versions of the cli revving frequently and supporting multiple platforms, we want to ensure that scripts run against a consistent platform. Implementation assemblies may have a bigger public surface, and one that is not consistent across platforms or between versions.

E.g ..

Internal.Runtime.InteropServices.WindowsRuntime.ExceptionSupport is a public internal implementation type, that would be risky to rely on.

> let t=typeof<Internal.Runtime.InteropServices.WindowsRuntime.ExceptionSupport>;;
val t : System.Type =
  Internal.Runtime.InteropServices.WindowsRuntime.ExceptionSupport

With this feature:

> let t = typeof<Internal.Resources.WindowsRuntimeResourceManagerBase.WindowsRuntimeResourceManagerBase>;;

  let t = typeof<Internal.Resources.WindowsRuntimeResourceManagerBase.WindowsRuntimeResourceManagerBase>;;
  ---------------^^^^^^^^

stdin(1,16): error FS0039: The namespace or module 'Internal' is not defined.

The dotnet framework contracts are defined by the framework definitions: DotNetStandard1.6 and up, and DotNetCoreApp1.0 and up and similar net461, net472 etc … not by the contents of any assemblies in the coreclr runtime directory.

Enabled by default use refs on DotNet Core 3.0 runtime
Always run scripts, against the netcoreapp version that matches the executing fsi.
It does not provide a downgrade switch. We can consider a downgrade switch for a future feature when the deployment story for these packages has stabilized.
To disable use --usesdkrefs-
If refs not supported, I.e. netcoreapp2.1 or 2.2 then falls back to implementation assemblies.
Desktop / Mono implementaions of fsi, do what they did before … use implementation assemblies, there is no switch.

Kevin

KevinRansom added 3 commits

April 17, 2019 16:29


          Add support for fsi against ref assemblies

28f3168


          temp

b83b0bd


          working

7d7effc

KevinRansom requested review from dsyme and brettfo

April 19, 2019 19:48

brettfo requested changes

View reviewed changes

src/fsharp/CompileOps.fs Show resolved Hide resolved

src/fsharp/CompileOps.fs Show resolved Hide resolved

src/fsharp/CompileOps.fs Show resolved Hide resolved

src/fsharp/CompileOps.fs

-                                 filename: string, sourceText:ISourceText, codeContext, useSimpleResolution: bool,
-                                 useFsiAuxLib, lexResourceManager: Lexhelp.LexResourceManager,
-                                 applyCommmandLineArgs, assumeDotNetFramework, tryGetMetadataSnapshot, reduceMemoryUsage): LoadClosure =
+                                   (ctok, legacyReferenceResolver, defaultFSharpBinariesDir,

Member

brettfo Apr 19, 2019

lots of trailing whitespace

src/fsharp/DotNetFrameworkDependencies.fs Show resolved Hide resolved

src/fsharp/DotNetFrameworkDependencies.fs

+                      else
+                          struct (Int32.Parse(elements.[0]), Int32.Parse(elements.[1]), Int32.Parse(elements.[2]), getSuffix)
+                  let versionCompare c1 c2 =

Member

brettfo Apr 19, 2019

Unit test this, because it could get nasty.

src/fsharp/FSComp.txt Show resolved Hide resolved

src/fsharp/service/service.fs Show resolved Hide resolved


          Fix fsi

d1cc1bf

KevinRansom merged commit d58159c into dotnet:master

dsyme reviewed

View reviewed changes

src/fsharp/DotNetFrameworkDependencies.fs

+                  let executionTfm =
+                      let file =
+                          try
+                              let depsJsonPath = Path.ChangeExtension(Assembly.GetEntryAssembly().Location, "deps.json")

Contributor

dsyme Apr 23, 2019

This is raising-then-catching exceptions on startup of desktop F# Interactive. SHould this code be executed for desktop fsi.exe at all?

dsyme reviewed

View reviewed changes

src/fsharp/DotNetFrameworkDependencies.fs

+                          path.StartsWith(appRefDir, StringComparison.OrdinalIgnoreCase)
+                      | _ -> false
+                  let getFrameworkRefsPackDirectory =

Contributor

dsyme Apr 23, 2019

This looks like it should be a function rather than a computation. Also have /// comments on all of these would be good please

dsyme reviewed

View reviewed changes

src/fsharp/DotNetFrameworkDependencies.fs

+                                          String.CompareOrdinal(suffix2, suffix1)
+                          with _ -> 0
+                  let executionTfm =

Contributor

dsyme Apr 23, 2019

Add a /// comment please, and avoid use of abbreviations like tfm (TargetFrameworkMoniker for those wondering)

dsyme reviewed

View reviewed changes

src/fsharp/DotNetFrameworkDependencies.fs

@@ @@ -34,6 +34,98 @@ module internal FSharp.Compiler.DotNetFrameworkDependencies @@
                                   None
                       with _ -> None
+                  // Compare nuget version strings

Contributor

dsyme Apr 23, 2019

Use a /// comment please

dsyme reviewed

View reviewed changes

src/fsharp/service/service.fs

    
            @@ -3209,10 +3209,10 @@ type FSharpChecker(legacyReferenceResolver, projectCacheSize, keepAssemblyConten
          
                      backgroundCompiler.KeepProjectAlive(options, userOpName)

                  /// For a given script file, get the ProjectOptions implied by the #load closure

                  member ic.GetProjectOptionsFromScript(filename, sourceText, ?loadedTimeStamp, ?otherFlags, ?useFsiAuxLib, ?assumeDotNetFramework, ?extraProjectInfo: obj, ?optionsStamp: int64, ?userOpName: string) = 

                  member ic.GetProjectOptionsFromScript(filename, source, ?loadedTimeStamp, ?otherFlags, ?useFsiAuxLib, ?useSdkRefs, ?assumeDotNetFramework, ?extraProjectInfo: obj, ?optionsStamp: int64, ?userOpName: string) =

Contributor

dsyme Apr 23, 2019

This is still showing as source in the diff - were you going to change it back to sourceText?

dsyme reviewed

View reviewed changes

src/fsharp/FSComp.txt

 ,tcCopyAndUpdateNeedsRecordType,"The input to a copy-and-update expression that creates an anonymous record must be either an anonymous record or a record"
 ,chkInvalidFunctionParameterType,"The parameter '%s' has an invalid type '%s'. This is not permitted by the rules of Common IL."
 ,chkInvalidFunctionReturnType,"The function or method has an invalid return type '%s'. This is not permitted by the rules of Common IL."
+              useSdkRefs,"Use reference assemblies for DotNET framework references when available (Enabled by default))."

Contributor

dsyme Apr 23, 2019

This is still showing DotNET on github though you said you'd change it to .NET in code review - was that done separately?

Contributor

cartermp Apr 23, 2019

It was not https://github.com/Microsoft/visualfsharp/blob/master/src/fsharp/FSComp.txt#L1461

@KevinRansom Let's queue up another PR here

Member Author

KevinRansom Apr 24, 2019 •

edited

Loading

Oh I fixed it, it must have got lost somehow. As did the )) on the end of the line.

KevinRansom deleted the userefassemblies branch

July 2, 2019 22:38

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet