[This article is part of the F# advent calendar 2016]
A couple of months ago I wrote a blog post called “Make failure great again” and this joke backfired on me. The post was meant as an introduction to the F# compiler project and showed in a simple scenario all the steps that are needed to get a Pull Request into the compiler.
Around the same time as the blog post I sent a Pull Request to the compiler project which is pretty similar to the one described in the post. In #1142 I extended it to give a better error message on the following snippet:
Before the change the compiler would just complain about the type error between the if and the corresponding else branch.
Newcomers from statement-based languages like C# might have a hard time to understand what’s going on.
After the change we would get something like the following:
This message now additionally explains that F# treats “if/then/else”-constructs as expressions and the return type of both branches must return the same.
Unfortunately reality is a bitch and it turned out my test cases were too narrow. Since the new error messages are already released in Visual Studio 2017 RC1, it didn’t take long for the following bug report to appear:
The compiler is now showing the following error:
Obviously this is not correct, but what exactly went wrong? In order to understand the problem we need to take a look at the principal change of the pull request.
This strange looking code is part of TypeChecker.fs and represents the part where F#’s compiler tries to type check “if/then/else”-constructs. It uses pattern mattching on the abstract syntax tree (line 1) to destructure the current expression and calls the the type checker with all sub-expressions recursively (look for TcExpr* calls).
In line 15 I added context information to the current type checker environment. This allows the type checker reporting engine to show a more concrete error message if a type check fails.
The new bug is that we actually type check recursively and that the context information is passed down into all sub-expressions. So basically every type error somewhere in a else branch will show this way too specific error message. Ouch.
So what we really want is to limit the context information only to the one type check of the result type. So I tried to replace line 15-17 with the following:
This code splits the type check in two parts. In line 1-2 we type check the else-branch with a fresh type variable and in line 4-5 we add the missing restriction that our if and else branches must have the same type. This actually works nicely and limits the error message to exactly the one type check that we want.
Fortunately red unit tests on the CI server catched the fact that I introduced a new problem. It took me a while to understand the problem, but it comes down to the fact the the type checker also acts as type inference algorithm. In contrast to C#’s var keyword where types are only inferred from the right side of an assignment the F# compiler also infers types from the context. By splitting the type check into 2 parts we didn’t pass captured type inference information (in overallTy) down into the recursive type check call and therefore some specific situations can’t be inferred anymore. Ouch.
I also forgot to pass the new type checker environment to the UnifyTypes call. This would probably have fixed all tests in the compiler code base, but since we would still change the order of type checking there might be cases that are still broken. So that’s a risk we clearly don’t want to take just for fancy error messages.
So what’s the correct solution then? We still need to limit the context to the last type check but we can’t split the check in this naive way.
One way to do that is to keep track from where the context information is coming. The F# compiler is already using a “range” type to specify locations in the source code.
This allows us to check if the range of reported error matches the range of the whole else-branch and report the corresponding error:
Case closed.
If you are interested in the details then take a look at #1827.