Fsharp: [15.3 pre] loading in VS / publish fails if FSharp.Compiler.Tools is imported

[0x53A]

Edit2: everything works, thanks @KevinRansom

_Edited_

I have tried to test if / how a project using paket works in the new world of dotnet 2, comparing C#, F#, and the previous F# sdk (FSharp.NET.Sdk). I have also tested if using F# without paket works.

Here are four branches, one for each test: https://github.com/0x53A/dotnet-tests.

Results:

• paket works with C# and Microsoft.NET.Sdk: https://github.com/0x53A/dotnet-tests/tree/csharp+paket
• paket works with F# when using the 1.x sdk (FSharp.NET.Sdk): https://github.com/0x53A/dotnet-tests/tree/fsharp+paket+FSharp.NET.Sdk
• paket ~does NOT~ does work with F# and Microsoft.NET.Sdk: https://github.com/0x53A/dotnet-tests/tree/fsharp+paket
• using F# with Microsoft.NET.Sdk and nuget does work: https://github.com/0x53A/dotnet-tests/tree/fsharp+nuget

The paket version I have tested this with is 5.6.12.

It is kind of funny that C# seems to work flawlessly with paket.

[0x53A]

I am mostly interested in how to find out the cause of "Project file is incomplete. Expected imports missing." That may be something that could be changed on the paket side. (It would be even better if it worked without changes on the paket side).

[0x53A]

I assume it is an issue in the F# implementation of Microsoft.NET.Sdk.

The equivalent C# project does work without issues:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFramework>net461</TargetFramework>
  </PropertyGroup>

  <Import Project="..\.paket\Paket.Restore.targets" />

</Project> 

full project: paket-csharp-test.zip

[forki]

@enricosada probably knows

[cartermp]

This does seem like a bug given that the same import works with C# projects. cc @KevinRansom

[KevinRansom]

@0x53A
how is this file "...paket\Paket.Restore.targets" expected to get on the machine?

This is a dotnet sdk csproj with the paket import on a cleanly installed vs2017:

I believe the FSharp.Net sdk may have explicitly added support for paket.

[forki]

No there was no support build into the sdk.

Paket.Restore.targets comes from paket itself. When you call install command.

[0x53A]

Edited, please see initial post

[0x53A]

@KevinRansom Attached is a working (well, at least for me) C# project

paket-csharp-test.zip

[KevinRansom]

@0x53A
On my machine paket.exe wasn't in the .paket directory. Copying it to the .paket directory made it work fine.

Looking at the targets that seems to be the case. Does that work for you?

[0x53A]

@KevinRansom Sorry, my initial post was a bit sloppy, just copy-pasting project xmls.

I have created a repository, with one branch for each test.

Can you please test if this works on your machine? My result is in readme.md https://github.com/0x53A/dotnet-tests/tree/fsharp+paket

[cartermp]

Should there be a paket.exe in both the root director and the .paket folder?

[0x53A]

it looks like it doesn't matter, as long as there is one in either the root or the .paket.

[cartermp]

I can repro:

• Fails with @0x53A's reported error in branch fsharp+paket
• Works with the csharp+paket branch

[KevinRansom]

@0x53A

git clone https://github.com/0x53A/dotnet-tests.git git checkout -b remotes/origin/fsharp+nuget
load into VS

I note that you are in fact using an dotnet cli 2.1.0 build you seem to be using the master branch of dotnet cli, which may not be the right one.

I think this is the branch to use https://github.com/dotnet/cli/tree/release/2.0.0 , let me try it out.

Kevin

[0x53A]

@KevinRansom Can you please also check the branch fsharp+paket?

[cartermp]

I repro'd this with that exact same branch of the CLI. It _appears_ that the F# bits of the .NET Core SDK is not recognizing the Paket targets, but the C# parts of the SDK are. This is likely on our end.

[KevinRansom]

It could be ours too ... we don't integrate with the sdk in exactly the same way as C#. Hmmm I got edited ....

[cartermp]

Also, as I learned for the CLI: the last coherent version should be used. The current download in the 2.0.0/release branch will error out when restoring Microsoft.NETCore.App...

[KevinRansom]

Looks like you are importing :
FSharp.Compiler.Tools.props

That is not a good thing, since it tells us the sdk that we are using the FSharpNet.sdk.

