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

Context propagates wrongly => wrong error message #1827

Merged
merged 1 commit into from Nov 27, 2016

Conversation

Projects
None yet
4 participants
@forki
Contributor

forki commented Nov 24, 2016

The “context” is persisting into the checking of the body of the “else” branch, but not being reset.
Therefor we get too specific error message:

image

  • Reproduce in test
  • fix context propagation
@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 24, 2016

Contributor

similar case:

image

Contributor

forki commented Nov 24, 2016

similar case:

image

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 24, 2016

Contributor

image

Contributor

forki commented Nov 24, 2016

image

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 24, 2016

Contributor

Arg this destroyed this case:

image

Contributor

forki commented Nov 24, 2016

Arg this destroyed this case:

image

Show outdated Hide outdated src/fsharp/TypeChecker.fs
@@ -5641,12 +5641,14 @@ and TcExprUndelayed cenv overallTy env tpenv (expr: SynExpr) =
TcRecdExpr cenv overallTy env tpenv (inherits,optOrigExpr,flds,mWholeExpr)
| SynExpr.While (spWhile,e1,e2,m) ->
let env = ClearContext env

This comment has been minimized.

@dsyme

dsyme Nov 24, 2016

Contributor

I expect you'll have to clear on all recursive calls. I think it may be more accurate to just pass the ctxt as an explicit parameter to TcExpr?

@dsyme

dsyme Nov 24, 2016

Contributor

I expect you'll have to clear on all recursive calls. I think it may be more accurate to just pass the ctxt as an explicit parameter to TcExpr?

This comment has been minimized.

@forki

forki Nov 24, 2016

Contributor

it's not easy. we have to be very careful to not erase from the wrong position.
Actually I'm not even convinced if the "context" is the best idea here.

@forki

forki Nov 24, 2016

Contributor

it's not easy. we have to be very careful to not erase from the wrong position.
Actually I'm not even convinced if the "context" is the best idea here.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 24, 2016

Contributor

@dsyme:

One of the issues is:

// f x
| SynExpr.App (hpa,_,func,arg,mFuncAndArg) ->
    TcExprThen cenv overallTy env tpenv func ((DelayedApp (hpa, arg, mFuncAndArg)):: delayed)

we want to keep the context for the result type check, but erase for x and for whaterver happens inside of f.

Contributor

forki commented Nov 24, 2016

@dsyme:

One of the issues is:

// f x
| SynExpr.App (hpa,_,func,arg,mFuncAndArg) ->
    TcExprThen cenv overallTy env tpenv func ((DelayedApp (hpa, arg, mFuncAndArg)):: delayed)

we want to keep the context for the result type check, but erase for x and for whaterver happens inside of f.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 24, 2016

Contributor

I think what we actually want to do is not set a context here at all. We just want to make the very last UnifyTypes call in that else expression aware of the situation

Contributor

forki commented Nov 24, 2016

I think what we actually want to do is not set a context here at all. We just want to make the very last UnifyTypes call in that else expression aware of the situation

@forki forki closed this Nov 24, 2016

@forki forki reopened this Nov 24, 2016

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 24, 2016

Contributor

something like:

            let tempty = NewInferenceType ()
            let e3',tpenv = TcExprThatCanBeCtorBody cenv tempty env tpenv e3 

            let env = { env with eContextInfo = ContextInfo.ElseBranch }                 
            UnifyTypes cenv env e3.Range overallTy tempty

instead of:

            let env = { env with eContextInfo = ContextInfo.ElseBranch } 
            let e3',tpenv = TcExprThatCanBeCtorBody cenv overallTy env tpenv e3 
Contributor

forki commented Nov 24, 2016

something like:

            let tempty = NewInferenceType ()
            let e3',tpenv = TcExprThatCanBeCtorBody cenv tempty env tpenv e3 

            let env = { env with eContextInfo = ContextInfo.ElseBranch }                 
            UnifyTypes cenv env e3.Range overallTy tempty

instead of:

            let env = { env with eContextInfo = ContextInfo.ElseBranch } 
            let e3',tpenv = TcExprThatCanBeCtorBody cenv overallTy env tpenv e3 
@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 24, 2016

Contributor

looks like #1827 (comment) is not enough since it forces us for type hints: cb3fa24

Contributor

forki commented Nov 24, 2016

looks like #1827 (comment) is not enough since it forces us for type hints: cb3fa24

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 24, 2016

Contributor

and there are coming more.

@dsyme is there another way to let the context only work for the return type check?

Contributor

forki commented Nov 24, 2016

and there are coming more.

@dsyme is there another way to let the context only work for the return type check?

@dsyme

This comment has been minimized.

Show comment
Hide comment
@dsyme

dsyme Nov 24, 2016

Contributor

