eclipse-jdt / eclipse.jdt.core

Eclipse Public License 2.0
157 stars 125 forks source link

Ecj doesn't report rawtype warning while javac does #1651

Open alienisty opened 9 months ago

alienisty commented 9 months ago

The following simple class:

public class TestCase {
    @SuppressWarnings({"unchecked"})
    private List<String>[] elements = new List[48];

    @Override
    public String toString() {
        return Arrays.toString(elements);
    }
}

if compiled with javac -Xlint will report a "rawtypes" warnings for the new List[48] statement, while ecj doesn't.

srikanth-sankaran commented 6 months ago

Simpler test case:


public class X<T> {

    Object o = new X[48]; // no warning here
    X x;                              // warning here

}
srikanth-sankaran commented 6 months ago

OK, I know why we don't report a warning there. We don't because it is not actionable!

You can't change the code wrt to the warning in any meaningful way to make the warning go away.

For instance wrt to javac warning what can you do ?

If you change that line to:

private List<String>[] elements = new List<String>[48];

you will fare worse. From warning you will now advance to an error!

error: generic array creation
    private List<String>[] elements = new List<String>[48];
                                      ^
1 error

So emitting a warning there is useless.

alienisty commented 6 months ago

@srikanth-sankaran what you can do is to add "raw type" to the @SuppressWarning values, which is what I have to do to have this compile under javac with the warning enabled. I'm not sure what do you mean by not being actionable, but you can definitely do something about it. I believe this should be reopened and addressed by raising the warning.

srikanth-sankaran commented 6 months ago

So you want the compiler to raise a warning only to force programmers to either put up with it or force them to suppress it with an annotation ??

That would upset some non-trivial number of programmers is my prediction.

By actionable I mean alter the code to avoid behavior warned about. Not just suppress the warning which is what the compiler is already being sweet enough to do even without an annotation 😊

A raw type warning makes sense ONLY in places generic type argument could be supplied. NOT in a place where providing generic type argument would be an error.

Array creation is one such place.

alienisty commented 6 months ago

@srikanth-sankaran is not that I want to, but I believe that ejc should align with javac behaviour, The reason for that is that you normally would not use ejc in build pipelines and, believe me, is more annoying discovering problems after eclipse shows that it is all good.

To me giving a hint to the compiler (i.e. using suppression annotation) is an action to avoid the warning, because sometimes we need to tell the type system that, despite the use is dangerous or not verifiable, we are taking responsibility for the fact that is safe to do so, an argument similar to when you have to use @SafeVarargs.

Those are my rationales and I'd be surprised if many developer would not welcome this change.

srikanth-sankaran commented 6 months ago

You are missing a crucial difference. In javac raw type warnings must be opted into. While in ecj they are reported out of the box. If ecj starts emitting this warning now, many projects will suddenly start seeing a new warning and they will have to add suppression annotations.

Besides as already pointed out raw type warnings make sense only in places where type arguments are missing. Not where they are illegal.

It is a goal to align with the JLS fully and JVMS fully and with the reference compiler - javac - as much as it makes sense and no more.

It is not a goal to be bug compatible.

Someone could make a case that it is actually a javac bug that it emits a raw type warning here.

srikanth-sankaran commented 6 months ago

Here is an experiment I invite you to do. (I am not in front of my computer)

In a sealed classes permits clause you CANNOT mention type arguments.

See if javac emits a raw type warning there too.

srikanth-sankaran commented 6 months ago

Here is something else. If it is not a declaration with initialization as in the example but simply is an assignment like

elements = new List[10];

Where would the programmer apply the suppress warning annotation ?? It cannot but applied to that line of code. If you apply it to the containing method it would suppress all raw warnings in that method - no ??

alienisty commented 6 months ago

@srikanth-sankaran I am not sure I would call a bug the fact that when javac is configured to warn about "rawtypes" (-Xlint) it does so.

Also, note that eclipse complains if I try to suppress a warning that it doesn't raise, which is equally annoying because in some cases such warnings are legit and useful so I need to keep that check to info level so that I can make a decision on a case by case, but it, nontheless, adds noise in the editor.

I'm not sure I understand your comment about sealed classes, the following:

public sealed class A<T> permits B {
}

final class B<T> extends A<T> {
}

compiles fine with javac -Xlint:all -Xlint:-processing -Werror

In your last comment you can simply do something like:

@SuppressWarnings("rawtypes")
List<String>[] reified = new List[10];
elements = reified;

which, by the way, is also one of the autofix suggested by eclipse itself.

As I said there is disconnect between the IDE and what is used to produce the deliverables. In gradle, for example, it is "possible" to use ejc to compile the code, but that comes at a cost because gradle will not be able to use its build cache. It is also a problem when team members use different IDEs.

So you may think that javac is buggy, but, whether you like it or not, it acts as "certifier" of the code written using different tools.

I believe my rationales are compelling, but I'll leave it up to you.

srikanth-sankaran commented 6 months ago

Make the permitted class generic not the sealed class and report back what javac says about it please. Permits clauses are not allowed to mention type arguments. Does javac complain about raw type there ??

srikanth-sankaran commented 6 months ago

Forcing programmers to convert assignments to new variable declaration with suppress warning annotation followed by assignment is DOA :-(

Summary. Javac warning there is not useful. It is not worth emulating.

srikanth-sankaran commented 6 months ago

@alienisty if you want I can reopen this and unassign myself.

May be there are other JDT committers who may find your arguments convincing.

alienisty commented 6 months ago

@srikanth-sankaran compiling with javac -Xlint:all -Xlint:-processing -Werror

public sealed class A permits B {
}

final class B<T> extends A {
}

produces no warnings.

Anyways, I personally think that this kind of quirks are part of the reasons people is moving away from eclipse, I know they would contribute my move away from it. As much as I can see your points the reality is that javac is the tool generally used in build systems (gradle, maven, etc) and I think it's very important to be able to have my IDE aligned with my build system because I want to have consistent results when I build my code with either. So if you don't agree it's fine, but I would like this to be reopened and left to someone else judgement, if you don't mind. Thank you for your time discussing this, it's been quite insightful getting another perspective.

srikanth-sankaran commented 6 months ago

Reopening for another opinion.