Fsharp: Internal type check error seen in production code

[forki]

that looks weird

[forki]

note: I don't even own a E: drive. @KevinRansom is that yours?

[cartermp]

Yeah it shouldn't be listing the file where this is happening, though I suppose that does make diagnosing the issue slightly easier

[forki]

@cartermp the path is just funny side effect - I'm more worried why compiling my user code is raising internal error in async.fsi which then gets emitted as warning

[matthid]

I think everyone with latest fable is seeing this currently, correct?

[forki]

it's raised on non-fable projects

[BillHally]

Simple repro

Stick the following in a .fs file and compile it with fsc while referencing FSharp.Core 4.5.1:

module A

let a = async { return 0 }

Change the reference to 4.5.0 instead, and it compiles without warnings.

[cartermp]

See #5378 - considering the API diff for F# 4.5.1 and F# 4.5.0 has nothing to do with async, I'm puzzled as to why this would be a problem. cc @dsyme @KevinRansom

[cartermp]

Okay, so @KevinRansom and I chatted and this may actually be a forwards-compatibility issue. Async is now different due to inlining certain members for better stack traces. But older compilers are not tested with this behavior. And confusingly, although the API surface area for FSharp.Core 4.5.1 does not have those changes, it actually does contain the inlining work for stack traces. That is, FSharp.Core 4.5.0 was not shipped when that work was merged in. Hence the difference.

Best course of action is to only use 4.2.3 or 4.5.0 with your older compiler until F# 4.5 ships.

[forki]

that should actually mean we should unlist latest fsharp.core

[KevinRansom]

@forki,

we need to talk about this some more. We don't really guarantee, that a particular released compiler will work with future fsharp.core dll's, although we do most definitely try really, really … really hard to make it work.

It seams to me that we need to
1: Prove that the theory above is accurate, it's just a theory so far.
2: See if there is an approach we can use to solve the issue.
3: Figure out what to do.

This should not hurt any developers who are using standard templates, since we explicitly select an FSharp.Core version or range that they support.

Developers who specify wild card nuget version's need to throttle back to 4.2.3 until we have an understanding about what is going.

Kevin

[forki]

a) standard templates are mostly irrelevant in practice
b) if I understand correctly there is no release compiler that works with latest fsharp.core
c) it's no wildcard nuget version (I'm not sure that even is a thing outside of Redmond) - it's just latest. You will get that version if you freshly install fsharp.core or update the FSharp.Core

[forki]

I think safest bet for now is doing the opposite. Instead of letting the user downgrade we should consider to unlist - at least until we figure out the root cause. This will be safer because only very small part of the user base will read this issue

[forki]

Argh just reread. It's probably sounding too impolite. That wasn't intended. It never is. Please add proper amount of smileys to make it appropriate for British and American citizens ;-)

[KevinRansom]

@forki. Lol … I have worked with you for 5 or 6 years now my friend, your directness is always filtered with added of smilies.

The tough part for me is figuring out how to say "no we can't do that yet", until we understand what is going on and have determined what we are going to do about it; if we can't figure out how to fix it, then we'll likely be reverting the comprehension refactoring that Don did. However, that is still just a theory right now.

Until that is the case latest is going to have to mean 4.2.3 for all projects built on old compilers.
latest can comfortably mean 4.5.1 for anything based on preview compilers, which is more or less exactly what the existing templates do.

[forki]

The good thing is: it's only a warning. Runtime behavior seems to be good.

Kevin Ransom (msft) [e-mail] schrieb am Di., 24. Juli
2018, 20:10:

@forki https://github.com/forki. Lol … I have worked with you for 5 or
6 years now my friend, your directness is always filtered with added of
smilies.

The tough part for me is figuring out how to say "no we can't do that
yet", until we understand what is going on and have determined what we are
going to do about it, if we can't figure out, how to fix it, then we'll
likely reverting the comprehension refactoring that Don did. However, that
is still just a theory right now.

