Java compiler choosing wrong overload [duplicate]

Question

@Test
public void test() {
    MyProperties props = new MyProperties();
    props.setProperty("value", new Date());

    StringUtils.isNullOrEmpty(props.getProperty("value"));
}

public class MyProperties {
    private Map<String, Object> properties = new HashMap<String, Object>();

    public void setProperty(String name, Object value) {
        properties.put(name, value);
    }

    @SuppressWarnings("unchecked")
    public <T> T getProperty(String name) {
        return (T) properties.get(name);
    }
}

public class StringUtils {

    public static boolean isNullOrEmpty(Object string) {
        return isNullOrEmpty(valueOf(string));
    }

    public static String valueOf(Object string) {
        if (string == null) {
            return "";
        }
        return string.toString();
    }

    public static boolean isNullOrEmpty(String string) {
        if (string == null || string.length() == 0) {
            return false;
        }
        int strLength = string.length();
        for (int i = 0; i < strLength; i++) {
            char charAt = string.charAt(i);
            if (charAt > ' ') {
                return true;
            }
        }
        return false;
    }

}

For years, this unit test has been passing. Then after upgrading to Java 8, in certain environments, when the code is compiled via javac, it chooses the StringUtils.isNullOrEmpty(String) overload. This causes the unit test to fail with the following error message:

java.lang.ClassCastException: java.util.Date cannot be cast to java.lang.String at com.foo.bar.StringUtils_UT.test(StringUtils_UT.java:35)

The unit tests passes when compiled and run on my machine via ant (ant 1.9.6, jdk_8_u60, Windows 7 64bit) but fails on another with the same versions of ant and java (ant 1.9.6 jdk_8_u60, Ubuntu 12.04.4 32bit).

Java's type inference, which chooses the most specific overload from all applicable overloads when compiling, has been changed in Java 8. I assume my issue has something to do with this.

I know the compiler sees the return type of the MyProperties.getProperty(...) method as T, not Date. Since the compiler doesn't know the return type of the getProperty(...) method, why is it choosing StringUtils.isNullorEmpty(String) instead of StringUtils.isNullorEmpty(Object) - which should always work?

Is this a bug in Java or just a result of Java 8's type inference changes? Also, why would different environments using the same version of java compile this code differently?

Answer 1

This code smells. Yes, this passes under Java 7, and yes it runs alright with Java 7, but there is something definitely wrong here.

First, let's talk about this generic type.

@SuppressWarnings("unchecked")
public <T> T getProperty(String name) {
    return (T) properties.get(name);
}

Can you at a glance infer what T should be? If I run those casts in Java 7 compliance mode with IntelliJ at that exact line, I get back this very helpful ClassCastException:

Cannot cast java.util.Date to T

So this implies that at some level, Java knew there was something off here, but it elected to change that cast instead from (T) to (Object).

@SuppressWarnings("unchecked")
public <T> Object getProperty(String name) {
    return (Object) properties.get(name);
}

In this case, the cast is redundant, and you get back an Object from the map, as you would expect. Then, the correct overload is called.

Now, in Java 8, things are a bit more sane; since you don't truly provide a type to the getProperty method, it blows up, since it really can't cast java.util.Date to T.

---

Ultimately, I'm glossing over the main point:

This use of generics is broken and incorrect.

You don't even need generics here. Your code can handle either a String or an Object, and your map only contains Objects anyway.

You should only return Object from the getProperty method, since that's what you can only return from your map anyhow.

public Object getProperty(String name) {
    return properties.get(name);
}

It does mean that you no longer get the ability to call directly into the method with a signature of String (since you're passing an Object in now), but it does mean that your broken generics code can finally be put to rest.

---

If you really want to preserve this behavior though, you would have to introduce a new parameter into your function that actually allowed you to specify which type of object you wanted back from your map.

@SuppressWarnings("unchecked")
public <T> T getProperty(String name, Class<T> clazz) {
    return (T) properties.get(name);
}

Then you could invoke your method thus:

StringUtils.isNullOrEmpty(props.getProperty("value", Date.class));

Now we are absolutely certain as to what T is, and Java 8 is content with this code. This is still a bit of a smell, since you're storing things in a Map<String, Object>; if you've got the Object overridden method and you can guarantee that all objects in that map have a meaningful toString, then I would personally avoid the above code.

Answer 2

Java 8 does have improved target type inference. This means that the compiler will use the target type to infer the type parameter.

In your case, this means that in this statement

StringUtils.isNullOrEmpty(props.getProperty("value"));

Java will use the parameter type of isNullOrEmpty to determine the type parameter of the getProperty method. But there are 2 overloads of isNullOrEmpty, one taking an Object and one taking a String. There is no bound on T, so the compiler will choose the most specific method that matches -- the overload that takes a String. T is inferred to be String.

Your cast to T is unchecked, so the compiler allows it, but it gives you an unchecked cast warning about casting an Object to a T. However, when the isNullOrEmpty method is called, the class cast exception is thrown, because the original object was really a Date, which can't be converted to a String.

This illustrates the dangers of ignoring the unchecked cast warning.

This didn't occur in Java 7, because the improved target type inference didn't exist. The compiler inferred Object.

The improved target type inference in Java 8 has revealed that your getProperty method is incorrectly ignoring the unchecked cast warning that you're suppressing with @SuppressWarnings.

To fix this, don't even have an overloaded method that takes a String. Move the String-specific logic inside the overload that takes an Object.

public static boolean isNullOrEmpty(Object o) {
    // null instanceof String is false
    String string = (o instanceof String) ? ((String) o) : valueOf(o);
    if (string == null || string.length() == 0) {
        return false;
    }
    int strLength = string.length();
    for (int i = 0; i < strLength; i++) {
        char charAt = string.charAt(i);
        if (charAt > ' ') {
            return true;
        }
    }
    return false;
}

Of course this means that the generics on the getProperty method are meaningless. Remove them.

public Object getProperty(String name) {
    return properties.get(name);
}