The Object Teams Blog

After my little excursion to detecting resource leaks let me return to our favorite bug: NPE.

Basic support for null annotations was big news at the release of the Eclipse SDK Juno M4.

To fully understand the approach it’s good to sit back and think of what exactly we’re telling the compiler when we add null annotations to a program.

Annotations as filters

One could certainly use the annotations to mean the following:

@NonNull

“I don’t think this variable will ever hold a null, no need to warn me, when I dereference that value.”

@Nullable

“I guess this variable could be null, so please warn me when I forget a null check before dereferencing.”

Interpreted in this way, the annotations would already be helpful, and actually this would rank them in the same category as @SuppressWarnings("null"): no matter what the compiler analyzed up-to this point, please take my word that this thing is / is not dangerous. In both cases we’d ask the compiler to look at certain things and ignore others, because we “know better”.

However, telling the compiler what’s dangerous and what’s not puts the cart before the horse. If I do so, I will be the weakest link in the chain of analysis. I could err, I do err - that’s why I have NPEs in the first place, so I shouldn’t tell the compiler to trust my judgment.

The good news is: when used appropriately null annotations provide a lot more help.

Annotations as an extension of the type system

The essence of static typing

Lets step back and imagine Java wouldn’t have static typing. A variable declaration would be nothing more than a name, introduced -say- using an imaginary keyword var:

   var o;
   o = new Person();
   o.meow();

Right, we could now assign any object, any value, to the variable o and on the other hand we could attempt to invoke any method. Only at runtime will we notice whether the object refered to by o actually has a method meow(). Obviously, this code is unsafe, it could abort saying “Message on understood”. As Java programmers we don’t accept this unsafety, so we use types as a specification:

A statically typed language forces a decision: what values do I want to allow to be bound to the variable? If I declare o as of type Cat the compiler can conclude that “o = new Person();” violates the constraint and cannot be accepted. If, OTOH, I declared o as of type Person the compiler won’t complain at the assignment but typically it will not find a meow() method in a class Person so that line is now illegal. Only if all things match: the declaration, the assignment, and the usage of a variable, only then will the compiler accept the program and certify that this program will not raise “Message not understood”. It’s a trade: constraint for guarantee.
In a statically typed language we are constrained in what we can say, but we gain the guarantee that a certain kind of error will not occur at runtime.

Sounds fair?

More constraints - more guarantees

Standard static typing constrains the program such that values match to the operations we perform with these values, except - there’s a big loop-hole: in traditional object-oriented type systems each class type contains a value that is not suitable for most operations: the value null, which Tony Hoare called his “billion dollar mistake”.

At a closer look, static type checking in Java is founded on a contradiction:

• When analyzing an assignment assume that null is a legal value for every object type.
• When looking at a method call / a field reference assume that null cannot occur (otherwise no unchecked dereference could be considered as legal).

This is exactly, what null annotations fix: they split each traditional object type into two types:

@NonNull Cat

This is the type that contains only cat values

@Nullable Cat

This is the union of the above type and the null-type which contains only one value: null

You can read the type @Nullable Cat as: either a cat or null.

Null warnings vs. type errors

Those users who try the new feature in the JDT may be surprised to see a whole new kind of error messages. While the original goal is to get alerted about potential NPEs, the compiler may now complain with messages like:

Type mismatch: required ‘@NonNull Cat’ but the provided value can be null

The question may arise, why this is reported as an error, even if no NPE can be directly caused at the statement in question. The answer can be deduced from the following analogy:

void foo(Object o, @Nullable Cat c) {
    Cat aCat = o;                 // "Type mismatch: cannot convert from Object to Cat"
    @NonNull Cat reallyACat  = c; // "Type mismatch: required '@NonNull Cat' but the provided value can be null."
}

(The wording of the second message will be still improved to better reflect different kinds of RHS values).

The analogy shows:

• The assignment itself could actually succeed, and even if types don’t match, a language without static typing could actually accept both assignments.
• If, however, the assignment were accepted, all subsequent analysis of the use of this variable is useless, because the assumption about the variable’s type may be broken.

Therefor, a first step towards making NPE impossible is to be strict about these rules. Assigning a value to a @NonNull variable without being able to prove that the value is not null is illegal. Just as assigning an Object value to a Cat variable without being able to prove that the value is indeed a cat is illegal.

Interestingly, for the first assignment, Java offers a workaround:

    Cat aCat = (Cat) o;

