Bug 85815 - [1.5] warn when raw iterator is used
Summary: [1.5] warn when raw iterator is used
Status: CLOSED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.1   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 3.1 RC2   Edit
Assignee: Philipe Mulet CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-02-18 07:44 EST by Nikolay Metchev CLA
Modified: 2005-06-10 10:15 EDT (History)
3 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolay Metchev CLA 2005-02-18 07:44:48 EST
the following code doesn't flag any typesafty warnings:
--------------------------------
import java.util.ArrayList;
import java.util.Iterator;
import java.util.List;

public class Test
{
   void m()
   {
      List<String> l = new ArrayList<String>();
      final Iterator iterator = l.iterator(); //this assignment should warn
      while (iterator.hasNext())
      {
         String element = (String) iterator.next(); //unecessary cast
      }
   }
}
--------------------------------
when converting 1.4 code to 1.5 you get typesafty warnings however if you use a
1.4 style iterator you get no typesafty warnings. You don't get any warnings
from javac either so I suspect I am missing something but I would like eclipse
to tell me if the above situation occurs so that I can convert the whole method
to use generics  and remove the unecessary cast!.
Comment 1 Martin Aeschlimann CLA 2005-02-18 09:15:28 EST
+ 1 for that. Not only iterators, all usages of raw types should be warned. Or
is there a good reason for new code to use raw types?
Comment 2 Frederic Fusier CLA 2005-02-18 09:26:17 EST
Comment 0 sample includes also issue reported in bug 85393 (unnecessary cast).
Comment 3 Nikolay Metchev CLA 2005-02-18 10:31:16 EST
the cast only becomes unecessary when the raw type isn't used. If you change
final Iterator iterator = l.iterator()
to
final Iterator<String> iterator = l.iterator()
then eclipse does correctly issue a warning.
Comment 4 Philipe Mulet CLA 2005-02-24 06:13:46 EST
Assigning a parameterized to a raw type doesn't require any unchecked
conversion, as there is no type safety issue there.

As said in comment#3, the cast only becomes unnecessary when the raw type is
removed.

Now, I am not disagreeing that an extra warning could be useful there since it
is likely causing grief in the end. Also I was expecting the use of #next() to
be flagged as a usage of a raw type method. This maybe the best thing, since if
not using the raw type in any way, you do not need to worry.
Comment 5 Nikolay Metchev CLA 2005-02-24 08:56:45 EST
as long as there is some warning it will be alright because the warnings will
propagate.
Comment 6 Philipe Mulet CLA 2005-02-24 12:48:55 EST
Warning is currently issued when invoking raw method, when it has substituted
arguments, and does not discriminate on return type (as per spec). I believe
this is intended to be such, since if you attempt to use the rawified return
type, you will get an unchecked warning due to unchecked conversion.

This is however somewhat misleading, since error prone (you may not realise your
mistake until you try to use the value somehow).
I am enclined to think that the raw method invocation warning should be provided
as soon as using a method from raw type with either arguments/return type
substituted.

In which case, we would see for your testcase:
----------
1. WARNING in X.java (at line 10)
	String element = (String) iterator.next();
	                          ^^^^^^^^^^^^^^^
Type safety: The method next() belongs to the raw type Iterator. References to
generic type Iterator<E> should be parameterized
----------

How does it sound ? The alternative would be to special case the cast expression
for it to detect this situation and barf accordingly.
Comment 7 Nikolay Metchev CLA 2005-02-24 13:01:02 EST
I agree with comment 6.
Comment 8 Philipe Mulet CLA 2005-02-25 09:39:18 EST
Tuned test results. Implemented behavior described in comment#6.
Fixed
Comment 9 Markus Keller CLA 2005-03-10 10:59:37 EST
From bug 86902: I disagree with the decision to merge the warning for general
raw type usage with the 'unchecked' aka 'Type Safety' warning.

Consider this class:
class Cell<T> {
    T t;
    public void setT(T t) { this.t= t; }
    public T getT() { return t; }
}

And a client:

        Cell<Boolean> bc= new Cell<Boolean>();
        Cell c= bc;
        c.setT("Hello"); //javac: unchecked

After this point, a call to bc.getT() may throw a ClassCastException! Therefore,
the 'unchecked' warning above are required by JLS3 5.1.9, which specifies that
an 'unchecked' warning is issued for every unchecked conversion from a raw type
to a parameterized type.

        bc.setT("Hello"); //javac & eclipse: error
        
On the other hand, there's never a type safety issue with a getter invocation.
The only issue there is that the return type of the raw method call may not be
as specific as one would like. This may require an additional cast (which is and
has always been unchecked).

        Boolean b1= (Boolean) c.getT();
        Boolean b2= (Boolean) bc.getT(); //eclipse: unnecessary cast

