Give nice explanation when else case is missing #1142

Merged
merged 1 commit into from May 9, 2016

Projects

None yet

10 participants

@forki
Contributor
forki commented May 2, 2016 edited

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:

image

For #1105 we now have:

image

@isaacabraham
Contributor

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

@forki
Contributor
forki commented May 2, 2016

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

@smoothdeveloper
Contributor

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
zoren commented May 2, 2016

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
Contributor

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

@isaacabraham
Contributor
isaacabraham commented May 2, 2016 edited

@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
Contributor
forki commented May 2, 2016

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

@forki
Contributor
forki commented May 2, 2016

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

@smoothdeveloper
Contributor

@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
Contributor
forki commented May 2, 2016

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

@forki
Contributor
forki commented May 2, 2016

For #1105 we now have:

image

@smoothdeveloper
Contributor

@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
Contributor
forki commented May 2, 2016

@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
Contributor
forki commented May 2, 2016 edited

@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 forki changed the title from WIP - Give nice explanation when else case is missing to Give nice explanation when else case is missing May 2, 2016
@forki
Contributor
forki commented May 2, 2016

So tests are green. This is ready for first review

@zoren
zoren commented May 2, 2016

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

@forki
Contributor
forki commented May 2, 2016 edited

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 dsyme commented on an outdated diff May 3, 2016
src/fsharp/TypeChecker.fs
@@ -265,7 +265,10 @@ type TcEnv =
eInternalsVisibleCompPaths: CompilationPath list // internals under these should be accessible
/// Mutable accumulator for the current module type
- eModuleOrNamespaceTypeAccumulator: ModuleOrNamespaceType ref
+ eModuleOrNamespaceTypeAccumulator: ModuleOrNamespaceType ref
+
+ /// Error Message for type errors
+ specializedErrorMessage : ((string*string) -> string) option
@dsyme
dsyme May 3, 2016 Contributor

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

@dsyme dsyme commented on the diff May 3, 2016
src/fsharp/TypeChecker.fs
@@ -5572,14 +5574,16 @@ and TcExprUndelayed cenv overallTy env tpenv (expr: SynExpr) =
TcStmtThatCantBeCtorBody cenv env tpenv e1
| SynExpr.IfThenElse (e1,e2,e3opt,spIfToThen,isRecovery,mIfToThen,m) ->
- let e1',tpenv = TcExprThatCantBeCtorBody cenv cenv.g.bool_ty env tpenv e1
- (if isNone e3opt && not isRecovery then UnifyTypes cenv env m overallTy cenv.g.unit_ty)
@dsyme
dsyme May 3, 2016 Contributor

You should not change the order in which this "unit" type constraint is applied in the case of if/then. So you need to keep this line. I think you can specify { env with specializedErrorMessage = Some FSComp.SR.missingElseBranch } on this line?

@dsyme dsyme commented on the diff May 3, 2016
src/fsharp/FSComp.txt
@@ -13,6 +13,8 @@ undefinedNameRecordLabelOrNamespace,"The record label or namespace '%s' is not d
undefinedNameRecordLabel,"The record label '%s' is not defined"
undefinedNameTypeParameter,"The type parameter '%s' is not defined"
undefinedNamePatternDiscriminator,"The pattern discriminator '%s' is not defined"
+missingElseBranch,"You have not supplied the \"else\" case for this expression. If / then is an expression and so it must always return a result of the same type in all cases. This expression was expected to have type '%s' but here has type '%s'."
@dsyme
dsyme May 3, 2016 Contributor

Suggest this:

This expression is missing an 'else' branch. The 'then' branch of this expression has type '%s'. The 'else' branch of an 'if ... then' expression may only be omitted if the 'then' branch has type 'unit'.
@KevinRansom
KevinRansom May 5, 2016 Contributor

@dsyme I think this is slightly more concise and conveys the same meaning:

The 'if' expression is missing an 'else' branch, the 'then' branch has type '%s'. The 'else' branch must match this type, it may only be omitted when the 'then' branch has type 'unit'.

Failing test is

[00:34:51] Libraries\Core\Unchecked (DefaultOf02)
[00:34:51] -- passed
[00:34:51] Warnings (WrongNumericLiteral.fs)
[00:34:51] -- passed
[00:34:51] Warnings (WarnIfMissingElseBranch.fs)
[00:34:51] -- failed
[00:34:51] Warnings (ElseBranchHasWrongType.fs)
[00:34:51] -- passed
[00:34:51] Libraries\Portable (consumer.fs (Desktop))

@forki forki Give nice error message when else case is missing
fbda3a2
@isaacabraham
Contributor

@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
Contributor
dsyme commented May 5, 2016

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
Contributor
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
Contributor

@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
psiLearn commented May 7, 2016

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

Something like that would have helped me

@dsyme
Contributor
dsyme commented May 7, 2016 edited

@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
Contributor

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

@KevinRansom
Contributor

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 KevinRansom merged commit fbda3a2 into Microsoft:master May 9, 2016

2 of 4 checks passed

Windows_NT Release_ci_part2 Build Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
Windows_NT Debug Build Build finished.
Details
Windows_NT Release_ci_part1 Build Build finished.
Details
@KevinRansom
Contributor

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
zbilbo commented May 10, 2016 edited

why not go all in?

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

or something ;-)

@forki forki deleted the forki:missing-else branch Nov 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment