It has been a long time since the last item went on the code pranger. I’ve pulled my hair out over many things during the last months, but this is the first time in a long while that I slapped my forehead and wanted to run away.
The issue was first discovered by one of our users and submitted as a bug report [1]. Let me first emphasize that this is clearly a bug in our DrJava code base, not in the submitted sample:
public class CompilerErrors {
int shared = 5;
public void sampleMethod() {
int x = 20;
System.out.println("This is sampleMethod. x is " + x);
}
Note in particular that the closing brace is missing, so we would expect something like “} expected” or “reached end of file while parsing”. The user noticed, however, that when he compiled the following file with Java 6, he got an internal DrJava error instead.
The problem is that we are using String messages to pass the compiler errors and exceptions thrown in the compiler, and then we try to separate these two categories again: Compiler errors are displayed in the “Compiler Output” pane, exceptions are reported as an error in the “DrJava Errors” window.
How did we decide to distinguish errors and exceptions? Using a very unstructured string comparison:
if (message != null && (message.indexOf("CompilerError") >= 0)) throw new UnexpectedException(message);
If the word “CompilerError” occurs in the error message, it is treated as exception. Since the position of the “CompilerError” substring isn’t restricted, any silly compiler error in a file named CompilerError.java, or CompilerErrorModel.java, or CompilerErrorModelTest.java, or CompilerErrorPanel.java — all of which exist in the DrJava code base! — will be treated as serious unexpected exception in the compiler.
It is a really bad idea to use common words that could occur in user-generated text, like file names, as a marker for error conditions.
The solution for this problem, of course, was to analyze the structure of the error messages more closely: The class name of the exception, fully qualified as sun.tools.java.CompilerError had to appear as part of the string "Compile exception: sun.tools.java.CompilerError" at the beginning of the message. Spaces are not allowed in Java class names, so this cannot clash with a user generated file name. I changed the conditional above to:
// need to precisely match the CompilerError message, otherwise a file name containing
// "CompilerError" may trigger an UnexpectedException (see bug 2872797)
if (message != null && message.startsWith("Compile exception: sun.tools.java.CompilerError"))
throw new UnexpectedException(message);
Two additional lessons to be learned from this are that we need to run our tests with both Java 5 and Java 6, and that we need to eat our dog food even more: We should compile DrJava inside DrJava.
Fixed as of revision 5105. I have made a new jar available: drjava-weekly-20091006-r5105.jar [2].