I suggest to only issue 'Type Safety' warnings when the JLS3 wants them (and
thus stay in sync with javac).

In order to flag raw types, a separate warning should be introduced, which can
 (a) flag all occurrences of raw types, or
 (b) flag calls to methods on a raw target, where the method return type
contains a type variable (i.e. getter calls, such as iterator.next() in comment 0).
Comment 10 Philipe Mulet CLA 2005-03-10 11:10:46 EST
Unchecked warnings are not only issued when unchecked conversion occurs.
Invocation of raw methods fall into this category as well, amongst others. As
for staying consistent with javac, remember they have quite a number of bugs
(check their database) in this area, so it is not always a good indication.

This being said, we could discriminate our unchecked warnings in various
categories, waiting for enough requests to go through the effort. I suspect we
could end up in a situation where there is fine granularity, but in the end no
one understands the nuances.

In the example here, the issue was that invoking on a raw type:
  T foo()
  void bar(T t)
would issue no warning in first case, and one in second. It doesn't strike to
also flag the situation when the return type is altered by raw conversion.
Comment 11 Nikolay Metchev CLA 2005-03-10 11:30:23 EST
End users perspective:
I originally raised this issue because I was migrating some 1.4 Java code to
1.5. I was hoping that eclipse would make my life easier but it was missing this
warning.
As far as I was concerned it didn't matter if it was a type safty warning or
not. What mattered more was which code is now outdated. The example I gave is
clearly now legacy code (provided you are allowed to use java 1.5). Maybe the
warnings could be called "Legacy code style" warnings because that is truly what
the end user cares about rather than the fact that there is the usage of raw type.
Comment 12 Philipe Mulet CLA 2005-03-10 14:55:35 EST
Markus - to clarify. I am not opposed to introduce flexibility to configure how
much type safety you want to achieve. I am simply wondering if once you are
interested in any, you aren't actually interested in all.
Comment 13 Markus Keller CLA 2005-03-11 03:41:44 EST
CC-ing Dirk for comments about one vs. two user options, and one vs. two problem
ids (one for javac's and JLS3's "unchecked", and one for other usages of raw types).
Comment 14 Philipe Mulet CLA 2005-03-11 04:30:20 EST
In this situation, the extra option would be:
something like "rawified method return type"

But we could indeed have to flag which would be minimalistic, and maximimalistic.
Comment 15 Dirk Baeumer CLA 2005-03-11 05:31:25 EST
A comment form someone you wasn't deeply involved so far:

I don't know if we are doing yourselfes a favour if we generate different type
safety warnings than javac. Although I would like to have only one option in the
UI I can live with two if we want to generate the same set of type safety
warnings than javac.
Comment 16 Philipe Mulet CLA 2005-03-11 06:58:48 EST
The 2 levels idea is seducing me: Bare minimum or more hints.
Reopening for further consideration
Comment 17 Philipe Mulet CLA 2005-03-11 06:59:38 EST
This is along the lines of our deprecation warning: all or some.
Comment 18 Markus Keller CLA 2005-03-15 11:48:07 EST
IMO, the "more hints" mode should just flag all occurrences of raw types. In the
example below, neither eclipse nor javac give a warning (and in fact, the code
snippet has no type safety problem).

Still, the code is not fully generified, and a warning for method invocations on
raw types would not flag anything here.

        Vector<Vector> nested= null; // note: nested Vector is raw
        
        Vector<String> vs= null;
        vs.add("Eclipse");
        
        Vector<Double> vd= null;
        vd.add(3.1);
        
        nested.add(vs);
        nested.add(vd);
Comment 19 Philipe Mulet CLA 2005-06-06 06:55:18 EDT
I buy this story. Will revert to old behavior which is actually the spec'ed one.

Unclear we can still provide a better warning at this stage (3.1RC2), but at
least we will behave from spec standpoint.
Comment 20 Philipe Mulet CLA 2005-06-06 07:14:38 EDT
Entered bug 98491 for extra diagnosis on raw types.

Tuned compiler back to not consider substituted return type for methods of raw
types. Adjusted the various tests exposing offending behavior.

Fixed
Comment 21 Philipe Mulet CLA 2005-06-06 15:46:16 EDT
Fixed
Comment 22 Frederic Fusier CLA 2005-06-07 08:56:47 EDT
Verified for 3.1 RC2 using build N20050607-0010 + JDT/Core HEAD.
Comment 23 Jerome Lanneluc CLA 2005-06-10 10:15:03 EDT
Verified with I20050610-0010