new check: type safe arrays #157

Open
isopov opened this Issue Nov 17, 2013 · 2 comments

Comments

Projects
None yet
3 participants
Owner

isopov commented Nov 17, 2013

SF issue: 612

Task

Disallow arrays to be passed to fields that are declared as a supertype (super class or interface). Only arrays of the exact same type as declared are be allowed.
Example

    final Number[] integers = new Integer[1];
    integers[0] = Double.valueOf(1.2d);

Only assignments would be checked (and method calls, when the array is assigned to a parameter). The check is simple: The Types must be equal:

    X[] x = new X[size]; // OK
    X[] x = new Y[size]; // Not OK!
    X[] x = (X[]) new Y[size]; // OK! as user do it intentionally

Description

(Note that those two lines of code would not have to be consecutive, they could even be in different units/methods.)
I would prefer a check that would mark the first line since the types do not match.
An alternative would be to mark the second line, to have the same as with generic collections. But that could also be achieved with generic arrays, but those are often just a lot of trouble and would just be replaces with Lists.
(Note that this check would have absolutely nothing to do with generics or collections, this is just to explain the problem! The check would only apply to "normal" arrays.)
The same thing would be done like this:

    final List<? extends Number> integers2 = new LinkedList<Integer>();
    integers2.add(Double.valueOf(1.2d));

Double is an instance of Number, but the actual type of the collection is not known. Therefore it is not allowed to add a Double.
Another example is this function:

    public static void set(final int index, final Number value, final Number[] array) {
        assert value != null && array != null;
        assert array.getClass().getComponentType().isAssignableFrom(value.getClass());
        array[index] = value;
    }
    // possible invocation that will fail:
    set(0, Double.valueOf(1.2d), new Integer[1]);

In this case it would be better to define a generic type.
But the check would mark the unsafe assignment is inside that function. This would be just like the same function with a generic list:
Parameter is List<? extends Number> and then list.set(index, value).
The generic version of the method would not solve the problem and still only throw an exception at runtime:

    public static <N extends Number> void set(final int index, final N value, final N[] array)

Then there are assignments to array fields (arr[42] = foo):
I already have given the check in the function above as an assertion (using isAssignableFrom).
This can only be done at runtime but becomes irrelevant if Checkstyle already asserts that the type of the arrays are correct.
Functions that should not declare a concrete type can still use generics:

    public static <N extends Number> N get(final int index, final N[] array) {
        assert array != null;
        return array[index];
    }

Rationale

Helps for better type safety. There would be a guarantee that any Array of type Foo[] would actually be of exactly that type and not any subtype.
Alternative:
Use (misuse?) of generics:

    public static <X extends Number> void main(final String args[]) {
        final X[] integers = (X[]) new Integer[1];
        integers[0] = Double.valueOf(1.2d);
        // Type mismatch: cannot convert from Double to X
    }

Use if (generic) collections (e.g. List<? extends Number>).

Further reading

http://www.ibm.com/developerworks/java/library/j-jtp01255/index.html
http://c2.com/cgi/wiki?JavaArraysBreakTypeSafety

radsaggi commented Mar 7, 2014

I was just wondering whether it is currently possible to implment this check as you have described...
If there is a function call to an external library, I dont think Checkstyle can currently determine the formal parameter type in the function...

Please do correct me if I am wrong.

Owner

isopov commented Mar 7, 2014

Yes - this check is dubious someway. You can though read the string value of the local variable, check if is not a generic type parameter and than check if assignment is correct. I doubt the need in such check and is it worth the effort. There also can be some nuances that I don't know of that can make this check impossible in Checkstyle.

However this is not the main Checkstyle project and it is fine to include here some checks on a "let's try" basis.

@daniilyar daniilyar added moderate and removed enhancement labels Aug 15, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment