Menu ▾ ▴

#301 type safe arrays

3.0.1
closed-rejected
William Pugh
arrays (1) type safety (1) covariance (1)
5
2015-02-17
2014-06-14
Claude Martin
No

There is this feature request I once wrote for "checkstyle". But checkstype is about style and not so much about finding bugs. Maybe this is something for FindBugs?

Before I post this I have tested to see if FindBugs doesn't already find it but to my surprise this is not marked as a problem:

    Number[] numbers = new Integer[] { 1, 2, 3 };
    numbers[0] = 3.141; // this fails at runtime

Here's the link to the original request for checkstyle:
http://sourceforge.net/p/checkstyle/feature-requests/612/

Below the line is a copy of the original request:

Description:

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 to be allowed.

Example:

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

(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)

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!

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:

1: 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
  }

2: Use of (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

Discussion

  • Andrey Loskutov

    Andrey Loskutov - 2014-06-19
    • Group: 3.0.0 --> 3.0.1
     

  • Andrey Loskutov

    Andrey Loskutov - 2014-11-12

    I would see this is as "correctness", "low priority" rank 20 warning in case no write happens after array creation or array is of type Object[].

    If at least one write happens and array is not of type Object[] I would see this as "correctness", "medium priority" rank 10 warning.

    If additionally we know the written type is incompatible (and we know the real array type and this is not Object[]) I would see this as "correctness", "high priority" rank 1 warning.

    "Correctness" because it is neither "dodgy code" nor "bad practice" but "don't know better".

    Does it make sense?

     

  • Tagir Valeev

    Tagir Valeev - 2014-11-13

    Sounds reasonable, though need to think about details. For example, covariant array assignment to a field and array element writing may occur in different classes, so we may need to analyze the whole project before reporting bugs. Ok, I'll take this task.

     

  • Tagir Valeev

    Tagir Valeev - 2014-11-13
    • status: open --> open-accepted
    • assigned_to: Tagir Valeev
     

  • Tagir Valeev

    Tagir Valeev - 2014-11-18

    I implemented this, though it reports in somewhat different way. Everything in new detector named CovariantArrayAssignment. There's a special check method allImplementationsDerivedFromSubclass which checks whether all known non-abstract subclasses of declared array type are also subclasses of actual array type. For example if you have an interface ISomething and only implementation class Something and your array is defined as ISomething[] something = new Something[10]. In this case ArrayStoreException will never occur unless you add new implementation class.

    There are 4 new bug patterns:

    CAA_COVARIANT_ARRAY_LOCAL - when covariant array is assigned to a local variable. It's always Low priority/20
    CAA_COVARIANT_ARRAY_FIELD - when covariant array is assigned to a field. It's Low/20 if the field is private or package protected or class is private or allImplementationsDerivedFromSubclass is true. Otherwise it's Normal/17.
    CAA_COVARIANT_ARRAY_RETURN - when covariant array is returned from the method. It's Low/20 if the method is private or package protected or class is private or allImplementationsDerivedFromSubclass is true. Otherwise it's Normal/17.

    I decided to put all of these patterns in "Dodgy code" as such assignments don't cause the problems directly, but make the code error-prone.

    One more pattern is CAA_COVARIANT_ARRAY_ELEMENT_STORE (correctness category) - when an element is stored into covariant array. There are three cases here:

    • If allImplementationsDerivedFromSubclass is true, then bug is not reported, because no ArrayStoreException will occur in current project and it's enough that we already reported a bug when the array was created.
    • If value class is the superclass of actual array class, then it's Normal/12. In this case ArrayStoreException may or may not occur.
    • If value class and actual array class are completely unrelated, then it's High/10. In this case ArrayStoreException will surely occur if this line is executed.

    I decided not to merge covariant array assignment and array element store to the single warning, because this is a little bit harder to implement. Current report looks ok for me.

    The only 100% ArrayStoreException I found in tested projects is here:
    http://grepcode.com/file/repository.grepcode.com/java/eclipse.org/4.4.0/org.eclipse.core/resources/3.9.0/org/eclipse/core/internal/resources/MarkerSet.java#232

    protected IMarkerSetElement[] elements;

public void shareStrings(StringPool set) {
    //copy elements for thread safety
    Object[] array = elements;
    if (array == null)
        return;
    for (int i = 0; i < array.length; i++) {
        Object o = array[i];
        if (o instanceof String) // impossible instanceof
            array[i] = set.add((String) o); // <<< set.add returns String
        if (o instanceof IStringPoolParticipant)
            ((IStringPoolParticipant) o).shareStrings(set);
    }
}

    But impossible instanceof is reported in the previous line, thus ArrayStoreException is also impossible.

    The code along with the testcase is committed. Feel free to comment if you have any feedback.

     

    Last edit: Tagir Valeev 2014-11-18

  • Tagir Valeev

    Tagir Valeev - 2014-11-18
    • status: open-accepted --> closed-fixed
     

  • Andrey Loskutov

    Andrey Loskutov - 2014-11-20
    • Group: 3.x --> 3.0.1
     

  • William Pugh

    William Pugh - 2015-02-17

    Not seen as much of a problem in practice, generates a clear exception when it does cause a problem, there is code that works perfectly fine that uses this pattern.

    Not enabling by default in 3.0.1.

     

  • William Pugh

    William Pugh - 2015-02-17
    • status: closed-fixed --> closed-rejected
    • assigned_to: Tagir Valeev --> William Pugh
     

Log in to post a comment.

MongoDB Logo MongoDB