Using the cast operator has two implications: we tell the compiler that we “know better”, that o is actually a cat (we do believe so) and secondly, as compiler and JVM cannot fully trust our judgment a check operation will be generated that will raise a ClassCastException if our assumption was wrong.

Can we do something similar for @NonNull conversion? Without the help of JSR 308 we cannot use annotations in a cast, but we can use a little helper:

void foo(Object o, @Nullable Cat c) {
    @NonNull Cat reallyACat  = assertNonNull(c);
}
@NonNull <T> T assertNonNull(T val) {
    if (val == null) throw new NullPointerException("NonNull assertion violated");
    return val;
}

corrected on 2012/03/05

What? We deliberately throw an NPE although the value isn’t even dereferenced? Why that?

The helper mimics exactly what a cast does for normal type conversions: check if the given value conforms to the required type. If not, raise an exception. If the check succeeds re-type the value to the required type.

Here’s an old school alternative:

void foo(@Nullable Cat c) {
    @SuppressWarnings("null") @NonNull Cat reallyACat  = c;
}

(Requires that you enable using @SuppressWarnings for optional errors).

Which approach is better? Throwing an exception as soon as something unexpected happens is far better than silencing the warning and waiting for it to explode sometime later at some other location in the code. The difference is felt during debugging. It’s about blame assignment.
If things blow up at runtime, I want to know which part of the code caused the problem. If I use @SuppressWarnings that part is in stealth mode, and an innocent part of the code will get the blame when it uses the wrong-typed value.

Remember, however, that cast and assertNonNull are not the solution, those are workarounds. Solutions must explicitly perform the check and provide application specific behavior to both outcomes of the check. Just as a cast without an instanceof check is still a land-mine, so is the use of the above helper: NPE can still occur. If you need to dereference a variable that’s not @NonNull you should really ask yourself:

• How can it happen that I end up with a null value in this position?
• How can the application safely and soundly continue in that situation?

These questions cannot be answered by any tool, these relate to the design of your software.

Help the JDT compiler helping you

This post showed you two things you can and should do to help the compiler helping you:

Add null annotations to resolve the contradiction that’s inherent in Java’s type system: a type can only either contain the value null or not contain the value null. Still Java’s type system opportunistically assumes a little bit of both. With annotations you can resolve the ambiguity and state which of the two possible types you mean.

Second, listen to the new type error messages. They’re fundamental to the analysis. If you disregard (or even disable) these messages there’s no point in letting the analysis apply all its sophisticated machinery. From false assumptions we cannot conclude anything useful.

If you apply these two hints, the compiler will be your friend and report quite some interesting findings. For a project that uses null annotations right from the first line of code written, this advice should be enough. The difficult part is: if you have a large existing code base already, the compiler will have a lot to complain. Think of migrating a fully untyped program to Java. You bet you could use some more help here. Let’s talk about that in future posts.

In my previous post I showed the basics of a new analysis that I originally introduced in the JDT compiler as of 3.8 M3 and improved for M5. This post will give yet more insight into this analysis, which should help you in writing code that the compiler can understand.

Flow analysis - power and limitation

An advantage of implementing leak analysis in the compiler lies in the synergy with the existing flow analysis. We can precisely report whether a resource allocation is definitely followed by a close() or if some execution paths exist, where the close() call is by-passed or an early exit is taken (return or due to an exception). This is pretty cool, because it shows exactly those corner cases in your implementation, that are so easy to miss otherwise.

However, this flow analysis is only precise if each resource is uniquely bound to one local variable. Think of declaring all resource variables as final. If that is possible, our analysis is excellent, if you have multiple assignments to the same variable, if assignments happen only on some path etc, then our analysis can only do a best-effort attempt at keeping track of your resources. As a worst case consider this:

  Reader r = new FileReader(f);
  Reader r2 = null;
  while (goOn()) {
     if(hasMoreContent(r)) {
        readFrom(r);
     } else {
        r.close(); // close is nice, but which resource exactly is being closed??
     }
     if (maybe()) {
        r2 = r;
     }             // at this point: which resource is bound to r2??
     if (hasMoreFiles()) {
        r = new FileReader(getFile()); // wow, we can allocate plenty of resources in a loop
     }
  }
  if (r2 != null)
    r2.close();

This code may even be safe, but there’s no way our analysis can keep track of how many resources have been allocated in the loop, and which of these resources will be closed. Which one is the resource flowing into r2 to be closed at the end? We don’t know. So if you want the compiler to help you, pretty please, avoid writing this kind of code