Until that is the case latest is going to have to mean 4.2.3 for all
projects built on old compilers.
latest can comfortably mean 4.5.1 for anything based on preview compilers,
which is more or less exactly what the existing templates do.

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

[cartermp]

standard templates are mostly irrelevant in practice

I've seen little evidence that this is true beyond the subset of F# programmers who happily deviate from things that are guaranteed to be tested together, and little to no evidence that the bulk of F# programmers do this.

I'm sympathetic to the fact that some people will take an update to something if they see it, or that they will specify their version with a wildcard. But we can't keep them from doing that without refusing to release new things until _everything_ is shipped and available. But that is also at odds with the following:

• F# must ship across multiple places that do not guarantee aligned schedules
• Tools that depend on NuGet packages may not support something like "flipping a switch" from a non-preview package to a released package (though this can be investigated further)
• Specifying a wildcard can make sure that at release someone gets the version of something that is tested with their compiler, but then sends people down into the danger zone if any further update is pushed

If we did not ship within Visual Studio then this wouldn't really be a problem. But we do, so it is. 😡

As for unlisting - that's a possibility, but this timeline:

• t0 --> push 4.5.1
• t1 --> unlist 4.5.1, optionally push a 4.5.2 with the older binary in it
• t2 --> push 4.5.3 (or 4.5.2 if previous option not taken) that corresponds with F# 4.5 language

Doesn't actually solve the problem. It just keeps _this particular instance_ of a forwards-compatibility issue (which we do not support and consider incidental if it works!) from being a problem ... provided an older binary is pushed.

That said, I think that if we can demonstrate that 45.1 renders existing F# code utterly useless, then unlisting needs to happen for this case.

[dsyme]

Ah OK. Here's my guess.

1. FSHarp.Core 4.5.0.0 includes metadata which says the type of the "this" pointer for the "OnSuccess" method is inref<AsyncActivation<T>>.

2. New compilers understand that inref is a form of byref

3. However the dev 15.7 compiler doesn't know that "inref" is a kind of "byref" and so this line https://github.com/Microsoft/visualfsharp/blob/dev15.7/src/fsharp/IlxGen.fs#L787 doesn't pass "isByrefTy", so it counts two arguments of byref<AsyncActivation<T>,In> instead of one of AsyncActivation<T>

If this is just a warning well that's good. I'm somewhat surprised we don't hit this in other ways. I think that's because for nearly all purposes the old compilers will consider "inref" to by a ILType.Byref because of this definition https://github.com/Microsoft/visualfsharp/blob/master/src/fsharp/FSharp.Core/prim-types.fsi#L847 . However it could be the case that this cases other issues with old compilers.

[KevinRansom]

@forki,

Okay, it seems as if we generate a signature that generates a warning in older compilers. This can impact user code, as well as FSharp.Core.

The fix may be to modify the signature that we emit for optprof/sigdata files to use byref, rather than inref and outref (Which are 4.5 compiler+ only).

Urgency, is not now, we can try to do this for 15.9. In the meantime developers have 3 different work arounds:

1. Update compiler to released 4.5 compiler, when available.
2. Pin FSharp.Core nuget package to 4.3.4
3. Ignore this warning.

/cc @dsyme, @cartermp, @Pilchie

[forki]

@cartermp if you push a new FSharp.Core then a lot of people will update. that just how nuget works.

If the compiler is not working with that - then we need to fix or at least unlist until we have a fix. This is pretty much standard practice across the ecosystem. I'm really confused by that comments above.

[forki]

Btw I'm not saying we should stop releasing stuff until it's proven to be safe as you stated above. I'm just saying that if we have evidence for things not working properly, then we should consider unlisting.

[forki]

if you are creating a new project in VS and go to "Manage NuGet packages" it's telling you to update. It's completely irrelevant if you started with a VS template or not. That's what I tried to say above.

we generate a signature that generates a warning in older compilers

The latest released compiler is binary incompatible with the latest FSharp,Core. We are not talking about random older compilers.

If you don't see it as urgent then ok, but please don't tell me only weirdos "who happily deviate from things that are guaranteed to be tested together" are impacted.

