Mysterious type inference changes break safe stack apps

[forki]

A recent change in type inference (probably related to SRTPs) breaks all safe stack apps that use task {}

Original report: SAFE-Stack/SAFE-BookStore#283

Repro steps

I tried to create a repro here see #4285 but that did not work. But repro is working on bookstore SAFE-Stack/SAFE-BookStore#293

Expected behavior

No further type hints needed

Actual behavior

We need a lot of new type annotations to make it compile

Known workarounds

Adding type hints

[realvictorprm]

I wasn't aware of such a bug. Sounds worse I hope we know soon what the cause is.

[dsyme]

@forki To repro I should use https://github.com/SAFE-Stack/SAFE-BookStore/tree/forki-patch-3?

[forki]

yes. sorry I don't have better repro yet.

[dsyme]

I'll take a look. I haven't yet verified the issue, but noting #4173 as a possible cause

[dsyme]

@forki Is this the same issue as giraffe-fsharp/Giraffe#222 (comment)?

[forki]

no that one was different and is solved by opening correct namespace. forki-patch-3 already has all namespaces correctly opened and would work against .net cli 2.1.3 - only the update to 2.1.300-preview1-008009 makes it break.

you should be able to repro by running "build.cmd run" on that branch

[dsyme]

@forki I've confirmed that #4173 is indeed the cause of this change in behaviour.

This is difficult, as that PR was itself a reversion of a change which @gusty reports as causing a regression. We thought that the change was innocuous but it seems the kind of inline/SRTP/object-programming code in TaskBuilder.fs has picked up a dependency on the interim behaviour.

By far the best interim solution would be to simplify TaskBuilder.fs so it doesn't depend on this kind of subtlety and works with or without the change in #4173

[gusty]

I think if this really caused a regression then trying the code in F# previous to 4.1 should also fail.
Is there a minimal repro snippet?

[forki]

I think @rspeele and @gerardtoconnor are the experts when it comes to the task { } - I'm not sure if / how to rewrite it.

[gusty]

I agree with @dsyme in that it might be worth fixing the taskbuilder code instead.
I can give it a try, if you send me a repro.

[forki]

@gusty repro is described above. that's all I have

[dsyme]

@gusty I'll work on a minimal repro, we'll need it to pin down behaviour either way.

Status:

• PR Revert #1650 (and #3366) due to regressions #4173 definitely caused a change in behaviour - I've tried the before-and-after.

Questions:

• Are we 100% sure that Revert #1650 (and #3366) due to regressions #4173 was a clean revert of Speed up Constraint Solver's overload resolution (II) #1650?
• Also, you mention Check LocallyAbortOperationThatFailsToResolveOverload - fixes #3343 #3366 in the description of Revert #1650 (and #3366) due to regressions #4173. Is the idea that Revert #1650 (and #3366) due to regressions #4173 reverts both Speed up Constraint Solver's overload resolution (II) #1650 and Check LocallyAbortOperationThatFailsToResolveOverload - fixes #3343 #3366?

I think if this really caused a regression then trying the code in F# previous to 4.1 should also fail.

We can try to verify this. It's plausible as the TaskBuilder.fs code is using SRTP very extensively in a way I wasn't seeing prior to 4.1

[gusty]

Are we 100% sure that #4173 was a clean revert of #1650?

I can double check it, but I remember I did it very carefully.

Also, you mention #3366 in the description of #4173. Is the idea that #4173 reverts both #1650 and #3366?

Yes, it reverts also #3366 which was a case not handled in #1650 so now it's not needed.

We can try to verify this. It's plausible as the TaskBuilder.fs code is using SRTP very extensively in a way I wasn't seeing prior to 4.1

Another possibility is to fix the root cause of the issue with the taskbuilder code. I mean #1650 is wrong, now I'm sure about it, because the reasoning behind it was wrong, but that doesn't mean that it could have accidentally workaround an existing bug (there are many), if that's the case, we should attack that specific bug instead. So the main question here is, regardless of the status of the F# compiler, is the taskbuilder code correct? Should it compile? (I'm having a look at it right now)

[dsyme]

@gusty Yes. However in terms of public releases it may make sense to reapply #1650 and #3366 until we've done a full investigation of what it means to revert and we have full agreement about root cause, impact of changes etc. #1650 and #3366 are already out in the wild in F# 4.1 and .NET SDK 4.1.

Perhaps #1650 and #3366 are "wrong", but we need to more deeply understand why and what impact undoing will have.