That file automatically sets these variables:
<_FscTaskAssemblyPath_netcoreapp1_0>$(MSBuildThisFileDirectory)netcoreapp1.0/FSharp.Build.dll</_FscTaskAssemblyPath_netcoreapp1_0> <_FscToolFullPath_netcoreapp1_0>$(MSBuildThisFileDirectory)netcoreapp1.0/fsc.dll</_FscToolFullPath_netcoreapp1_0> <!-- specific .NET Framework fsc --> <_FscTaskAssemblyPath_net>$(MSBuildThisFileDirectory)../tools/FSharp.Build.dll</_FscTaskAssemblyPath_net> <_FscToolFullPath_net>$(MSBuildThisFileDirectory)../tools/fsc.exe</_FscToolFullPath_net>
If these variables are set we get out of the way and assume you are using the FSharpNet.sdk.

Somewhere in the paket magic somone is referencing the FSharp.Compiler.Tools nuget package. I'm afraid that package has a tools....props file that sets the values described above.

Kevin

[KevinRansom]

It's the paket.references file

Newtonsoft.Json

You don't need:
FSharp.Core
FSharp.NET.Sdk

The SDK takes care of them.

[0x53A]

Wow. Ok, now I feel a little bit stupid. I guess I should not just have blindly copy-pasted files from somewhere...

Thank you!

I can confirm that removing that package fixes both issues: publish and loading in vs.

[KevinRansom]

Lol ... you are so funny ... and that is a great GIF. It never occurred to me at all that you were stupid.

[forki]

Implicit referencing of fsharp.core from the sdk is probably a recipe for disaster in terms of resolution soundness. I have to carefully evaluate that but my gut feeling is very bad here.

[Krzysztof-Cieslak]

Aside of any technical problems (I wonder how will Paket behave for packages that has FSharp.Core as indirect dependencies), adding FSharp.Core by Paket was a standard Community has been using for many years. We can expect lot of issues similar to this one if for some reason there is conflict between SDK and adding FSharp.Core with Paket.

[forki]

Yes that is the problem I fear.

[0x53A]

@forki @Krzysztof-Cieslak This is from my limited testing:

• referencing FSharp.Core with paket does not cause any failures - I only removed FSharp.Compiler.Tools, and everything seemed to work at a first glance.

• not referencing FSharp.Core with paket does not cause any failures - in my test it then referenced a dll with version 4.4.1.0.

• you can't downgrade (*) FSharp.Core with paket, I locked 4.0.0.1 (from 2015!), used the Result type, and everything still compiled!

• no idea if you could upgrade FSharp.Core, afaik the project system references 4.2.*, but there is no 4.3 yet. You would need to create a custom FSharp.Core package with version >= 4.3, and check whether it unifies the versions, taking the highest, or ignores and overrides it.

• (*) I could NOT get the flags from https://github.com/fsharp/fslang-design/issues/193 to work, should they? My version: dotnet --version: 2.1.0-preview1-006784.

---

The following project compiled, I would have expected it to fail (__no paket used this time, really only these two files in the directory__):

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFramework>net461</TargetFramework>
    <DisableAutoFSharpCoreReference>true</DisableAutoFSharpCoreReference>
  </PropertyGroup>

  <ItemGroup>
    <Compile Include="Library.fs" />
  </ItemGroup>

</Project>

```F#
namespace paket_fsharp_test

module Say =
let hello name =
let r = Result.Ok name
printfn "Hello %s" (match r with Ok n -> n | Error x -> x)

This project compiled and published OK, I would have expected it to error out because I don't reference ANY FSharp.Core.

-----------------------

With the same source file, the following project file also compiled. I would have expected it to reference a really old FSharp.Core, and error out, because there is no ``Result`` type yet:


```Xml
<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFramework>net461</TargetFramework>
    <TargetFSharpCorePackageVersion>4.0.0.1</TargetFSharpCorePackageVersion>
  </PropertyGroup>

  <ItemGroup>
    <Compile Include="Library.fs" />
  </ItemGroup>

</Project>

(4.0.0.1 is from 2015: https://www.nuget.org/packages/FSharp.Core/4.0.0.1)

---

[forki]

Ok with the sdk pinning dependencies and no way to override that is really really really bad. I think this is a serious flaw in how sdks work. With pinning fsharp.core we will see big issues - probably things C# land will not have to this degree since they don't have a corresponding thing.

[0x53A]

@forki

Looking at https://github.com/Microsoft/visualfsharp/blob/master/src/fsharp/FSharp.Build/Microsoft.FSharp.NetSdk.targets#L56, the properties seem to actually be FSharpCoreImplicitPackageVersion and DisableImplicitFSharpCoreReference.

1) testing FSharpCoreImplicitPackageVersion

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFramework>net461</TargetFramework>
    <FSharpCoreImplicitPackageVersion>4.0.0.1</FSharpCoreImplicitPackageVersion>
  </PropertyGroup>

  <ItemGroup>
    <Compile Include="Library.fs" />
  </ItemGroup>