[isaacabraham]

The issue is that this is a problem that could impact any and all F# users, right now. There's no way for them to know about this github issue, so whilst it's great that there are some workarounds here, it won't help the majority of people that are potentially affected by it.

I now have to tell my team on all our projects not to update, and any new projects we have to explicitly pin FSharp.Core. This is not the pit of success.

Why not just unlist, even if just temporarily?

[cartermp]

Hmmm, actually I think the ship has long sailed on this now.

Assuming an unlisting were to occur, we would break F# in VS 15.8 Preview 5 (which is an RC) with no exception. It would also break F# support for .NET Core 2.1.400.

Today, our payload is set up such that Preview 5 will have the right compiler referencing the right package and all features and bug fixes in. Unlisting the package will break this release for any new F# project where someone wanted to try out F# 4.5 features and give feedback. Insert joke about F# + an RC of Visual Studio here.

Changing this payload is simply not possible as its release is imminent, and something as small as a version number change in F# templates will set that back by at least a day, assuming that every single other part of the product is deemed completely ready for the RC.

However, assuming we were to unlist, then we would have two remaining options:

• Get last-last-last-minute approval to update VS 15.8 RTW to use a newer FSharp.Core in templates, thus shipping F# 4.5 in Visual Studio for .NET Framework (classic) projects, and leaving it broken for .NET Core 2.1.400 and any new .NET Core F# projects in Visual Studio.
• Get last-last-last-minute approval to update VS 15.8 RTW to yank out F# 4.5 for .NET Framework (classic) projects, and leaving a broken F# 4.5 for .NET Core 2.1.400 and any new .NET Core F# projects in Visual Studio.

I see the above options as far more frustrating for everyone than if we just keep things as-is, which is a state that can easily be dealt with by downgrading the package to what is guaranteed to work with F# 4.1.

Unless the following are true:

• This can break running code today; i.e., the warning wasn't just a warning but an indicator of a legitimate issue that destroys a production system
• Downgrading this package is seen as worse for a large number of people than releasing a broken (or half-broken) F# 4.5

Then I don't think we will consider unlisting the package.

@isaacabraham regarding this:

I now have to tell my team on all our projects not to update, and any new projects we have to explicitly pin FSharp.Core. This is not the pit of success.

This has never been the pit of success. It is incidental that a newer FSharp.Core works with an older compiler version, as we do not support forwards-compatibility, despite it being something we'd like to have work. I would strongly encourage any new projects that do not use our templates to pin their FSharp.Core version, with updates to the package taken judiciously as they would any other package upgrade.

[cartermp]

Actually, I honestly don't remember or know if the default NuGet client in VS 15.8 and/or .NET Core 2.1.400 will still just pick an unlisted version and be happy, thus making things okay (even though it's depending on an unlisted version). So maybe F# wouldn't be broken in those cases? That gives me serious heebie jeebies though

[cartermp]

Actually, I just remembered that Preview 5 would be fine if we unlisted. So it would really just be the release that's questionable.

[matthid]

@forki I think we now slowly realize why the VisualF# team was so hesitant to move FSharp.Core to NuGet. It just seems to be too tightly coupled to the compiler.

This is n unfortunate situation but as long as we don't see any reports/proof that this is more than a "warning".... It is a bit unsatisfying to be in a situation like this where you can't move in any direction without blowing something up...

[forki]

I would strongly encourage any new projects that do not use our templates ...

that template gets updated via NuGet VS GUI - there is absolutely nothing special or safer about the template

anyway it seems I'm fighting with windmills and Preview versions in VS are more important than regular released stuff that real people work with. Closing this since it doesn't lead to anything

[isaacabraham]

@cartermp I think perhaps this issue speaks to larger concerns, which @matthid has touched upon - whereby some parts of F# Core are inherently coupled to specific versions of the F# compiler, and some parts are perhaps not. I vaguely remember after F#4.0 came out and we were still on F#3.x, we held off moving to new versions of FSharp.Core for this sort of reason.