BTW the repro is simple, just compile this against the latest TaskBuilder.fs, e.g. I used https://github.com/rspeele/TaskBuilder.fs/blob/8aec0ecc259bb936dc0b882730592d3649641c5b/TaskBuilder.fs, e.g. F# 4.1 latest:

fsc.exe TaskBuilder.fs Repro.fs

versus master:

C:\github\dsyme\visualfsharp\Debug\net40\bin\fsc.exe TaskBuilder.fs Repro.fs

Code for Repro.fs:

open System.Threading.Tasks
open FSharp.Control.Tasks.ContextInsensitive

type Client() =
    member x.SomeMethod (s:string) = 1

let getBooksTable () = task {
    let! client = Unchecked.defaultof<Task<Client>>
    let table = client.SomeMethod "book"
    return table }

Note that Bind fails to propagate return type in type inference.

[gusty]

@dsyme I agree that we should consider unreverting as a quick fix.

I tried the repro and it fails in F# 4.0

> ;;

      let table = client.SomeMethod "book"
  ----------------^^^^^^^^^^^^^^^^^

C:\Users\gus.leon\AppData\Local\Temp\stdin(32,17): error FS0072: Lookup on object of indeterminate type based on information prior to this program point. A type annotation may be needed prior to this program point to constrain the type of the object. This may allow the lookup to be resolved.
> 

I'll keep looking ...

[gerardtoconnor]

Hey guys, im no expert but can try assist. When its said to 'fix' the TaskBuilder, To be able to accept plain Tasks, Tasks<'T> and task-like arguments (via awaiter), needs to be SRTP, as overloading for each breaks type inference. Is it the inlining of the bindings that's issue? The CE already creates a lot of nesting via combine/delay so inlining is needed for perf. I saw in error logs that specific overload of getAwaiter could not be resolved (both plain & generic loaded) but sounds like further constraints won't fix this.

[gusty]

@gerardtoconnor No, you'll need to keep inlining, but notice that the taskbuilder fails in F# lower than 4.1 (see the repro above).

So I'll try to make it work, my experience working with SRTPs tells me that subtle changes in the source code of the builder/inline functions have big impacts on the client code.

[gerardtoconnor]

Thanks @gusty. The one hack to note is that to pass output type through from method overload to inline SRTP fun, static generic param binder method used, this might be place to start.

[gusty]

Yes, I used that technique all the time. Agree, that's a good starting point.

[rspeele]

Some info from my end:

Why it's overcomplicated

The purpose of the inline/SRTP skulduggery in TaskBuilder.fs is to support arbitrary awaitables, with something close to the C# rules about what can be awaited. As far as I know, simplifying it would require changing user code to manually convert those awaitables to a common type before Binding them. I'm happy to code it however I need to as long as it's still possible to:

• Write a Bind that works for any awaitable (including the non-generic Task)
• Write a Bind for the special case of Task<T>, so it doesn't get confused and try to bind as a Task
• Have the context-insensitive builder automatically call ConfigureAwait(false) on any awaitable that supports that method, but cleanly fall back to binding without that on awaitables that don't support that method

The third point I could be flexible on. It probably wouldn't be too bad if the automatic ConfigureAwait only worked for a small number of hardcoded types.

Extension methods

The extension methods at the end of TaskBuilder.fs are a tricky part of the code. To the best of my memory these rely on two behaviors, which I observed by experiment rather than reading in the compiler code, but it sounds like these might've come from #1650 .

1. Methods on the type are preferred over extension methods. The Task<T> binds are true instance methods of the builders, whereas the generic awaitable binds using SRTPs are extension methods. When binding Task<T> the compiler finds a perfect overload right there on the builder and doesn't seem to bother trying the generic SRTP extension bind and complaining that it's not sure which to use.

2. Extension methods seem to be tried most-recently-opened first. So when using the context-insensitive builder, there are the same extensions as for the context-sensitive one, but then also an auto-opened module HigherPriorityBinds containing extensions for awaitables that support a ConfigureAwait method. For types like System.Threading.Task that support ConfigureAwait the compiler uses this method, and doesn't complain that the generic Bind for all awaitables could've worked too.

[gusty]

@rspeele Thanks for all that info. What you describe I think is in the F# spec and a behavior I have long time observed, so I don't think there something specific to #1650 , it seems to be a type inference issue in presence of static constraints with overloads. Believe me it's not the first time I see this. I was about to report a trickier one today.

[dsyme]

