Added test for checker.GetProjectOptionsFromScript

[ncave]

Re-did this PR to just have the test for checker.GetProjectOptionsFromScript
(test failing on netcore in 10.01).

[dsyme]

We really have to sort this stuff out....

I'm pretty certain we actually don't want this reference - and indeed I'm pretty certain that we never want any references to any "implementation" assemblies. No compilation should reference System.Private.CoreLib directly.

This is because of the following

• In the .NET Standard and all System.Runtime-based programming models, the canonical location for "Object" and "String" is System.Runtime.dll
• In the implementation assemblies, the canonical location for "Object" and "String" is System.Private.CoreLib.dll

If you mix implementation assemblies and .NET Standard assemblies in a single compilation, you will thus get a mixed up view of the world, where these types (and many others) have different canonical locations depending on how you look at things.

Perhaps I'm only imagining this problem, and we could adjust the F# compiler to consider System.Private.CoreLib a suitable canonical home (just as mscorlib is when running on .NET Framework). But currently the F# compiler is set up to consider either System.Runtime or mscorlib.dll as the canonical home for these types and I'm pretty sure it's not going to fly to have otherwise.

[ncave]

@dsyme I completely agree, but for some reason it doesn't work without explicitly referencing System.Private.CoreLib, as it cannot find System.Object. It used to work before but that seems to have changed with 1.0.0 somehow.
Looking at it with dnspy I thought it should work without an explicit reference since the .netcore mscorlib references System.Private.CoreLib and type-forwards to it, but it does not.

.net 4.6: System.Runtime -> mscorlib (GAC)
.netcore: System.Runtime -> mscorlib -> System.Private.CoreLib

[dsyme]

Could you give repro steps please? I think tests were passing on .NET Core without adding this, but it could be that specific scenarios (e.g. where explicit assembly references and --noframework are not given) are not being tested properly. Thanks

[ncave]

Yes, we were not properly testing this particular scenario on .NET Core. I've added a test for it, here is what the test error is when the reference to System.Private.CoreLib is not there:

Project35b error: <<<The type 'Object' is required here and is unavailable. You must add a reference to assembly 'System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e'.>>>
Project35b error: <<<A reference to the type 'System.Object' in assembly 'mscorlib' was found, but the type could not be found in that assembly>>>

[dsyme]

@ncave Do we expect this to be failing? thanks

[00:15:47] 1) Failed : Tests.Service.ProjectAnalysisTests.Test Project35b whole project errors
[00:15:47]   Expected: 2\nActual: 3
[00:15:47]   Expected: 2
[00:15:47]   But was:  3
[00:15:47] at FsUnit.shouldEqual[a](a x, a y)

[ncave]

@dsyme Yes, it shouldn't be failing but it is, look at the messages it prints when it fails. Perhaps as you said the assumeDotNetFramework flag needs to be set.

[ncave]

@dsyme Just adding the assumeDotNetFramework flag was not enough. I added back the missing reference from the original PR and now the test is passing again.

[dsyme]

We need to get to the bottom of why this is needed. AFAIK it should never be necessary to reference System.Private.CoreLib or any implementation assemblies which in turn refer to this.

I'll admit I find the facade/implementation assembly split bewildering in practice and the details always slip away. Do you know what's causing the need for this reference? Thanks

[ncave]

I don't, but as already mentioned in the original PR, there is a very unequivocal error message saying it's needed:

The type 'Object' is required here and is unavailable. You must add a reference to assembly System.Private.CoreLib
A reference to the type 'System.Object' in assembly 'mscorlib' was found, but the type could not be found in that assembly

Which kind of points to some issue in resolving forwarded types. Perhaps somebody from the dotnet/core team can shed a light?

[ncave]

I'm really hoping this issue will go away with .netstandard 2.0, we'll see.

[dsyme]

The type 'Object' is required here and is unavailable. You must add a reference to assembly System.Private.CoreLib

I believe this indicates that the compilation is referencing .NET Core private implementation assemblies, which is now considered invalid

In .NET Framework you used to be able to just reference any assemblies under the framework implementation directory c:\windows\... . But my understanding is that in the .NET Core world they want all compilations only use facade/reference/... assemblies. I understand why they are doing this, but it easily gets really confusing for dynamic compilation: lots of people in the .NET world just assume they can reference implementation assemblies as a last resort.