So what rules should you follow to get on terms with the compiler? To understand the mentioned limitation it helps to realize that our analysis is mostly connected to local variables, keeping some status bits for each of them. However, when analyzing variables the analysis has no notion of values, i.e., in the example the compiler can only see one variable r where at runtime an arbitrary number of Reader instances will be allocated, bound and dropped again.

Still, there are three special situations which the analysis can detect:

1
2
3
4
5
6
7

  Reader r = new FileReader("someFile");
  r = new FileReader("otherFile");
  r = new BufferedReader(r);
  Reader r2 = getReader();
  if (r2 != null) {
     r2.close();
  }

1. In line 2 we’re leaking the instance from line 1, because after the assignment we no longer have a reference to the first reader and thus we cannot close it.
2. However, line 3 is safe, because the same reader that is being dropped from r is first wrapped into a new BufferedReader and indirectly via that wrapper it is still reachable.
3. Finally at the end of the example snippet, the analysis can see that r2 is either null or closed, so all is safe.

You see the compiler understands actually a lot of the semantics.

My fundamental advice is:

Resource ownership

Still not every method lacking a close() call signifies a resource leak. For an exact and definite analysis we would need one more piece of information: who owns any given resource?

Consider a group of methods happily passing around some resources among themselves. For them the same happens as for groups of people: diffusion of responsibility:

Well, no, I really thought that you were going to close this thing?!!?”.

If we had a notion of ownership we’d simple require the unique owner of each resource to eventually close that resource. However, such advanced concepts, while thoroughly explored in academia, are lacking from Java. To mitigate this problem, I made the following approximations of an ownership model:

1. If a method allocates a resource, it owns it - initially.
2. If a method obtains a resource by calling another method, it may potentially be responsible, since we cannot distinguish ownership from lending
3. If a method passes a resource as an argument to another method (or constructor), it may or may not transfer ownership by this call.
4. If a method receives a resource as a parameter, it assumes the caller is probably still responsible
5. If a method passes a resource as its return value back to the caller, it rejects any responsibility
6. If a resource is ever stored in a field, no single method feels responsible.
7. If a resource is wrapped in an array we can no longer track the resource, but maybe the current method is still responsible?

In this list, green means: the compiler is encouraged to report anything fishy as a bug. Blue means, we still do the reporting, but weaken the message by saying “Potential resource leak”. Red means, the compiler is told to shut up because this code could only be checked by whole system analysis (which is not feasible for an incremental compiler).

The advice that follows from this is straight-forward:

In this regard, unclean code will actually cancel the leak analysis. If ownership of a resource is unclear, the compiler will just be quiet. So, do you think we should add a warning to signal whenever this happens? Notably, a warning when a resource is stored in a field?

The art of being quiet

Contrary to naive thinking, the art of good static analysis is not in reporting many issues. The art is in making yourself heard. If the compiler just rattles on with lots of uninteresting findings, no one will listen, no one will have the capacity to listen to all that.

A significant part of the work on resource leak analysis has gone into making the compiler quieter. And of course this is not just a matter of turning down the volume, but a matter of much smarter judgment of what the user might be interested in hearing.

By way of two recently resolved bugs (358903 and 368546) we managed to reduce the number of resource leak warnings reported against the sources of the Eclipse SDK from almost 100 down to 8. Calling this a great success may sound strange at first, but that it is.

At the level we reached now, I can confidently encourage everybody to enable this analysis (my recommendation: resource leaks = error, potential resource leaks = warning). The “Resource leak” problems indeed deserve a closer look, and also the potential ones could give valuable hints.

For each issue reported by the compiler you have three options:

1. Agree that this is a bug
2. Explain to the compiler why you believe the code is safe (unique assignment to locals, less passing around)
3. Add @SuppressWarnings("resource") to tell the compiler that you know what you are doing.

But remember the nature of responsibility: if you say you don’t want to here any criticism you’d better be really sure. If you say you take the responsibility the compiler will be the humble servant who quietly forgets all worries.

Finally, if you are in the lucky position to use Java 7 for your projects, do the final step: enable “Resource not managed with try-with-resource” analysis. This was actually the start of this long journey: to let the compiler give hints where this new syntax would help to make your code safer and to make better visible why it is safe - with respect to resource leaks.

Final note: one of the bugs mentioned above was only resolved today. So with M5 you will still see some avoidable false positives. The next build from now should be better

I’ll be back, soon, with more on our favorite exception: NPE.