@gusty @gerardtoconnor @rspeele For now there's no need to jump to modify TaskBuilder.fs. Current POR is to turn the handle on reverting #4173 before next non-preview release.

Separately we'll now start to look closely at the new behaviour w.r.t. overloads (and extension method overloads) that has been enabled by #1650 - it's definitely the case that new behaviour has been enabled by that which we haven't fully put under test.

TaskBuilder.fs acts as an interesting test case for reasonable (though complicated) uses of SRTP that have high practical value, and will use it to assess any future modifications.

@gusty Assuming the revert of #4173 goes through I'd like to get more test cases from you for behaviour you deem to be wrong. We also have #4063 and #3967 to consider. Whatever we do here we have to be really careful going forward.

thanks all

[cartermp]

Closing #4173 has been applied and a build for insertion is being spun up. It remains to be seen if this can make the cut for 15.6 RTW or a servicing fix for 15.6 (it's coming in hot).

[gusty]

@dsyme You seem to be absolutely correct in that TaskBuilder took a dependency on my PR.

To my surprise, what I'm finding out is that it's a "strong dependency".

At the moment I didn't manage to solve the issue in the TaskBuilder code, but here's a 25 lines repro:

open System
open System.Threading.Tasks

type Step<'a,'b> =
    | Return of 'a
    | Await of 'b * (Step<'a,'b>)

type TaskBuilder() =
    member inline __.Bind(task  : _ Task , continuation )  = continuation(task.GetAwaiter().GetResult())

module M =
    type TaskBuilder with
        member inline __.Bind(taskLike, continuation )  = 
                    let awt = (^abl : (member GetAwaiter : unit -> _) taskLike)
                    Await (awt, continuation (^awt : (member GetResult : unit -> _) awt))

open M

type Client() =
    member x.SomeMethod (s:string) = 1

let getBooksTable () = 
    TaskBuilder().Bind(Unchecked.defaultof<Task<Client>> , fun client ->
        let table = client.SomeMethod "book"
        Return table)

As I said before, this doesn't compile in F# 4.0, although type inference in intellisense seems to be smart enough to figure out the type of client.

The type cycle in the DU Step seems to play a key role in this issue.

Still I'm sure I can re-write the task builder using the approach I use in FSharpPlus and it will probably work just fine. But what I'm sure at this point is that the code in TaskBuilder makes sense, I mean it has to compile the way it is right now.

Moving forward, looks like we'll have to unrevert #1650 (I saw you already did it) as there is definitely some interesting stuff there, but we need to fix it in such a way that we don't get the regressions I'm finding from time to time.

I can definitely start to report many small repro similar to this one where type inference seems to be stuck half-way with no apparent reason and that might lead us to improve significantly the type inference.

Particularly I have ton of cases in FSharpPlus when combining computation expressions where if I specify all the types it doesn't compile, which is clearly an F# bug, since more should be better or at least not worst. My workaround at the moment is to use a wildcard _ in as type parameter.

Since this issue is already closed, should we create a separate issue?

[cartermp]

We may want to have a sit-down about SRTP inference for F# moving forward, since it's clearly such a touchy part of the compiler. Since we're now in the unfortunate state of things taking a dependency on behavior which may (or may not) have been questionable, we may just have to freeze the behavior here and just allow things to stabilize.

[dsyme]

@gusty Thanks for the analysis - I also came to the conclusion that a simple fix in TypeBuilder.fs wasn't practical

I can definitely start to report many small repro similar to this one where type inference seems to be stuck half-way with no apparent reason and that might lead us to improve significantly the type inference.

@gusty I'm actually more interested in positive test cases - things that compile today using SRTP that are "extreme" but "good". Those matter more in terms of evolving the feature. It is an area where I am willing to have some very exotic code break but I want all common, reasonable, mainline uses of SRTP characterised by corresponding tests.

This applies particularly in good uses feature interactions. SRTP for basic operator-like uses is fine but the corner cases are coming when it is combined with

• type-directed name resolution
• method overloading
• implicit operators (e.g. Type inference regression in type-associated operators #4063)
  etc. There are cases in the wild were people are relying on these combinations.

@cartermp I do want to make progress and fix the more egregious flaws, but to do it intentionally where we know exactly the corner cases we are fixing.

[gusty]

I can confirm now, that writing the TaskBuilder with the overload patterns I use in FSharpPlus, solves this issue.
Actually it looks a bit more readable to me and also works in F# 4.0 and lower.

@rspeele I can do a PR to the original project, so you can have a look at the code and test it.

[gusty]

FYI here's the PR with the fix rspeele/TaskBuilder.fs@9db31f6

[forki]

@gusty are they working with both versions of the compiler?

[gusty]

@forki Yes, this code works universally in all versions of F# with or without #1650 and it uses sort of a pattern I was using since 2010 so we can say it's bulletproof overloading code ;)

[forki]

Awesome. Thanks so much for making SAFE safe again. Am 19.02.2018 11:24 schrieb "Gustavo Leon" <[email protected]>: @forki <https://github.com/forki> Yes, this code works universally in all versions of F# with or without #1650 <#1650> and it uses sort of a pattern I was using since 2010 so we can say it's bulletproof overloading code ;) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4343 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADgNGt3lCKckCm7l4UpYiDu2WWO-UNsks5tWUvZgaJpZM4SDURr> .

[gusty]

Glad to help.

Just to be clear, I still think that the F# compiler should be able to compile the repro with the original code, but not at the cost of #1650 which introduced more problems than solutions.

Independent of that discussion and current status of the compiler, I suggest that taskbuilder is updated with this pattern, which seems to be more robust and easier to read, since you have all overloads in one place and the priority is explicit whereas in the original code you have to open, close modules, extensions and add comments like "see further below more overloads for ..." and so on.

[forki]

sigh taskbuilder broke. rspeele/TaskBuilder.fs#15

[gusty]

@dsyme Now that TaskBuilder.fs is fixed, can we consider re-re-reverting #1650 and avoid another accidentally introduced dependency?

[dsyme]

@gusty We'll just have to keep things stable for a while, and can't add/remove #1650 again.

You can, however, add both before and after versions of TaskBuidler.fs as test cases to this repo please? Then we pin down behaviour both positive and negative, and make sure any changes we make going forward have known effect.

[gusty]

But precisely in order to keep things stable I strongly recommend not to include #1650, the sooner we remove it, the better.

It will not only produce breaking changes, it will also break the compiler, see below a quick example I found today, but there are more. I encountered this error at least twice a month.

Remember the headaches caused until it was patched with #3366 , otherwise read again the conversations there, then think about it, considering that there are still more scenarios today where that internal error pops up. We would need more patches like #3366

Also keep in mind that the only known regression is in TaskBuilder, which is already fixed and in anycase that specific problematic version of TaskBuilder doesn't work in most F# versions (many companies still use F# 4.0) and it also has other incompatibility problems, as the one recently reported by @forki

Worst case, if you still insist to keep #1650 , I can try to find the time to investigate why the old TaskBuider didn't compile and try to solve that specific problem in the constraint solver.

[dsyme]

But precisely in order to keep things stable I strongly recommend not to include #1650, the sooner we remove it, the better.

I'm not sure I understand. #1650 has been in for 18 months now and is now the "stable" state. I can see we need to remove the internal error showing above, but we need to do it in a way that doesn't change what does and doesn't compile.

Anyway, please add a specific new issue for the current problems (e.g. the internal error), thanks :)

[gusty]

OK, just wanted to make sure you understand what's the current status, which is not "great" from my viewpoint.

I will create a separate issue, and add tests and repros.

I have a question for you, a very important question indeed: Do you think my reasoning behind #1650, (regarding stop exploring solutions in case of failure to resolve overload) was correct, incorrect, something in-between?

[dsyme]

@gusty I keep staring at that PR thinking "why did this make such a big difference". And in particular why this line should make such a difference

I think you should add a new issue that shows the very simplest scenario where commenting/uncommenting that line makes a major difference, and lets reason through it from there. And I assume that when that line is commented out, then #1650 is a noop?

[dsyme]

There is also this change and I'd like to know if/when that makes a difference.

[gusty]

And I assume that when that line is commented out, then #1650 is a noop?

Yes.

There is also this change and I'd like to know if/when that makes a difference.

It's hard to remember my reasoning at that time, but I think I had studied a case where that short-circuit lead to a failure to resolve an overload, and in that particular scenario by observing the fact that there was an Unresolved-Overload and signaling it, lead to explore and select the good one.

Looks like we'll have to revisit it, I will try to find the example I was analyzing, I think I know which one is, then figure out a better fix which solves the current problems (like the one in the taskbuilder) without causing regressions, and ideally solving more problems like that (there are many cases where I hit the error message saying that the default 'obj' was inferred and no overloads match that type).