That sort of implementation looks much more robust (once it's green)

Contributor

dsyme commented Nov 24, 2016

That sort of implementation looks much more robust (once it's green)

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 24, 2016

Contributor

Ok this strategy didn't work, but now I think we can do this:

            let env = { env with eContextInfo = ContextInfo.ElseBranchResult e3.Range }
            let e3',tpenv = TcExprThatCanBeCtorBody cenv overallTy env tpenv e3 
            e3',SequencePointAtTarget,tpenv

and at the error reporting site we check if the type error comes from the right range. that's basically narrowing the context to a minimum.

Contributor

forki commented Nov 24, 2016

Ok this strategy didn't work, but now I think we can do this:

            let env = { env with eContextInfo = ContextInfo.ElseBranchResult e3.Range }
            let e3',tpenv = TcExprThatCanBeCtorBody cenv overallTy env tpenv e3 
            e3',SequencePointAtTarget,tpenv

and at the error reporting site we check if the type error comes from the right range. that's basically narrowing the context to a minimum.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 24, 2016

Contributor

This is how it felt:

aua

But I think this is now the correct strategy and my manual checks are all working.

Contributor

forki commented Nov 24, 2016

This is how it felt:

aua

But I think this is now the correct strategy and my manual checks are all working.

Show outdated Hide outdated src/fsharp/CompileOps.fs
| ConstraintSolverTypesNotInEqualityRelation(denv,t1,t2,m,m2) ->
// REVIEW: consider if we need to show _cxs (the type parameter constrants)
| ConstraintSolverTypesNotInEqualityRelation(denv,t1,t2,m,m2,contextInfo) ->
// REVIEW: consider if we need to show _cxs (the type parameter constraints)

This comment has been minimized.

@matthid

matthid Nov 24, 2016

Contributor

Now git blame doesn't show the original author, might be a good idea to add it (if it is even correct)

@matthid

matthid Nov 24, 2016

Contributor

Now git blame doesn't show the original author, might be a good idea to add it (if it is even correct)

This comment has been minimized.

@forki

forki Nov 24, 2016

Contributor

what?

@forki

forki Nov 24, 2016

Contributor

what?

This comment has been minimized.

@matthid

matthid Nov 24, 2016

Contributor

I mean if I ever go across a comment like this I might want to talk with the original author (which would be you now). But that was really just a suggestion, as one can figure it out with multiple git calls as well ...

@matthid

matthid Nov 24, 2016

Contributor

I mean if I ever go across a comment like this I might want to talk with the original author (which would be you now). But that was really just a suggestion, as one can figure it out with multiple git calls as well ...

This comment has been minimized.

@forki

forki Nov 24, 2016

Contributor

Yeah I think I already fucked up couple of such comments. But tbf I assume
most "original authors" are the initial OSS commit anyways.

@forki

forki Nov 24, 2016

Contributor

Yeah I think I already fucked up couple of such comments. But tbf I assume
most "original authors" are the initial OSS commit anyways.

This comment has been minimized.

@dsyme

dsyme Nov 24, 2016

Contributor

If it helps that comment is from me :)

@dsyme

dsyme Nov 24, 2016

Contributor

If it helps that comment is from me :)

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 25, 2016

Contributor

This is fixed as well 👍
image

Contributor

forki commented Nov 25, 2016

This is fixed as well 👍
image

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 25, 2016

Contributor

this is ready for review. I had to disable the nice async error message for now since the range where the exception happens is inside the "return" part. So that can't be found right now. I leave that for F# 4.2

But this is now removign all the false positive warnings and should be merged to FCS as soon as possible since Ionide and Xamarin Studio? (cc @Krzysztof-Cieslak @nosami) are already using it.

Contributor

forki commented Nov 25, 2016

this is ready for review. I had to disable the nice async error message for now since the range where the exception happens is inside the "return" part. So that can't be found right now. I leave that for F# 4.2

But this is now removign all the false positive warnings and should be merged to FCS as soon as possible since Ionide and Xamarin Studio? (cc @Krzysztof-Cieslak @nosami) are already using it.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 25, 2016

Contributor

All green. Yay. Finally

Contributor

forki commented Nov 25, 2016

All green. Yay. Finally

@dsyme

dsyme approved these changes Nov 27, 2016

@dsyme dsyme merged commit ecb4307 into Microsoft:master Nov 27, 2016

6 checks passed

Ubuntu14.04 Release Build Build finished.
Details
Windows_NT Debug Build Build finished.
Details
Windows_NT Release_ci_part1 Build Build finished.
Details
Windows_NT Release_ci_part2 Build Build finished.
Details
Windows_NT Release_net40_no_vs Build Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@forki forki deleted the forki:contextprop branch Nov 28, 2016

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