Skip to content
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.

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

Compiler type inference regression #2730

Closed
saul opened this issue Mar 29, 2017 · 12 comments · Fixed by #2843
Closed

Compiler type inference regression #2730

saul opened this issue Mar 29, 2017 · 12 comments · Fixed by #2843
Milestone

Comments

@saul
Copy link
Contributor

saul commented Mar 29, 2017

type 'a Doge () = class end
with
    static member (|~>) (_ : 'b Doge, _ : 'b -> 'c) : 'c Doge = Doge ()

let x : System.DateTime Doge = Doge ()

let y = x |~> (fun dt -> dt.Year) // error on this line around 'dt.Year'

In F# 4.0 (and prior), the above code worked fine. However in F# 4.1 there seems to have been some kind of regression:

regress.fsx(7,26): 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 c
onstrain the type of the object. This may allow the lookup to be resolved.

This regression means we cannot use F# 4.1 (or VS 2017 due to IntelliSense errors) at work due to this regression in type inference that the code heavily depends upon.

/cc @dsyme

@dsyme
Copy link
Contributor

dsyme commented Mar 29, 2017

@saul Ouch, that's unexpected and frustrating.

I wonder if the cause is the optimizations to constraint solving #1650 and #1530. It would be good to do before/after checks or a bisect

// cc @gusty

@gusty
Copy link
Contributor

gusty commented Mar 29, 2017

Yes, it could be related to those optimizations. I'll dig again into it and find out why is not inferring dt based on the type of x. It's a shame we didn't have a test like this available.
Anyway looks like a fix shouldn't be complicate.

@0x53A
Copy link
Contributor

0x53A commented Mar 29, 2017

Tooltips actually work correctly on both dt and dt.Year, but it seems to be unable to infer the type of y:

image

image

image

No idea if this helps ...

@smoothdeveloper
Copy link
Contributor

@0x53A just FYI and based on my experience (with associated confusion) the tooling/tooltip will use a "larger part" of code in the inference while the compiler has somehow a more restricted way to unify the type.

I've had a few tickets where the tooling would allow me completion on infered types (#1310 is one of those) but the compiler will refuse with same FS0072 error.

@gusty
Copy link
Contributor

gusty commented Mar 29, 2017

Yes, that's almost always the case. It would be nice to make the compiler's type inference as smart.

@cartermp cartermp added this to the VS 2017 Updates milestone Mar 31, 2017
@saul
Copy link
Contributor Author

saul commented Apr 12, 2017

@gusty any update on this? No real rush, I'm just worried that a compiler regression has fallen by the wayside!

@cartermp what's the deadline for VS 2017 Update 2 in terms of code submissions on this repo? If we can't get this regression fixed in a reasonable timeframe before then, would it make sense to revert the performance optimisations that caused this bug? This is stopping our office from upgrading to VS 2017.

@gusty
Copy link
Contributor

gusty commented Apr 12, 2017

@saul I'd like to get to the bottom of this ASAP but right now am on holidays, this and next week.
It would be a shame to have to revert, since it's not just an optimization, it also solves some existing problems.
I also wonder if there is a deadline for the code submissions.
In the meantime I wonder if it's possible/makes sense for you to do some small adjustments in your code in order to make it more resilient to these changes, i.e. add some type annotations.

@cartermp
Copy link
Contributor

@saul Unfortunately, this isn't something we can get in for Update 2. We've already begun the insertion process, and we're in the current shipping process where we can only get something in if it's a critical bug which does something like crash VS. I don't think we can do anything here to revert the change between now and then. We could investigate something for Update 3 if a fix for the regression doesn't come in time.

@saul
Copy link
Contributor Author

saul commented Apr 12, 2017

Apologies - I was referring to Update 3 :) Do we have a cut off for that release?

@dsyme
Copy link
Contributor

dsyme commented Apr 12, 2017

This is the same root cause as #2416. I'm looking into trying to craft a fix that satisfies all the various tests.

@cartermp
Copy link
Contributor

@saul Currently, no. I imagine that would be sometime in early June, though. Or perhaps mid-may. It's hard to say, since we can sometimes make a case for a fix (or set of fixes), and the size of our component is both small and less tangled than others, making the justification for taking a fix less difficult.

I would assume, to be safe, that we have 1-1.5 months.

@dsyme dsyme mentioned this issue Apr 12, 2017
@dsyme
Copy link
Contributor

dsyme commented Apr 12, 2017

@saul Fix is in #2843, would be great if you could try that out on your larger company codebases.

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

Successfully merging a pull request may close this issue.

6 participants