I don't know what the answer is (or if there even is one) but I do think that this is worth further "thinking about" e.g. splitting out some parts of FSharp.Core to other packages which would allow them to evolve independently?

[matthid]

Honestly, we just introduced F# (and fable) in our project and this is a bit of an annoyance, if people ask what's going on with the F# compiler. This is definitely not the best experience :(

Also if you are releasing for pre-releases why don't you use prereleases for that?

[cartermp]

I initially pushed for a preview release before the switch for templates, but I believe at the time it was deemed too risky approval-wise to go back and update them to point to a non-prerelease version unless we took it as a servicing fix. The thing to keep in mind is that even an update like that will set back a release of VS by at least a day (assuming all other parts of the product are ready) if it comes too late.

Timing-wise this particular release sucks because it's also the first where we only depend on the package. Future releases won't need to additionally insert those changes, so an update during a preview "wave" from prerelease to release is not too difficult, assuming that preview wave is long enough.

[TIHan]

In summary, the problem here is that a newer FSharp.Core gets released that is not compatible with the latest released compiler. This is due to an imminent release of a new version of VS with a newer compiler that requires that newer version of FSharp.Core. The reason why this is a problem is that users see that they can update FSharp.Core (and they will) and then experience funky behavior; in this case odd warnings.

I can certainly agree this is not the user experience that we want, regardless of our reasons for doing what we are doing.

I'm not familar with NuGet that well, but from what I'm reading, unlisting really seems like the logical choice here. Our templates can still refer to the package and download without a problem and users with existing F# projects will not see they need to update to that version. @cartermp are there any other side effects to unlisting that I'm not aware of? Can we not also undo the unlisting at some point after we ship? One downside is that it doesn't fix wildcards though.

[matthid]

If this indeed not a supported scenario why is the compiler not emitting a warning if it encounters a newer FSharp.Core than it supports? At least people would know what to do (and it would be a warning as well if stuff "just works" anyway).

[forki]

@tihan independent of the issue here. In general unlisting is considered a
relatively safe operation since the package is still available for users
that depend on exactly that version. In theory it only affects user
triggered updates and new installations. In terms of paket this would be
paket update and paket add. These commands would try to come up with a
sound resolution that exclude all unlisted package versions. If there is no
such resolution then paket would retry and also accept unlisted packages.
For normal paket restore we would always keep restoring the version that's
in the lock file. So we would not break existing builds.

For some extent this used to be true for nuget.exe as well. But there is a
long standing bug which is still not fixed AFAIK. Nuget.exe did ignore the
unlisting flag since the dnx times which means even new installations may
bring it in. Which basically means that unlisting is not effective for
these users. But if you want to see it positive: it also does not make
matters worse. It's just completely ignorant here.

If we think about the future then we want to consider the path that
@matthid mentioned and push prerelease versions (aka versions that are
needed for VS previews to work) as explicit prerelease nuget packages.

In that case both clients ignore the version for update and new
installations until told otherwise. The "template" could still depend on
the concrete prerelease version. But to make that more explicit we should
move away from the implicitly referenced nuget package and make and
explicit reference in the fsproj. So users that create a new project from
the template understand what dependency they have - especially if it's
created from the VS preview bits.

Will Smith [e-mail] schrieb am Mi., 25. Juli 2018, 19:45:

In summary, the problem here is that a newer FSharp.Core gets released
that is not compatible with the latest released compiler. This is due to an
imminent release of a new version of VS with a newer compiler that requires
that newer version of FSharp.Core. The reason why this is a problem is that
users see that they can update FSharp.Core (and they will) and then
experience funky behavior; in this case odd warnings.

I can certainly agree this is not the user experience that we want,
regardless of our reasons for doing what we are doing.

I'm not familar with NuGet that well, but from what I'm reading, unlisting
really seems like the logical choice here. Our templates can still refer to
the package and download without a problem and users with existing F#
projects will not see they need to update to that version. @cartermp
https://github.com/cartermp are there any other side effect to unlisting
that we are not aware of? Can we not also undo the unlisting at some
point after we ship?

