How do I address unchecked cast warnings?

Question

Eclipse is giving me a warning of the following form:

Type safety: Unchecked cast from Object to HashMap

This is from a call to an API that I have no control over which returns Object:

HashMap<String, String> getItems(javax.servlet.http.HttpSession session) {
  HashMap<String, String> theHash = (HashMap<String, String>)session.getAttribute("attributeKey");
  return theHash;
}

I'd like to avoid Eclipse warnings, if possible, since theoretically they indicate at least a potential code problem. I haven't found a good way to eliminate this one yet, though. I can extract the single line involved out to a method by itself and add @SuppressWarnings("unchecked") to that method, thus limiting the impact of having a block of code where I ignore warnings. Any better options? I don't want to turn these warnings off in Eclipse.

Before I came to the code, it was simpler, but still provoked warnings:

HashMap getItems(javax.servlet.http.HttpSession session) {
  HashMap theHash = (HashMap)session.getAttribute("attributeKey");
  return theHash;
}

Problem was elsewhere when you tried to use the hash you'd get warnings:

HashMap items = getItems(session);
items.put("this", "that");

Type safety: The method put(Object, Object) belongs to the raw type HashMap.  References to generic type HashMap<K,V> should be parameterized.

Solution 1

Wow; I think I figured out the answer to my own question. I'm just not sure it's worth it! :)

The problem is the cast isn't checked. So, you have to check it yourself. You can't just check a parameterized type with instanceof, because the parameterized type information is unavailable at runtime, having been erased at compile time.

But, you can perform a check on each and every item in the hash, with instanceof, and in doing so, you can construct a new hash that is type-safe. And you won't provoke any warnings.

Thanks to mmyers and Esko Luontola, I've parameterized the code I originally wrote here, so it can be wrapped up in a utility class somewhere and used for any parameterized HashMap. If you want to understand it better and aren't very familiar with generics, I encourage viewing the edit history of this answer.

public static <K, V> HashMap<K, V> castHash(HashMap input,
                                            Class<K> keyClass,
                                            Class<V> valueClass) {
  HashMap<K, V> output = new HashMap<K, V>();
  if (input == null)
      return output;
  for (Object key: input.keySet().toArray()) {
    if ((key == null) || (keyClass.isAssignableFrom(key.getClass()))) {
        Object value = input.get(key);
        if ((value == null) || (valueClass.isAssignableFrom(value.getClass()))) {
            K k = keyClass.cast(key);
            V v = valueClass.cast(value);
            output.put(k, v);
        } else {
            throw new AssertionError(
                "Cannot cast to HashMap<"+ keyClass.getSimpleName()
                +", "+ valueClass.getSimpleName() +">"
                +", value "+ value +" is not a "+ valueClass.getSimpleName()
            );
        }
    } else {
        throw new AssertionError(
            "Cannot cast to HashMap<"+ keyClass.getSimpleName()
            +", "+ valueClass.getSimpleName() +">"
            +", key "+ key +" is not a " + keyClass.getSimpleName()
        );
    }
  }
  return output;
}

That's a lot of work, possibly for very little reward... I'm not sure if I'll use it or not. I'd appreciate any comments as to whether people think it's worth it or not. Also, I'd appreciate improvement suggestions: is there something better I can do besides throw AssertionErrors? Is there something better I could throw? Should I make it a checked Exception?

Solution 2

The obvious answer, of course, is not to do the unchecked cast.

If it's absolutely necessary, then at least try to limit the scope of the @SuppressWarnings annotation. According to its Javadocs, it can go on local variables; this way, it doesn't even affect the entire method.

Example:

@SuppressWarnings("unchecked")
Map<String, String> myMap = (Map<String, String>) deserializeMap();

There is no way to determine whether the Map really should have the generic parameters <String, String>. You must know beforehand what the parameters should be (or you'll find out when you get a ClassCastException). This is why the code generates a warning, because the compiler can't possibly know whether is safe.

Solution 3

Unfortunately, there are no great options here. Remember, the goal of all of this is to preserve type safety. "Java Generics" offers a solution for dealing with non-genericized legacy libraries, and there is one in particular called the "empty loop technique" in section 8.2. Basically, make the unsafe cast, and suppress the warning. Then loop through the map like this:

@SuppressWarnings("unchecked")
Map<String, Number> map = getMap();
for (String s : map.keySet());
for (Number n : map.values());

If an unexpected type is encountered, you will get a runtime ClassCastException, but at least it will happen close to the source of the problem.

Solution 4

In Eclipse Preferences, Go to Java->Compiler->Errors/Warnings->Generic types and check the Ignore unavoidable generic type problems check-box.

This satisfies the intent of the question, i.e.

I'd like to avoid Eclipse warnings...

if not the spirit.

Solution 5

You can create a utility class like the following, and use it to suppress the unchecked warning.

public class Objects {

    /**
     * Helps to avoid using {@code @SuppressWarnings({"unchecked"})} when casting to a generic type.
     */
    @SuppressWarnings({"unchecked"})
    public static <T> T uncheckedCast(Object obj) {
        return (T) obj;
    }
}

You can use it as follows:

import static Objects.uncheckedCast;
...

HashMap<String, String> getItems(javax.servlet.http.HttpSession session) {
      return uncheckedCast(session.getAttribute("attributeKey"));
}

Some more discussion about this is here: http://cleveralias.blogs.com/thought_spearmints/2006/01/suppresswarning.html

View more solutions

Comments

[Brett]

If you're using HttpSession like that, check out Brian Goetz's article on the subject: ibm.com/developerworks/library/j-jtp09238.html

[Philip Guin]

If an unchecked cast is unavoidable, a good idea is to tightly couple it with something that logically represents it's type (like an enum or even instances of Class<T>), so you can glance at it and know it's safe.

[blahdiblah]

Related/dupe: Type safety: Unchecked cast

[Raedwald]

possible duplicate of Type safety: Unchecked cast

[JGFMK]

I would add, I found I could only add @SuppressWarnings("unchecked") at the method level that contains the offending code. So I broke the code out to a routine where I had to do this. I always thought you could do this immediately above the line in question.

[Yaser Har]

Unfortunately it's not that easy a situation. Code added.

[palantus]

You can't parameterize it unless you add two Class parameters also. Your signature would be "public static <K,V> HashMap<K,V> checkHash(HashMap<?,?> input, Class<K> keyClass, Class<V> valueClass)". Then you'd use keyClass.isAssignableFrom(key.getClass()) and keyClass.cast(key).

[brady]

I'm not sure it's worth it; it depends on the context. An alternative would be to declare the method like this: Map<?,?> getItems() and do the cast to String in the client code. Properly use of generics assures that a ClassCastException is never raised in the absence of an explicit cast.

[Yaser Har]

Awesome; I think I can combine that with my answer to parameterize it and avoid having to suppress warnings altogether!

[ChenZhou]

+1 for pointing out that it can go on local variables. Eclipse only offers to add it to the whole method...

[Deva]

I came here looking for an answer to a slightly different problem: and you told me exactly what I needed! Thanks!

[Stijn de Witt]

No it doesn't. Actually, this creates two warnings where first there was one.

[ubi]

this stuff is confusing, but i think all you have done is traded ClassCastExceptions for AssertionErrors.

[ubi]

not downvoting, but the wrapper adds precisely nothing over just suppressing the warning.

[ubi]

I added an answer to this thread; future googlers may wish to refer to my answer: stackoverflow.com/questions/509076/…

[Matteo Italia]

the fact that you don't have comment privileges do not allow you to edit others' answers to add your comments; you edit others' answers to improve them in formatting, syntax, ..., not to add your opinion on them. When you'll reach 50 rep you'll be able to comment everywhere, in the meantime I'm quite sure you can resist (or, if you really can't, write your comments to existing answers in your post). (note for others: I write this because I saw - and rejected - his proposed comments-edits to other posts in the moderation tools)

[user102008]

key.isAssignableFrom(e.getKey().getClass()) can be written as key.isInstance(e.getKey())

[Craig B]

Dude, that's definitely not worth it! Imagine the poor sap who has to come back and modify some code with that mess in there. I don't like suppressing warnings, but I think that's the lesser evil here.

[sweetfa]

Eclipse 3.7 (Indigo) has support for adding unchecked to local variables.

[allyourcode]

@thSoft Is that because this only works on newer versions of Java?

[ChenZhou]

@allyourcode I don't think so, AFAIK this works since annotations exist.

[Dan Is Fiddling By Firelight]

It's not just that it's an ugly, confusing, mess (when you can't avoid one copious comments can walk the maintenance programmer through it); iterating over every element in the collection turns the cast from an O(1) to an O(n) operation. This is something that would never be expected and can easily turn into a horrible mystery slowdown.

[Yaser Har]

@DanNeely you are correct. In general, nobody should ever do this.

[Woot4Moo]

@DanNeely compiler warnings exist for a reason, we would all benefit as engineers from not ignoring them.

[Theodore Norvell]

The warning is not just because the compiler does not know that the cast is safe. For example String s = (String) new Object() ; gets no warning, even though the compiler does not know that the cast is safe. The warning is because the compiler (a) does not know that the cast is safe AND (b) will not generate a complete run-time check at the point of the cast. There will be a check that it is a Hashmap, but there will not be a check that it is a HashMap<String,String>.

[Theodore Norvell]

Suppose theHash is of compile-time type Hash<String,String>. If you have String s = theHash.get(x), there is a run-time check that the result is in fact a String. Thus, if for some reason theMap does not map Strings to Strings, there will be an exception eventually thrown. It just won't be at the point of the original cast.

[MByD]

May I ask why? globally disabling a warning will hide other places where this issue is real. adding a @SuppressWarnings doesn't make the code unreadable at all.

[Ti Strga]

Sadly, even though the cast and the warning are for the assignment, the annotation has to go on the variable declaration... So if the declaration and the assignment are in different places (say, outside and inside a 'try' block respectively), Eclipse now generates two warnings: the original unchecked cast, and a new "unnecessary annotation" diagnostic.

[Dan J]

@skiphoppy I think you should update your answer to add some huge warnings saying why this (lovely) generics code should not be used in this context. I think we all agree a big performance hit and a maintenance nightmare are not appropriate here to suppress a build warning.

[Stephen C]

The quote is attributed to the late Professor David Wheeler. en.wikipedia.org/wiki/…

[Patrick]

If it's absolutely critical that it doesn't generate ClassCastExceptions during retrieval AND you're legitimately concerned that the Map might contain the wrong types then I think that would be the only time it would make sense. As stated, though, you're getting the AssertionErrors up front rather than ClassCastExceptions during retrieval, and I must add to the list of doubts that this would ever be worth it.

[MetroidFan2002]

Some comments...the method signature is wrong because it doesn't "cast" a damn thing, it just copies the existing map into a new map. Also, it could probably be refactored to accept any map, and not rely on HashMap itself (i.e. take Map and return Map in the method signature, even if the internal type is HashMap). You don't really need to do the casting or the storage into a new map - if you don't throw an assertion error, then the given map has the right types inside it as of right now. Creating a new map with the generic types is pointless as you could still make it raw and put whatever.

[lukewm]

Ah, OK. Not sure why I thought that.

[Stijn de Witt]

Much, much better answer than the one provided by skiphoppy, for multiple reasons: 1) This code is much, much shorter. 2) This code actually throws ClassCastException as expected. 3) This code does not do a full copy of the source map. 4) The loops can be easily wrapped in a separate method used in an assert, which would easily remove the performance hit in production code.

[Tino]

+1 as this solution does not waste precious code lines.

[Tino]

+1 probably best recipe (easy to understand and maintain) to do it safely with runtime checks

[RenniePet]

Isn't there a possibility that the Java compiler or the JIT compiler will decide that the results of this code is not being used and "optimize" it by not compiling it?

[candied_orange]

I work in an environment where this kind of code is demanded of us. The performance hit is nothing in our case but it sure is ugly to look at. So I developed a way to hide this noise: stuff it all down into a generic method. Put it before your loops and you'll be looking at cleaner code. stackoverflow.com/questions/25618452/…

[Ar5hv1r]

Ah, thanks for this :) I was getting a "uses unchecked or unsafe operations." error in javac, but adding @SuppressWarnings("unchecked") made Eclipse unhappy, claiming the suppression was unnecessary. Unchecking this box makes Eclipse and javac behave the same, which is what I wanted. Explicitly suppressing the warning in the code is much clearer than suppressing it everywhere inside Eclipse.

[Rup]

His five-year-old question? Do you need to do that much work? Given Java has type erasure the second hashmap should be identical to the first at runtime; I think it'd be more efficient and avoid the copy if you just iterated through the entries and verified that they were all instances of strings. Or, TBH, inspect the source of the servlet JAR you're using and verify it only ever puts strings.

[abbas]

To this day I am seeing this warning in projects. His problem was not verification of the type, but a warning caused by a "put" into an uncasted map.

[eel ghEEz]

I see this run-time cast check as a middle ground between compile-time warnings that may be too restrictive in their anticipation of the worst case and the run-time errors that may occur far away from the dangerous modification, as @TheodoreNorvell mentioned. I just learned that using the wildcard ? as a type parameter may relieve from getting a warning during the cast of an instance to a parameterized-type reference. The wildcard's purpose seems to assume that reading Object-like members will be safe and that writing to these members will raise a different compile-time warning.

[Jeff Lockhart]

A workaround for the annotation needing to accompany the local variable declaration, which may be in a different scope on a different line than the actual cast, is to create a local variable within the scope of the cast specifically to perform the cast on the same line as the declaration. Then assign this variable to the actual variable which is in a different scope. This is the method I used also to suppress the warning on a cast to an instance variable as the annotation can't be applied here either.

[GPrathap]

This is really helpful answer.

[GrandOpener]

It's not really dead code if it can potentially throw an exception. I don't know enough about the JIT compilers in use today to guarantee that none of them would mess this up, but I feel fairly confident in saying that they're not supposed to.

[user2219808]

This still doesn't guarantee type safety as the same map is still being used. It might have originally been defined as Map<Object,Object> that just happens to have Strings and Numbers in and then later on if a Boolean is added then the user of this code will get a confusing and rather hard to trace surprise. The only way to guarantee type safety is to copy it into a new map with the requested type that guarantees whats allowed to go into it.

[Tino]

@ErikE Too much. Much more expensive bigger and higher-resolution monitors to give room to all those wasted lines, a bigger desk to put all those bigger monitors onto, a bigger room to put the bigger desk into, and an insightful boss ..

[Tino]

@ErikE Scrollbars, for vi? Are you kidding?

[user7294900]

HashMap is still a raw type, You(/Eclipse) can generify code using HashMap<?, ?> input

[Qw3ry]

One more reason why you should never use this method: If someone did class X<K,V> extends HashMap<K,V> you are actually altering the behaviour of the map after the cast, because x.put(k,v) will call HashMap::put instead of X::put. This can lead to issues that are very hard to debug!

[firstpostcommenter]

This is also necessary incase of clients which connect to graphql. Because graphql can return polymorphic collections and later in Java code when we try to cast the collection to a List of concrete class then we get the sonar warning