Give nice explanation when else case is missing

[forki]

Implements #1104 and #1105

For the following code

let x = 10
let y =
   if x > 10 then "test"

we had the following error:

error FS0001: This expression was expected to have type
    unit    
but here has type
    string   

Now it's:

For #1105 we now have:

[isaacabraham]

Do we need to keep the original (confusing) unit / string error message?

[forki]

we can easily replace the whole message. Can you come up with something nice?

[smoothdeveloper]

The expression is missing the 'else' case. In this case 'if - then - else' is an expression and must always return a value of the same type.

Also, I'm thinking this message shouldn't occur with

if foo then
   1

In this case we should somehow mention about returning unit too.

[zoren]

I find the last sentence in the error message confusing. There is only one case, so "All cases" is correct but maybe not easy to understand. When the else-branch is omitted the type of the then-branch must be unit. Maybe this works instead "If / then is an expression in F#, the then branch it must be of type unit."

[smoothdeveloper]

Also we shouldn't mention in F# in those messages, it sounds like a redundant statement from redundant department of redundancy :)

[isaacabraham]

@forki - just get rid of the first bit :-)

You have not supplied the "else" case for this if expression. If / else is an expression and so must always return some result in all cases

@smoothdeveloper

Also, I'm thinking this message shouldn't occur with
if foo then 1

In this case we should somehow mention about returning unit too.

This is exactly where this error message should live - someone wants to return 1 but needs a default value for the else branch.

@zoren

The original idea is that the user wants to a return a value e.g. integer in this case. So they need to return a value on the "else" branch as well, rather than using ignore and turning it into a statement.

[forki]

Seems there is no tests that checks error message for missing else branch yet.

[forki]

@dsyme does it make sense to give a different error code in this case?

[smoothdeveloper]

@isaacabraham

This is exactly where this error message should live - someone wants to return 1 but needs a default value for the else branch.

Sorry for confusion, I meant my code outside of a let binding.

[forki]

@smoothdeveloper can you create a specific case and tell me what you want to improve?

[forki]

For #1105 we now have:

[smoothdeveloper]

@forki

let a  = true
if a then
  1

This expression was expected to have type
    unit    
but here has type
    int    

I think in this case (we don't assign the expression to anything) we should keep this current message (or find a better one) instead of talking about else.

The user probably did a mistake with the last statement in the if but it is not possible to know the intent.

[forki]

@smoothdeveloper as far as I can see we don't have the left hand side available at this point. So we would need to make the error message cover both cases.

[forki]

@dsyme I'm not sure if it's a good idea to extend the TcEnv, but somehow I need to pass context information down to the deeper levels of the type checker and to the constraint solver. I assume if we implement more of these messages this additional information will generalize a bit.

But I'm happy to use whatever techniques you prefer to get this one done.

[forki]

So tests are green. This is ready for first review

[zoren]

@isaacabraham So the assumption is that it's a forgotten else, not a forgotten ignore. But both options are possible I guess?

[forki]

Yeah there are like a million more things that are possible. We can't respond to everything, but I think with this assumption (and without changing too much) we can at least help newcomers to overcome one of the main issues.

[dsyme]

Extending TcEnv is a bit problematic. for two reasons

• We really need to be reducing the size of TcEnv instead of increasing it. One technique would be to split it between hot (varying fields) and cold (usually empty/None/...) fields, and only allocate the cold block when one of the fields gets a non-default value.
• The entry you've added needs to be reset to "None" at an appropriate point. For example, when you go into a lambda, or perhaps any expression that's not a control construct i.e. not an if/then/else, match, try/with, try/finally.

Adding a function also feels a bit wrong - I'd rather have a more explicit representation of the information being added here, so we know more clearly when the information no longer applies (e.g. in the cases mentioned above), or we can work out how it interacts with other "expression context" information we are bound to add in the future. So it feels right to change this to be a "eContextInfo" field that contains a union between various expression contexts, with a clear definition in the /// comments for the cases as to what it means to be in that context. The two contexts added for this PR would presumably be ElseBranch and IfBranchWithoutElse

[isaacabraham]

@KevinRansom @dsyme

Personally I actually wouldn't refer to unit in the message - in any case, you wouldn't see this message in the case that you had a single branch that returned unit anyway.

What I originally was hoping was getting across that if / then / else is an expression which has some tangible result, and therefore the return type of all branches must be unified - hence the original (somewhat verbose) explanation. You wouldn't believe how many people just assume that if / then / else is a statement in every language. How about this?

This 'if' expression evaluates to '%s' but is missing an 'else' branch. Because 'if' is an expression, and not a statement, you must add an 'else' branch which also returns an instance of '%s'.

[dsyme]

You wouldn't believe how many people just assume that if / then / else is a statement in every language.

@isaacabraham I can believe that :)

Perhaps this (taking @KevinRansom and @isaacabraham suggestions together):

The 'if' expression is missing an 'else' branch. The 'then' branch has type '%s'. Because 'if' is an expression, and not a statement, you must add an 'else' branch which returns a value of the same type.

[KevinRansom]

The 'if' expression is missing an 'else' branch. The 'then' branch has type '%s'. Because 'if' is an expression, Consider adding an 'else' branch which returns a value of the same type or applying ignore to the then branch.

[isaacabraham]

@KevinRansom I've said my piece. I think adding "ignore" is just going to confuse people and doesn't relate to 99.9999% of the times that this error will show up.

[psiLearn]

If expression needs an else expression if the return type differs from unit

Something like that would have helped me

[dsyme]

@KevinRansom Yes, no need to mention "ignore", it's almost certainly not the right action for the user to take. For example, consider

let f x y = ()

if true then f 3

The problem here is that the function has not been applied to all its arguments and adding "ignore" will be incorrect.

My current suggestion:

    The 'if' expression is missing an 'else' branch. The 'then' branch has type '%s'. Because 'if' is 
    an expression, it needs an 'else' branch of the same type as the 'then' branch. Consider adding 
    an 'else' branch.

I've tried a few times and it's very hard to also explain that if the 'then' branch has type 'unit' then the 'else' branch can be omitted. Here's my best attempt - @isaacabraham what do you think?

    The 'if' expression is missing an 'else' branch. The 'then' branch has type '%s'. Because 'if' is 
    an expression, it needs an 'else' branch of the same type as the 'then' branch. The 'else' 
    branch can be omitted if the 'then' branch has type 'unit'. Consider adding an 'else' branch.

thanks

[isaacabraham]

@dsyme I think your suggestion above (2 days ago) was very good and would be my preference.

[KevinRansom]

Going with:

The 'if' expression is missing an 'else' branch. The 'then' branch has type '%s'. Because 'if' is an expression, and not a statement, add an 'else' branch which returns a value of the same type.

[KevinRansom]

Thank you for taking care of this.

[SamiPerttu]

One more suggestion. Trying to put it as simply as possible.

An 'if' expression must always return a value. The 'else' branch may be omitted only when the 'then' branch has type unit.

[zbilbo]

why not go all in?

if ifs and elses where optional on impulses, we'd all have some sorry diplomas...

or something ;-)