To be honest I'm not at all clear on where/how .NET Core dynamic code generation tools like fsi.exe are expected to find the appropriate reference assemblies (e.g. .NET Standard 2.0 assemblies). It almost seems to me that they expect these tools to download all the nuget packages etc. for a particular framework facade and use those, or find cached ones on the machine. That's a really different model and feels to me like it makes a whole lot of dynamic compilation scenarios much heavier, and tightly bound to specific packaging and compilation tooling which is under incredible flux. I suspect in practice people doing dynamic compilation are often going to keep trying to reference the implementation assemblies, just because it's dead simple and you can be guaranteed that they actually exist.

Because of this, when I last looked at fsi.exe on .NET Core, I ended up trying to teach the compiler to respect System.Private.CoreLib as a primary assembly reference for System.Object etc. But @KevinRansom told me that this is wrong, and compilation should never reference this assembly. I understand why, but I'm still confused about what they should reference.

Another way to put this is that I'm simply not sure what the correct values of DefaultReferencesForScriptsAndOutOfProjectSources should be for .NET Core, i.e. what the default references for scripts should be. It feels like they should be the locations of a bunch of .NET Standard 2.0 reference assemblies, but I don't think we can assume they are installed.

Anyway, to help us make progress, let's characterize the specific scenario carefully. I assume it is this:

• You are running the .NET Core version of a tool called Fable.EXE (or whatever its name)

• Your Fable.EXE uses the .NET Core version of FCS.dll to analyze an F#/Fable script or project, extract its Symbols and Exprs, and generate (print) Javascript code. Your Fable.EXE doesn't actually do any .NET codegen, but rather JS codegen. Your Fable.EXE may accept references to .NET and F# assemblies as part of its input specification.

• Your Fable.EXE prefers not to require any additional facade or reference assembly packages. For the moment it's happy to rely on those returned by evaluating these fragments on .NET Core, taken from DefaultReferencesForScriptsAndOutOfProjectSources:

yield Path.Combine(Path.GetDirectoryName(typeof<System.Object>.Assembly.Location),"mscorlib.dll"); // mscorlib
         yield typeof<System.Object>.Assembly.Location; // System.Private.CoreLib"
...

• We know these assembly references are "wrong" (they are private implementation assemblies) but they appear to "work" because no actual .NET code generation is done, and the F# compilation logic doesn't really care

• We know that it may be better to reference .NET Standard 2.0 reference assemblies or Fable-specific reference assemblies but that adds complexity to the tool and we don't know where to find these

Is that correct? BTW have you tried adding the above reference explicitly in otherArgs of GetProjectOptionsFromScript?

thanks
don

[ncave]

@dsyme It's not really about Fable, it affects any client that uses checker.GetProjectOptionsFromScript on .NET Core. But yes, adding the reference directly to the script works too.

As you suggested, I have removed the System.Private.CoreLib from the list of default references and added it only at the test script so the test can pass.

[KevinRansom]

So ... this is fine for a test but not as a long term solution. We will have a decent solution for this eventually.

A preferable approach for tests would be to use:
https://github.com/Microsoft/visualfsharp/blob/master/tests/scripts/fsci.fsx
with a project.json file identifying dependencies.

Kevin

[ncave]

@KevinRansom
For checker.GetProjectOptionsFromScript, is there a duplication of responsibility between assumeDotNetFramework=false and otherFlags = [| "--targetprofile:netcore" |] ?

[dsyme]

@ncave Not sure why we're got this - am retesting

[00:11:02] log  : Restoring packages for C:\projects\fsharp-compiler-service\src\fsharp\FSharp.Compiler.Service.MSBuild.v12\project.json...
[00:11:04] error: Unable to load the service index for source https://dotnet.myget.org/F/dotnet-buildtools/api/v3/index.json.
[00:11:04] error:   Response status code does not indicate success: 503 (Service Unavailable).
[00:11:04] info :   GET https://api.nuget.org/v3-flatcontainer/system.diagnostics.tracesource/index.json
[00:11:04] Running build failed.
[00:11:04] Error:
[00:11:04] System.Exception: Command failed with exit code 1

[ncave]

Closing as perhaps obsolete.