Community
Participate
Working Groups
I20100810-0800 While generifying the JDT plug-ins, I found some places where I couldn't get rid of warnings because the used API was still on 1.4 (e.g. in TestRunnerViewPart in org.eclipse.jdt.junit). I would like to have a new compiler option to ignore unavoidable type safety problems due to raw APIs. These warnings are not interesting until the API is generified, but they should not be completely suppressed, since they are only applicable as long as the used API stays raw. @SuppressWarnings(..) should only be used to hide "real" problems. The new option should modify the existing two options that generate problems for raw types and unchecked operations (e.g. COMPILER_PB_UNAVOIDABLE_GENERIC_TYPE_PROBLEMS, enabled by default). Below is an example that shows various kinds of type safety problems that should be ignorable. Note that I don't make a difference on whether 'Top' is declared in a 1.4 or in a 1.5 project (although the former will be the common use case). package problems; import java.util.List; class Top { public void set(List arg) { } // OK to warn in 1.5 code public List get() { return null; } // OK to warn in 1.5 code } class Sub extends Top { @Override public void set(List arg) { // should not warn (overrides) super.set(arg); arg.set(0, "A"); // should not warn ('arg' is forced raw) } @Override public List get() { // should not warn (overrides) return super.get(); } } public class GenericWarnings { void run() { new Top().get().add("arg"); // should not warn (uses raw API) List raw= new Top().get(); // OK to warn ('raw' declared here) raw.add("arg"); // OK to warn ('raw' declared here) // When Top#get() is generified, both of the following will fail // with a compile error if type arguments don't match: List<String> unchecked= new Top().get(); // should not warn (forced) unchecked.add("x"); // Should not warn about unchecked cast, but should warn about // unnecessary cast: List<String> cast= (List<String>) new Top().get(); cast.add("x"); } }
To make the request more explicit: The new option should hide: 1.) problems in method declarations that override a method with raw types in its signature 2.) problems in method invocations whose receiver is a raw method parameter that needs to stay raw because of (1) 3.) problems in method invocations whose receiver has a raw type and the declaration of the receiver is in a different compilation unit than the invocation 4.) unchecked conversion from raw to parametrized type where the raw expression is raw because of (1), (2), or (3) (e.g. initializer of variable 'unchecked' in the example)
*** Bug 163093 has been marked as a duplicate of this bug. ***
Bug 163093 is one instance of the general problem. This problem makes it really hard to convert code to use generics when prerequisite APIs have not been generified yet (i.e. it hampers our progress with generifying JDT/UI).
Srikanth, could you please investigate?
It would be great to have this for M3. M4 otherwise.
(In reply to comment #5) > It would be great to have this for M3. M4 otherwise. Just got back from my time off. I have a few things tagged for M3 already that I am working through. I'll see if I can accommodate this one, if not surely for M4 as early in the cycle as possible. Thanks.
(In reply to comment #1) > To make the request more explicit: The new option should hide: > 1.) problems in method declarations that override a method with raw types in > its signature I meant: Problems in *the parameter and return types of* method declarations...
Created attachment 184023 [details] A very early version. This patch: - Defers reporting of raw type usages on method arguments until after method contract verification happens and we are able to determine whether a method overrides/implements a super type method. - New behavior is not optional at this point - needs to be fixed. - If super type did use parameterized type and the overriding method used raw types, we don't complain - needs to be fixed. - Only method argument case is handled as of now, so that in the following case with the patch: interface Adaptable { public Object getAdapter(Class clazz); // we complain here } public class X implements Adaptable { @Override public Object getAdapter(Class clazz) { // no complaint here return null; } }
(In reply to comment #7) > (In reply to comment #1) > > To make the request more explicit: The new option should hide: > > 1.) problems in method declarations that override a method with raw types in > > its signature > > I meant: Problems in *the parameter and return types of* method declarations... Markus, a couple of questions: Why can't the return type be fixed by using an unbounded wildcard in the method declaration ? As in: import java.util.List; class Base { List get() { System.out.println("Base"); return null; } } public class Blah extends Base { List<?> get() { // Avoid raw type warning by using ? System.out.println("Blah"); return null; } public static void main(String [] args) { Base b = new Base(); b.get(); b = new Blah(); b.get(); } } Since method return type is not a part of method signature, it should be ok to generify that without affecting override equivalence ? (In reply to comment #1) > 2.) problems in method invocations whose receiver is a raw method parameter > that needs to stay raw because of (1) I wonder if these should be suppressed in any case, i.e even without the new option kicking in. After all the way to fix is the warning is to either fix the type parameter which must also have been warned against already, or introduce a cast which will promptly warned against.
(In reply to comment #9) > option kicking in. After all the way to fix is the warning is to either fix > the type parameter which must also have been warned against already, or I meant ... fix the type of the parameter ...
(In reply to comment #1) > 4.) unchecked conversion from raw to parametrized type where the raw expression > is raw because of (1), (2), or (3) (e.g. initializer of variable 'unchecked' in > the example) This one I feel a bit queasy about as most of the unchecked conversion warnings are essentially forced and unavoidable ones, if interoperability between 1.4 and 1.5 code is desired and the 1.4 pieces cannot be generified right now for what ever reason. The very purpose of the warning is to force the programmer to think about type safety and reason to his/her satisfaction that the warning is harmless. The language mandates these warnings in specific scenarios and also makes strong guarantees about type safety in the absence of such warnings. I think suppressing these options even under an option is not the right thing to do. While seasoned programmer's may surf through, we may be causing the not so seasoned programmers to shoot themselves in their foot. Why can't a intra-linguistic device such as SuppressWarnings suffice here ?
Created attachment 184108 [details] Patch v0.2 More progress made in this patch. (In reply to comment #8) > - New behavior is not optional at this point - needs to be > fixed. Per comment# 0 I have introduced a new option: org.eclipse.jdt.core.JavaCore.COMPILER_PB_UNAVOIDABLE_GENERIC_TYPE_PROBLEMS This is enabled by default. (i.e complaint behavior identical to say 3.6) The corresponding batch compiler option token is "unavoidableGenericProblems" > - If super type did use parameterized type and the overriding > method used raw types, we don't complain - needs to be fixed. This is fixed. Javadoc is barely there. This passes all JDT/Core tests.
(In reply to comment #12) > Per comment# 0 I have introduced a new option: > org.eclipse.jdt.core.JavaCore.COMPILER_PB_UNAVOIDABLE_GENERIC_TYPE_PROBLEMS With this patch, the option does not work in a Java project for 2 reasons: - CompilerOptions#OPTION_ReportUnavoidableGenericTypeProblems should start lowercase - missing code at end of CompilerOptions#set(Map): if ((optionValue = optionsMap.get(OPTION_ReportUnavoidableGenericTypeProblems)) != null) { if (ENABLED.equals(optionValue)) { this.reportUnavoidableGenericTypeProblems = true; } else if (DISABLED.equals(optionValue)) { this.reportUnavoidableGenericTypeProblems = false; } } See bug 331447 for the UI patch.
(In reply to comment #9) > > 1.) problems in the parameter and return types of method declarations that > > override a method with raw types in its signature > > Why can't the return type be fixed by using an unbounded wildcard > in the method declaration ? That would work in that specific case (if List<?> is really the right parametrization in Base, otherwise it could also be List<String> or the like). However this won't work as soon as the right parametrization for a method has more constraints that also tie in the method parameter types, e.g.: class Base<E> { List<E> get(E o) { ... } <T> List<T> perturb(List<T> l, T o) { ... } } In such cases, you can't add type arguments to the return type without also parametrizing the method parameters. > > 2.) problems in method invocations whose receiver is a raw method parameter > > that needs to stay raw because of (1) > > I wonder if these should be suppressed in any case, i.e even without the new > option kicking in. After all the way to fix is the warning is to either fix > the type of the parameter which must also have been warned against already, or > introduce a cast which will promptly warned against. This is the "Unchecked generic type operation" warning that is stipulated by the JLS. That option should stay as close as possible with the spec, and we shouldn't deviate without a separate option. I still think the new option is a good way to cover this.
(In reply to comment #11) > > 4.) unchecked conversion from raw to parametrized type where the raw expression > > is raw because of (1), (2), or (3) (e.g. initializer of variable 'unchecked' in > > the example) > > Why can't a intra-linguistic device such as SuppressWarnings suffice here ? The goal is that the client code doesn't need to be polluted with unnecessary SuppressWarnings annotations that are only there because the API contains raw types. I agree that this hides some safety checks that would be there in pure 1.5 code, but since 1.4 APIs are in the game, we're unsafe anyway. So the new option (which is inactive by default) just shifts the type safety border a little more into the 1.5 code. The nice point about the proposed option is that as soon as the raw API gets generified, you either get compile errors (in cases where the client guessed a wrong type argument), or the problem is gone even if you revert the option. Compare this to adding @SuppressWarnings("unchecked") everywhere: - code is harder to read - can't put the annotation on all statements (e.g. I can't easily suppress only 'arg.set(0, "A");', where arg is a raw List) - unnecessary annotation stays after API is generified - if I had to put the annotation on the enclosing method, this will hide all problems in the whole method -- even relevant ones that I would like to see because they really point out a problem
Moving to M5 as the new option in the UI will be added post M4. We should be consistent and release the core side at the same time we release the UI side.
(In reply to comment #13) > (In reply to comment #12) > > Per comment# 0 I have introduced a new option: > > org.eclipse.jdt.core.JavaCore.COMPILER_PB_UNAVOIDABLE_GENERIC_TYPE_PROBLEMS > > With this patch, the option does not work in a Java project for 2 reasons: > > - CompilerOptions#OPTION_ReportUnavoidableGenericTypeProblems should start > lowercase Thanks, will fix it. > - missing code at end of CompilerOptions#set(Map): > if ((optionValue = optionsMap.get(OPTION_ReportUnavoidableGenericTypeProblems)) > != null) { > if (ENABLED.equals(optionValue)) { > this.reportUnavoidableGenericTypeProblems = true; > } else if (DISABLED.equals(optionValue)) { > this.reportUnavoidableGenericTypeProblems = false; > } > } This code is here in the patch already. Things worked for me when I had manually edited the preferences file to disable this option (I had been testing with the initial capital letter).
(In reply to comment #16) > Moving to M5 as the new option in the UI will be added post M4. > We should be consistent and release the core side at the same time we release > the UI side. We are ready when you are (the patch is already available).
(In reply to comment #14) > > I wonder if these should be suppressed in any case, i.e even without the new > > option kicking in. After all the way to fix is the warning is to either fix > > the type of the parameter which must also have been warned against already, or > > introduce a cast which will promptly warned against. > > This is the "Unchecked generic type operation" warning that is stipulated by > the JLS. That option should stay as close as possible with the spec, and we > shouldn't deviate without a separate option. You are right, I was actually looking at the wrong line of code (super.set(arg)) and was puzzled as to why a warning there would have value. We don't warn there of course.
Created attachment 184788 [details] Patch v0.3 In this patch: > - CompilerOptions#OPTION_ReportUnavoidableGenericTypeProblems should start > lowercase This is fixed. (In reply to comment #1) > To make the request more explicit: The new option should hide: > 1.) problems in method declarations that override a method with raw types in > its signature > 2.) problems in method invocations whose receiver is a raw method parameter > that needs to stay raw because of (1) These should work now.
Created attachment 184931 [details] Proposed patch + tests This patch addresses all the four scenarios discussed in comment# 1 and the behavior matches the annotated source code from comment# 0. Markus, please give it a spin. Let me know if something is amiss -- Thanks.
I got this NPE when I tried to compile a partly-generified o.e.jdt.ui: java.lang.NullPointerException at org.eclipse.jdt.internal.compiler.ast.ASTNode.checkInvocationArguments(ASTNode.java:346) at org.eclipse.jdt.internal.compiler.ast.AllocationExpression.resolveType(AllocationExpression.java:369) at org.eclipse.jdt.internal.compiler.ast.LocalDeclaration.resolve(LocalDeclaration.java:182) at org.eclipse.jdt.internal.compiler.ast.AbstractMethodDeclaration.resolveStatements(AbstractMethodDeclaration.java:463) at org.eclipse.jdt.internal.compiler.ast.MethodDeclaration.resolveStatements(MethodDeclaration.java:212) at org.eclipse.jdt.internal.compiler.ast.AbstractMethodDeclaration.resolve(AbstractMethodDeclaration.java:422) at org.eclipse.jdt.internal.compiler.ast.TypeDeclaration.resolve(TypeDeclaration.java:1147) at org.eclipse.jdt.internal.compiler.ast.TypeDeclaration.resolve(TypeDeclaration.java:1248) at org.eclipse.jdt.internal.compiler.ast.TypeDeclaration.resolve(TypeDeclaration.java:1046) at org.eclipse.jdt.internal.compiler.ast.TypeDeclaration.resolve(TypeDeclaration.java:1257) at org.eclipse.jdt.internal.compiler.ast.CompilationUnitDeclaration.resolve(CompilationUnitDeclaration.java:540) at org.eclipse.jdt.internal.compiler.Compiler.process(Compiler.java:759) at org.eclipse.jdt.internal.compiler.ProcessTaskManager.run(ProcessTaskManager.java:137) at java.lang.Thread.run(Thread.java:619)
A null check is required as the receiver can be null. Once the null check is added, the code compiles fine.
Even with the "|| receiver == null" check, the patch only solves some of the issue in comment 0. As a real life example, for org.eclipse.jdt.junit from HEAD, I wouldn't expect any remaining problem in the "Type Safety and Raw Types" category (Problems view grouped by Java Problem Type). The latest patch solves none of these problems.
(In reply to comment #24) > Even with the "|| receiver == null" check, the patch only solves some of the > issue in comment 0. > > As a real life example, for org.eclipse.jdt.junit from HEAD, I wouldn't expect > any remaining problem in the "Type Safety and Raw Types" category (Problems > view grouped by Java Problem Type). The latest patch solves none of these > problems. The patch uses the test case in comment# 0 as the starting point and the warning behavior now matches the annotated code. If it solves none of the problems in the real life example, it could be perhaps those cases are not modeled by the test case. It could also be that the patch as it stands is too specific and doesn't handle certain common variant strains. I'll study the diagnostics that show up in org.eclipse.jdt.junit, extract these variants and write junits for them.
(In reply to comment #23) > A null check is required as the receiver can be null. Once the null check is > added, the code compiles fine. Added org.eclipse.jdt.core.tests.compiler.regression.GenericsRegressionTest.test322817k to verify that we don't fault with NPE for this case.
(In reply to comment #24) > Even with the "|| receiver == null" check, the patch only solves some of the > issue in comment 0. Did you turn on this option either at the workspace level or > As a real life example, for org.eclipse.jdt.junit from HEAD, at least for this project ? > I wouldn't expect > any remaining problem in the "Type Safety and Raw Types" category (Problems > view grouped by Java Problem Type). The latest patch solves none of these > problems. I don't get any of these warnings if the option is properly disabled for the project either locally or globally.
(In reply to comment #27) > (In reply to comment #24) > > Even with the "|| receiver == null" check, the patch only solves some of the > > issue in comment 0. > > Did you turn on this option either at the workspace level or > > > As a real life example, for org.eclipse.jdt.junit from HEAD, > > at least for this project ? (in the patch you seem to have shared with Olivier to reproduce the problem, this option is not in force for org.eclipse.jdt.junit)
Created attachment 185200 [details] Revised patch Same as earlier patch with - null check for receiver to prevent NPE. - test to ensure there is no NPE. This patch builds all of JDT/UI HEAD successfully. With this patch I don't get any warnings in jdt.junit project when the option is properly disabled. Markus, appreciate continued testing and feedback.
(In reply to comment #29) With this patch, everything looks good now in org.eclipse.jdt.junit. The problems I saw were probably due to an inconsistent state after the NPEs. > Even with the "|| receiver == null" check, the patch only solves some of the > issue in comment 0. Sorry, that was a false alarm. I was just too lazy and only copy-pasted the whole snippet as a single CU. When I put the classes into separate CUs, everything looks good. I'll continue working with the patch while generifying jdt.ui. 'Go' from my side for releasing it (though I didn't check the implementation at all).
(In reply to comment #30) > I'll continue working with the patch while generifying jdt.ui. 'Go' from my > side for releasing it (though I didn't check the implementation at all). Thanks, I'll get it review and release it later this week. Any issues can be addressed by follow up defects.
Jay, can you please review this patch ? TIA.
(In reply to comment #29) > Created an attachment (id=185200) [details] > Revised patch Patch looks good to me. Just a minor comment - If we are sure that the ForcedToBeRawType flag wouldn't have been set inside LocalVariableBinding's constructor, do we really require the explicit clearing? Not a big deal but just wondering.
(In reply to comment #33) > (In reply to comment #29) > > Created an attachment (id=185200) [details] [details] > > Revised patch > > Patch looks good to me. Just a minor comment - If we are sure that the > ForcedToBeRawType flag wouldn't have been set inside LocalVariableBinding's > constructor, do we really require the explicit clearing? Not a big deal but > just wondering. I have removed that line and released the patch on HEAD for 3.7 M5. Thanks!
Verified in I20110124-1800. NOTE: It is important is to paste each class from comment 0 into a separate CU.
Verified for 3.7RC0 using I20110428-0845