—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
https://github.com/Microsoft/visualfsharp/issues/5369#issuecomment-407838302,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADgNBZJDbrS9cBLYq47O6IdW1rgAx9Aks5uKK6wgaJpZM4Vakuk
.

[KevinRansom]

I would like to leave this open for a while to track this. I am not certain that we have drawn to a conclusion.

For sure updating from VS2017.7 to VS2017.8 isn't one where we can tolerate a high level of breaking changes. And a library developer would certainly intuit that merely rebuilding code that compiled with the VS2017.7.5 compiler should work everywhere that it would when compiled with the VS2017.7 compiler.

So we need to work this out, right now, I think the current behaviour is incorrect. However, the work arounds are all relatively straigtforward.

Rest assured we will arrive at a defensible outcome.

Kevin
Kevin

[cartermp]

@matthid Because we haven't implemented it yet 🙂

@KevinRansom and I have talked about how to handle this before, and concluded that it would be a warning rather than an error. The Compiler SDK would have to contain information for the compiler to understand the FSharp.Core version it was built with and emit a warning for any newer version. Probably not too difficult to implement, and we could consider this the tracking issue for that.

[KevinRansom]

@matthid @forki,

the issue isn't really about deployment of FSharp.Core dlls. Yes, maybe we should delist this one, maybe not. That decision really depends on where we land on the underlying issue. Which is that the latest compiler may generate an assembly from source code that compiled fine with the earlier compiler, but when compiled with F# 4.5 can no longer be consumed by the earlier compiler. without the developer specifically having typed in an F# 4.5 only feature.

If we decide that the above is fine and dandy (Idiom means good), which lets face it we could, then delisting is not really helpful.
If we decide that the above is not fine and dandy, which would probably be a more popular decision, then reverting the compiler back to an earlier version in dotnet cli and dev15.8 is the way to go.

Right now, I don't know the answer to that question, for certain, de-listing would delay the release of the next dotnet cli and VS 2017.8 releases, and so is very unlikely to happen.

For now, the work-arounds are:

1. Disable warning on project that encounters this specific error.
2. Pin FSharp.Core version to 4.2.3 for projects built with existing compilers.
3. Upgrade to the latest vs and dotnet/cli when they hit the streets.

I realize this is unsatisfactory, but it's what I have.

Kevin

[forki]

That decision really depends on where we land on the underlying issue.

No, not at all. We see error reports with the released bits. Unlisting would mitigate and give time to come up with a fix or just delays the release until a compiler is available. Whatever the underlying issue is and whatever strategy to deal with it: unlisting would help your real users.

de-listing would delay the release of the next dotnet cli and VS 2017.8 releases

how!? serious question. How does it influence dev of VS? You current preview templates are pinned to 4.51, right? they would continue to work.

[jwosty]

(@matthid)

If this indeed not a supported scenario why is the compiler not emitting a warning if it encounters a newer FSharp.Core than it supports? At least people would know what to do (and it would be a warning as well if stuff "just works" anyway).

(@cartermp)

@matthid Because we haven't implemented it yet 🙂

@cartermp is there an issue / feature request tracking this somewhere?

[cartermp]

I'll summarize this + what's being done here:

• This was initially viewed as a forwards-compatibility bug, which is something that we do not support.
• Upon a further investigation, we found that this is _actually_ a binary compatibility bug where F#-specific metadata is being "leaked" to downstream consumers. Since FSharp.Core is binary compatible, this is being fixed.
• This metadata leakage will result in older F# code that compiled just fine to not necessarily compile just fine if it uses FSharp.Core 4.5.1 (either directly or via transitive reference)
• The fix is to not "leak" this metadata.

To further remediate this change, we're discussing the following:

• Regression test that ensures F#-specific metadata is "pinned down" and does not leak out
• Smoke test of consuming newer FSharp.Core binaries to see what breaks (note: since we are not forwards compatible, we will expect some breaks that will not be fixed)
• Poisoning constructs (e.g., how C# poisons byref-like types) such that if compilers that do not understand the construct attempt to consume them, they'll get a specific error message
• Emitting a warning when the compiler attempts to build with an FSharp.Core that is of a higher version that what it was built with
• Unlisting 4.5.1

From a timing perspective, we are currently kicking off builds to insert a compiler with #5388 into VS and the .NET SDK, along with updates to targets and templates files so that an FSharp.Core binary that is build from that compiler is referenced by new projects.

In general, it can be very difficult to distinguish between what is a binary breaking change and what is a forwards compatibility issue. Sorry @forki / @isaacabraham if we've seemed standoff-ish about this, as it was not clear what this issue was actually bringing to the fold, nor was it clear how we would proceed until just now.

Thus, we will treat this as a tracking issue for the binary compatibility fix.

@jwosty I believe we will track a separate issue for any and all remediations against this sort of thing. How to approach this is actually up for discussion, IMO, since they do come with some different tradeoffs.

[forki]

Thanks everyone!

Phillip Carter [e-mail] schrieb am Do., 26. Juli 2018,
21:17:

I'll summarize this + what's being done here:

• This was initially viewed as a forwards-compatibility bug, which is
  something that we do not support.
• Upon a further investigation, we found that this is actually a
  binary compatibility bug where F#-specific metadata is being "leaked" to
  downstream consumers. Since FSharp.Core is binary compatible, this is being
  fixed.
• This metadata leakage will result in older F# code that compiled
  just fine to not necessarily compile just fine if it uses FSharp.Core 4.5.1
  (either directly or via transitive reference)
• The fix is to not "leak" this metadata.

To further remediate this change, we're discussing the following:

• Regression test that ensures F#-specific metadata is "pinned down"
  and does not leak out
• Smoke test of consuming newer FSharp.Core binaries to see what
  breaks (note: since we are not forwards compatible, we will expect some
  breaks that will not be fixed)
• Poisoning constructs (e.g., how C# poisons byref-like types) such
  that if compilers that do not understand the construct attempt to consume
  them, they'll get a specific error message
• Emitting a warning when the compiler attempts to build with an
  FSharp.Core that is of a higher version that what it was built with
• Unlisting 4.5.1

From a timing perspective, we are currently kicking off builds to insert a
compiler with #5388 https://github.com/Microsoft/visualfsharp/pull/5388
into VS and the .NET SDK, along with updates to targets and templates files
so that an FSharp.Core binary that is build from that compiler is
referenced by new projects.

In general, it can be very difficult to distinguish between what is a
binary breaking change and what is a forwards compatibility issue. Sorry
@forki https://github.com/forki / @isaacabraham
https://github.com/isaacabraham if we've seemed standoff-ish about
this, as it was not clear what this issue was actually bringing to the
fold, nor was it clear how we would proceed until just now.

Thus, we will treat this as a tracking issue for the binary compatibility
fix.

@jwosty https://github.com/jwosty I believe we will track a separate
issue for any and all remediations against this sort of thing. How to
approach this is actually up for discussion, IMO, since they do come with
some different tradeoffs.

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

[cartermp]

The fix for this is now merged. We will push a 4.5.2 that is compiled with the fixed compiler, unlist 4.5.1, and kick off updates to the compiler for .NET SDK and VS so that we don't ship a compiler that could produce binary-incompatible bits for users. Closing, and thanks for the report and patience here @forki.

[KevinRansom]

@cartermp, may I please keep this open, until we have run a bit more verification and published the update FSharp.Core and updated the tools :-)

[cartermp]

Yep yep

[forki]

I can confirm "die Kuh ist vom Eis" as we say in Germany. After paket update I get FSharp.Core 4.5.2 and the warning is gone. Good work!

[KevinRansom]

4.5.1 unlisted.
4.5.2 posted --- https://www.nuget.org/packages/FSharp.Core/4.5.2

[isaacabraham]

You know you've done something special when @forki hands out a "good work" ;-)

[cartermp]

Yep, we've allowed the cow some warmth now.