</Project>

=>

PS C:\source\dotnet-tests\fsharp-no-paket-no-fsharp-core> dotnet publish
Microsoft (R) Build Engine version 15.3.406.54721 for .NET Core
Copyright (C) Microsoft Corporation. All rights reserved.

C:\source\dotnet-tests\fsharp-no-paket-no-fsharp-core\Library.fs(5,17): error FS0039: The value, namespace, type or module 'Result' is not defined. [C:\source\dotnet-tests\fsharp-no-paket-no-fsharp-core\fsharp-test.fsproj]
C:\source\dotnet-tests\fsharp-no-paket-no-fsharp-core\Library.fs(6,42): error FS0039: The pattern discriminator 'Ok' is not defined. [C:\source\dotnet-tests\fsharp-no-paket-no-fsharp-core\fsharp-test.fsproj]

Which is good!

---

2) DisableImplicitFSharpCoreReference

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFramework>net461</TargetFramework>
    <DisableImplicitFSharpCoreReference>true</DisableImplicitFSharpCoreReference>
  </PropertyGroup>

  <ItemGroup>
    <Compile Include="Library.fs" />
  </ItemGroup>

</Project>

publishes OK, which is unexpected, because there is no FSharp.Core.

[forki]

DisableImplicitFSharpCoreReference feels like a big hack around the real issue. But good thing that it's there. We should set it in paket.restore.targets (PRs welcome in paket) because it should really be the default

[0x53A]

3) DisableImplicitFSharpCoreReference and paket

paket.dependencies:

nuget FSharp.Core 4.0.0.1

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <TargetFramework>net461</TargetFramework>
    <DisableImplicitFSharpCoreReference>true</DisableImplicitFSharpCoreReference>
  </PropertyGroup>
  <ItemGroup>
    <Compile Include="Library.fs" />
  </ItemGroup>

  <Import Project=".paket\Paket.Restore.targets" />
</Project>

=>

C:\source\dotnet-tests>dotnet publish
Microsoft (R) Build Engine version 15.3.406.54721 for .NET Core
Copyright (C) Microsoft Corporation. All rights reserved.

C:\source\dotnet-tests\Library.fs(7,17): error FS0039: The value, namespace, type or module 'Result' is not defined. [C:\source\dotnet-tests\paket-fsharp-test.fsproj]
C:\source\dotnet-tests\Library.fs(8,42): error FS0039: The pattern discriminator 'Ok' is not defined. [C:\source\dotnet-tests\paket-fsharp-test.fsproj]

which is good.

---

So it looks like both properties work as expected. It seems that in __2)__ the compiler automagically referenced some FSharp.Core, because none was explicitly referenced.

[forki]

It should never be do that. But hey we at least set sane defaults in paket.

[0x53A]

@forki I don't think we should always set DisableImplicitFSharpCoreReference in paket, because then there is a big different between a vanilla project and a paket project.

1) vanilla with no explicit core reference: There is an implicit reference to the nuget package.

2) paket with no explicit core reference: There is no nuget package reference, BUT the compiler seems to automagically reference ANY ambient FSharp.Core version, which may break in unexpected ways.

So I think we should check if the user references FSharp.Core (explicitly, or through a transitive dependency). If yes, set DisableImplicitFSharpCoreReference and add a hard reference. If no, do not set anything.

---

Basically my concern is that there are two different magic references:

1) the SDK auto-references some FSharp.Core nuget package
2) the compiler will auto-reference some FSharp.Core DLL.

and __2)__ is way worse than __1)__.

[forki]

I very strongly disagree. The default is just not a good choice. We should fix that.

(and let convert command add the FSharp.Core reference in references files and deps files)

[baronfel]

Practically speaking, if paket only conditionally added the disablement flag when Fsharp.core is directly or transitively referenced, you're still going to hit near total usage of that attribute. It's been prevailing best practice to expose a direct reference to Fsharp.Core in many of the most used packages in the ecosystem.

[forki]

And more generally speaking: a sdk should never ever override manual given versions. Never. I mean it can fail if it thinks this is completely broken. That's fine. But seriously if it uses it's own resolution then everything is lost

[forki]

