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

Give nice explanation when else case is missing #1142

Merged
merged 1 commit into from May 9, 2016

Conversation

@forki
Copy link
Contributor

@forki forki commented May 2, 2016

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

@isaacabraham isaacabraham commented May 2, 2016

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

@forki
Copy link
Contributor Author

@forki forki commented May 2, 2016

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

@smoothdeveloper
Copy link
Contributor

@smoothdeveloper smoothdeveloper commented May 2, 2016

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
Copy link

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

@smoothdeveloper smoothdeveloper commented May 2, 2016

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

@isaacabraham
Copy link
Contributor

@isaacabraham isaacabraham commented May 2, 2016

@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 forki force-pushed the forki:missing-else branch from 561fad6 to 8973269 May 2, 2016
@forki
Copy link
Contributor Author

@forki forki commented May 2, 2016

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

@forki forki force-pushed the forki:missing-else branch from 8973269 to 48ffa6e May 2, 2016
@forki
Copy link
Contributor Author

@forki forki commented May 2, 2016

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

@smoothdeveloper
Copy link
Contributor

@smoothdeveloper smoothdeveloper commented May 2, 2016

@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 forki force-pushed the forki:missing-else branch from 9911639 to 62f79ef May 2, 2016
@forki
Copy link
Contributor Author

@forki forki commented May 2, 2016

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

@forki forki force-pushed the forki:missing-else branch from 62f79ef to d37015e May 2, 2016
@forki
Copy link
Contributor Author

@forki forki commented May 2, 2016

For #1105 we now have:

image

@smoothdeveloper
Copy link
Contributor

@smoothdeveloper smoothdeveloper commented May 2, 2016

@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
Copy link
Contributor Author

@forki 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 forki force-pushed the forki:missing-else branch from d37015e to 144b3f6 May 2, 2016
@forki
Copy link
Contributor Author

@forki forki commented May 2, 2016

@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 WIP - Give nice explanation when else case is missing Give nice explanation when else case is missing May 2, 2016
@forki
Copy link
Contributor Author

@forki forki commented May 2, 2016

So tests are green. This is ready for first review

@zoren
Copy link

@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
Copy link
Contributor Author

@forki forki commented May 2, 2016

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.

eModuleOrNamespaceTypeAccumulator: ModuleOrNamespaceType ref

/// Error Message for type errors
specializedErrorMessage : ((string*string) -> string) option

This comment has been minimized.

@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

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

This comment has been minimized.

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

@@ -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'."

This comment has been minimized.

@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'.

This comment has been minimized.

@KevinRansom

KevinRansom May 5, 2016
Member

@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 force-pushed the forki:missing-else branch 2 times, most recently from 13e8b69 to ea8f0f2 May 4, 2016
@forki forki force-pushed the forki:missing-else branch from ea8f0f2 to fbda3a2 May 4, 2016
@isaacabraham
Copy link
Contributor

@isaacabraham isaacabraham commented May 5, 2016

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

@dsyme 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
Copy link
Member

@KevinRansom KevinRansom commented May 6, 2016

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

@isaacabraham isaacabraham commented May 7, 2016

@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
Copy link

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

@dsyme dsyme commented May 7, 2016

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

@isaacabraham isaacabraham commented May 7, 2016

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

@KevinRansom
Copy link
Member

@KevinRansom KevinRansom commented May 8, 2016

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 dotnet:master May 9, 2016
2 of 4 checks passed
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
Copy link
Member

@KevinRansom KevinRansom commented May 9, 2016

Thank you for taking care of this.

@SamiPerttu
Copy link

@SamiPerttu SamiPerttu commented May 9, 2016

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
Copy link

@zbilbo zbilbo commented May 10, 2016

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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants
You can’t perform that action at this time.