Bug 335953 - No warning for wrongly typed map get
Summary: No warning for wrongly typed map get
Status: VERIFIED INVALID
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.7   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 3.7 M6   Edit
Assignee: Olivier Thomann CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-01 08:04 EST by Juergen Weber CLA
Modified: 2015-04-02 17:31 EDT (History)
5 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Juergen Weber CLA 2011-02-01 08:04:34 EST
Build Identifier: 20100917-0705

There is no warning if you call map.get(type) with wrong type for parametrized collections.
In the code below there should be a warning or error for m.get(String).


import java.util.HashMap;
import java.util.Map;

public class C
{
	public static void main(String[] args)
	{
		Map<C,C> m =  new HashMap<C,C>();
		m.get("C");
	}
}

Reproducible: Always
Comment 1 Ayushman Jain CLA 2011-02-01 08:34:28 EST
What warning are you looking for exactly? javac compiles the code fine without any warnings, so does eclipse.
Comment 2 Juergen Weber CLA 2011-02-01 08:38:49 EST
I'd expect the same error as the one that is right now given for put:
"The method put(C, C) in the type Map<C,C> is not applicable for the arguments (String, C)"

Map<C,C> m =  new HashMap<C,C>();
m.get("C");
m.put("C", new C());

The second line should give the same error/warning as the third.
Comment 3 Olivier Thomann CLA 2011-02-01 08:43:24 EST
There is nothing wrong with this code. The signature of the get method is:
V java.util.Map.get(Object)
where V is the type variable for the key.
The signature of put is:
java.util.Map.put(K, V) which is different.
Closing as INVALID.
Comment 4 Juergen Weber CLA 2011-02-01 08:50:57 EST
Olivier, you're right, the code is correct. Still, you get an exception at runtime.
In Preferences/Java/Compiler/Errors you can set lots of warnings for code that compiles fine.
So I do think Eclipse should warn for the sample I gave and prevent the programmer from making a programming error. The sample shows definitely a "Potential programming problem".
I reopen the bug as enhancement request.
Comment 5 Juergen Weber CLA 2011-02-01 08:53:57 EST
I correct myself. There is no runtime exception, it returns just null.
Still, it is a Potential programming problem.
Comment 6 Olivier Thomann CLA 2011-02-01 08:58:32 EST
(In reply to comment #5)
> I correct myself. There is no runtime exception, it returns just null.
> Still, it is a Potential programming problem.
Yes and no. As long as you get an error on put(..), there is no way you can add non null elements to the map for which the type would not be right.
So I don't see how you get a "Potential programming problem". The API clearly says that you can get any kind of Object from the map even if this a map of C, C.
There is no relationship for the compiler between the put(K, V) and the get(Object). The compiler cannot know that the parameter of the get method is supposed to be an instance of C.
Comment 7 Juergen Weber CLA 2011-02-01 09:43:46 EST
I had refactored a Map and changed the key Type. Usually Eclipse helps you by finding errors, but not in this case, I had forgotten to update some Map.get() calls and got errors at runtime.

> The compiler cannot know that the parameter of the get method is
supposed to be an instance of C.

Specifying Map<C,C> is a hint that I'd like to get Cs ...

Also, in C c = m.get("C"); I don't need to cast to C, so the compiler seems to know what is in the map ..

So the compiler could as well warn that I used the wrong key.
Comment 8 Olivier Thomann CLA 2011-02-01 09:51:40 EST
(In reply to comment #7)
> I had refactored a Map and changed the key Type. Usually Eclipse helps you by
> finding errors, but not in this case, I had forgotten to update some Map.get()
> calls and got errors at runtime.
This means you forgot some places where you added elements to the map or you used a raw map to add elements. In this case you should have unchecked warnings at other places of your code. If you don't have any warning in your code and you don't use raw types and you could end up adding a element of a wrong type into the map, then yes we have a bug. In this case, please provide details on how you added elements to the map. But in this case, we should update the title of this bug as it doesn't reflect the actual problem.

> Specifying Map<C,C> is a hint that I'd like to get Cs ...
yes, but the compiler doesn't know that get(Object) means retrieving elements from the map.

> Also, in C c = m.get("C"); I don't need to cast to C, so the compiler seems to
> know what is in the map ..
Of course you don't because a runtime cast is added for you. This is the whole purpose of using generics. My point is that if you use the right type Map<C, C> at all locations where you add elements to the map, you should not be able to add elements of a wrong type inside the map. So retrieving an element should not fail as you cannot add a wrong element.

> So the compiler could as well warn that I used the wrong key.
No, the compiler has no clue that get(Object) is used to retrieve elements from the map. You do as a developer that can read the javadoc. From the compiler point of view, there is nothing wrong.

Closing as INVALID as there is really nothing we can do to give more feedback to the user in this case.
Comment 9 Juergen Weber CLA 2011-02-01 10:00:45 EST
I think you misunderstood my problem. The problem is not with add(), it is with get().

In the code below a C object is added to the Map, but because of a programmer's error the get() call returns null.
And I think, it should be possible, that Eclipse show a warning that you try to get() from the Map with a wrong key type, i.e. a type not in the Map declaration. Eclipse could deduce and warn that this is a "Potential programming problem".

Map<C,C> m =  new HashMap<C,C>();
m.put(new C(), new C());
C c = m.get("C");
Comment 10 Ayushman Jain CLA 2011-02-01 10:26:57 EST
(In reply to comment #9)
> I think you misunderstood my problem. The problem is not with add(), it is with
> get().

Really speaking, the get() is legal in the compiler's understanding and there's nothing wrong with it even when you have changed the key type. This is bacause get takes a parameter of type Object and you're passing a String, which is perfectly ok. What changed is the return type. 

So if you'd written String s = m.get("C") before refactoring, this line will not compile after refactoring because return type changed to C, and C cant be cast to S. With a standalone statement m.get("C"), everything is still the same.

Also, consider what happens if we do introduce a potential problem warning. In a parameterized type T, suppose C is a parameter. You write:

public class C<T>
{
    public <T> void m(T arg)
    {
        Map<T,T> m =  new HashMap<T,T>();

        m.get("C");
    }
}

and when you call m, you call it as:

C<String> c = new C<String>();
c.m();
Comment 11 Ayushman Jain CLA 2011-02-01 10:28:15 EST
(In reply to comment #9)
> I think you misunderstood my problem. The problem is not with add(), it is with
> get().

Really speaking, the get() is legal in the compiler's understanding and there's nothing wrong with it even when you have changed the key type. This is bacause get takes a parameter of type Object and you're passing a String, which is perfectly ok. What changed is the return type. 

So if you'd written String s = m.get("C") before refactoring, this line will not compile after refactoring because return type changed to C, and C cant be cast to S. With a standalone statement m.get("C"), everything is still the same.

Also, consider what happens if we do introduce a potential problem warning. In a parameterized type T, suppose C is a parameter. You write:

public class C<T>
{
    public <T> void m(T arg)
    {
        Map<T,T> m =  new HashMap<T,T>();
        m.put (arg,arg);
        m.get("hello");
    }
}

and when you call m, you call it as:

C<String> c = new C<String>();
c.m("hello");

This is now perfectly legal code!! And we end up with a false warning!

Hence, lets leave it as it is. :)
Comment 12 Ayushman Jain CLA 2011-02-01 10:28:48 EST
Ignore comment 10.
Comment 13 Olivier Thomann CLA 2011-02-01 10:36:11 EST
(In reply to comment #9)
> I think you misunderstood my problem. The problem is not with add(), it is with get().
I am not sure. I got that part.
What I am saying is that there is nothing in the get signature that would let the compiler know that the key is not the wrong type.

As long as you cannot add wrongly typed object to the map, there is nothing wrong on get. If you are using raw types when you add elements to the map, then you should have unchecked warnings at other locations of your code already.
Comment 14 Juergen Weber CLA 2011-02-01 11:54:33 EST
(In reply to comment #13)
> (In reply to comment #9)
> > I think you misunderstood my problem. The problem is not with add(), it is with get().
> I am not sure. I got that part.
> What I am saying is that there is nothing in the get signature that would let
> the compiler know that the key is not the wrong type.
> 
> As long as you cannot add wrongly typed object to the map, there is nothing
> wrong on get. If you are using raw types when you add elements to the map, then
> you should have unchecked warnings at other locations of your code already.

There is nothing wrong with

int n = 0;
n = n;

but Eclipse is helpful and says "The assignment to variable n has no effect"

And it would be nice of Eclipse, too, if it would say "Do you really want to read Map<C,C> with a key of type String?"

But if this is not possible, never mind.
Comment 15 Olivier Thomann CLA 2011-02-02 20:25:04 EST
(In reply to comment #14)
> There is nothing wrong with
> 
> int n = 0;
> n = n;
> 
> but Eclipse is helpful and says "The assignment to variable n has no effect".
This is completely different. The compiler can clearly guess without any ambiguity that this assignment has no effect.

> And it would be nice of Eclipse, too, if it would say "Do you really want to
> read Map<C,C> with a key of type String?"
Yes, this is possible. The compiler doesn't know that the get method for a Map means to read elements from the Map. The signature of the get methods makes it possible to call the code you are calling.
This is type-safe as long as you populate the map using all type-safe operations (no unchecked warnings).

> But if this is not possible, never mind.
I tried to explain many times why this is not possible.

Closing as INVALID.
Comment 16 Satyam Kandula CLA 2011-03-07 06:35:42 EST
Verified for 3.7M6
Comment 17 Markus Keller CLA 2013-06-11 09:07:00 EDT
See bug 410218 for an optional compiler problem for this.
Comment 18 Sigma Moo CLA 2015-04-02 17:31:08 EDT
> > But if this is not possible, never mind.
> I tried to explain many times why this is not possible.
> 
> Closing as INVALID.

Which is why it is so strange that the implementers of IntelliJ managed to have their tool generate this warning anyway.  Moreover I can detect the condition with a visual inspection of the code and some simple reasoning -- if I happen to catch it, that is, which is the issue.  But the tool, which knows everything I do about the codebase (and some things I don't), can't.  I guess it's one of them "AI-hard" problems.