And this discussion is in the wrong repo again and belongs to places where people accuse me of spreading FUD when I make these kinds of statements. ;-(

[0x53A]

I agree with the second-last comment, @forki , the question is if it is possible at the stage where the F# sdk is implemented.

Optimally, it should either respect the user reference, and continue, or fail with a conflict error message and clear guidance how to disable the implicit import.

[forki]

@0x53a there is no F# sdk anymore. It's one big Microsoft sdk.

[0x53A]

@forki I meant the F# part of the microsoft sdk. This magic stuff is implemented here:

https://github.com/Microsoft/visualfsharp/blob/master/src/fsharp/FSharp.Build/Microsoft.FSharp.NetSdk.targets#L60

[forki]

Someone should PR it out....

[forki]

... But that would not fix the underlying issue that earlier definitions (from sdk) win over manual definitions

[0x53A]

@forki any idea how the C# team handles https://www.nuget.org/packages/Microsoft.CSharp?

Or is that something completely different?

[forki]

I guess that might be similar. Dunno

Am 15.07.2017 7:31 nachm. schrieb "Lukas Rieger" [e-mail]:

@forki https://github.com/forki any idea how the C# team handles
https://www.nuget.org/packages/Microsoft.CSharp?

Or is that something completely different?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/Microsoft/visualfsharp/issues/3335#issuecomment-315549631,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADgNFrocPmgkS0uDwzbO8fK9AU_gSs_ks5sOPdUgaJpZM4OYpTB
.

[0x53A]

I found https://github.com/dotnet/sdk/issues/594, which talks about re-adding Microsoft.CSharp, so they may not do it at all at the moment? Do you know who we could ask?

[0x53A]

Also, what I find even a bit more suspect is the System.ValueTuple handling, because that is a topic that should also be relevant in C# land (or even more), so I think F# should either handle it the same way, or not at all.

[0x53A]

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFramework>net461</TargetFramework>
  </PropertyGroup>

</Project>

```C#
using System;

namespace csharp_test
{
public class Class1
{
public static (int,int) Test() => (1,2);
}
}

``dotnet publish``

Class1.cs(7,17): error CS8179: Predefined type 'System.ValueTuple`2' is not defined or imported [C:\source\dotnet-tests\csharp-test\csharp-test.csproj]
```

[0x53A]

@forki Thinking about this a little more, here is my preference for paket:

Always set DisableImplicitFSharpCoreReference and DisableImplicitSystemValueTupleReference.
Error out if there is no reference to the FSharp.Core nuget package, either explicitly, or as a transitive.

Maybe we should move that discussion to the paket repo ...

[KevinRansom]

@0x53A @forki
Those switches are specifically to enable developers to override the defaults. So a paket converted project should set them if a developer wishes to use paket to override the behavior.

[forki]

@kevinransom yes but we're discussing if it should reference fsharp.core and valuetuple at all. I personally see only disadvantages of this. I think it should be removed right now before it hits a release and we have backwards compatibility issues.

Instead the dotnet template should explicitly reference it.

[0x53A]

I think the auto referencing is great if you just want to get started. Similar to fsc.exe source.fs working.

But if you have a medium sized project, you never want "something" to be referenced, you need to actually control what you reference.

So I kind of agree with forki. The downside is two more lines in the absolutely simplest project.
The upside is more control and less unexpected behavior.

[forki]

"The downside is two more lines in the absolutely simplest project."

that would be scaffolded by dotnet new - I don't see that as a downside.

[KevinRansom]

There are knobs to disable it. You can control when you need to, and not be bothered by it when you don't want to.

Set the disable flags and type what you want.

[forki]

Which knobs? The ones listed above do not seem to work.

Am 17.07.2017 7:20 nachm. schrieb "Kevin Ransom (msft)" <
[email protected]>:

There are knobs to disable it. You can control when you need to, and not be
bothered by it when you don't want to.

Set the disable flags and type what you want.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/Microsoft/visualfsharp/issues/3335#issuecomment-315821075,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADgND0yMD8xTfVwgffzraiaPjkppIrxks5sO5fhgaJpZM4OYpTB
.

[0x53A]

@forki, they do work correctly for the sdk, but unfortunately the compiler has a second magic reference resolution for fsharp.core. see https://github.com/Microsoft/visualfsharp/issues/3340

[forki]

yes that's what I meant. The sdk build should never succeed - it should probably pass an arg to compiler.

[0x53A]

@forki I went through https://docs.microsoft.com/de-de/dotnet/fsharp/language-reference/compiler-options, and there does not seem to be a way to disable that (unless there is an undocumented flag).

[forki]

@0x53A yeah it needs to pass a NEW arg ;-)

[0x53A]

sooo, do you want to work on the pr? :D

[forki]

no I actually just want it to work ;-)