Warnings

[John Hendrikx]

We've done a few fixes in JavaFX trying to reduce the amount of 
warnings, and there was some discussion as to the value of fixing 
certain warnings. Some of these are purely aesthetic (like using the 
diamond operator) but others can hide current or future programming 
problems.

I thought I'd post why I think fixing certain warnings is important for 
the continued health of a large code base.

- Unnecessary casts (on Objects)

Every cast in a code base is a potential place where a 
ClassCastException may lurk.  When a cast is unnecessary, it may seem 
harmless to leave it in.  However, as a code base grows or is 
refactored, a harmless cast hides a potential compilation problem which 
can become a runtime ClassCastException.  Java developers rely a lot on 
the compiler helping them when they do small refactorings to point out 
places where a type no longer matches after a change. Take the code below:

     private Number n;
     private Float f = (Float) n;  // warning unnecessary cast

Changing `n` to a more specific type (let's say `Double`) will not 
generate an error for `f`.  The only thing that happens is that the cast 
is no longer deemed unnecessary and will do an actual cast (resulting in 
CCE at runtime) -- as there is no error reported, the developer may 
easily miss this.

- Unnecessary casts (on primitives)

A cast on a primitive that is not currently required can become a place 
where precision is lost without warning when a refactoring occurs.

     private int x;
     private int y = (int) x;  // warning unnecessary cast

A refactoring that changes `x` to a type like `long` would go unnoticed 
by the compiler.  The version without the unnecessary cast would emit an 
error.

- Unnecessary declared checked exceptions

When a checked exception is declared that is never thrown from the 
underlying code, it is good to remove these to prevent them from 
propagating up the call chain.  Sometimes code bases have a checked 
exception in some deeply nested function that is not actually thrown, 
leading to perhaps top level public API to declare this exception. When 
the problem is discovered, the exception could be removed from dozens of 
places and could even affect public API (which now throws an exception 
that it can't actually throw but can't be removed as it is was already 
made public).

- Missing @Override annotations

Adding these everywhere makes extending super types and refactoring 
safer.  A sub type may declare a method that currently does not override 
any of its super types. If a super type is changed and a method is added 
that has a signature that is compatible with one that is already present 
in a sub type, the compiler will emit a warning that it is missing the 
override annotation alerting you of a potential problem (assuming the 
code base is relatively warning free, or it may get lost in the noise).  
Conversely, removing or changing a currently overriden method would 
result in errors if sub types declared their overriden methods with the 
override annotation, alerting you that this functionality was perhaps 
more broadly used than anticipated.  Without the annotation, the method 
in the sub type would simply change to a non-overriden version without 
any further warning.

- Avoiding indirect access to static methods and fields

Code that accesses static members via `this` or indirectly via a sub 
type that didn't declare it, runs the risk of that access being diverted 
to a new member if one is introduced with the same name without a 
compiler error:

        class A { static final int ONE = 1; }
        class B { }
        class C extends B {
             int one = ONE;  // unqualified access to A.ONE
        }

A "harmless" constant is introduced in B:

        class B { static final int ONE = 0x10000;  }  // 1 in 16-bit 
fixed point

No error occurs, and class C is broken.

A further reason to not allow indirect accesses to static members is 
that it may hide problems where an instance wasn't needed at all (if 
using `this.ONE` for example).

- Using raw types

With the introduction of generic types in Java 1.5, use of raw types 
should be considered something to be always avoided.  Using raw types 
will require casts in places that the compiler could have inferred or 
checked for you if you had used a generic type. Not using a generic type 
is basically risking a ClassCastException in those places (it was one of 
the most common exceptions in the pre-1.5 days).  Further more, the 
compiler can't help you during refactorings.  If a generic declared List 
is changed to contain a different type, there will be compiler errors in 
all the places you interact with the List.  Not so for a raw List; 
failing to fix all the casts will pay a runtime price.

- Unused fields / variables / parameters

All of these may point to a potential problem. It could be that the 
author mistyped something and meant to use one of the unused values; it 
could be that a parameter is genuinely unused and should be removed 
(triggering a potential cascade in callers that may be doing unnecessary 
actions to "provide" this unneeded information); it could be that a 
field is accessed by reflection (in which case that would have meritted 
a BIG warning comment and `SuppressWarnings`); whatever the case, it is 
confusing to future maintainers, may involve unnecessary memory use or 
unnecessary calculations (setting but never reading a value could 
involve a big calculation that may be completely unnecessary).

--John