Community
Participate
Working Groups
Add an optional compiler warning if arguments of "unexpected" types are passed to java.util.Map#get/remove(Object), Collection#contains/remove(Object), Collection#retain/remove/containsAll(Collection<?>), List#indexOf(Object), etc. Example: Set<Short> set = new HashSet<>(); short one = 1; set.add(one); set.remove("ONE"); // bad (wrong type) set.remove(1); // bad (tries to remove "Integer 1") System.out.println(set); // shows that the "Short 1" is still in! set.remove(one); // ok System.out.println(set); The calls marked as "// bad" compile without error, although they often hide a programming error that would have been caught at compile time if these accessor methods would not accept all types, but just the type variable E (or K or Collection<? extends E> etc.). Unfortunately, the affected accessor methods cannot be determined in a generic way, so we need an explicit list of special methods and their "expected" argument types. For method invocations of a special method, the compiler can then also check whether a variant with the "expected" signature would also be applicable. If it's not applicable, then a warning should be issued. Some background: http://stackoverflow.com/questions/857420/what-are-the-reasons-why-map-getobject-key-is-not-fully-generic http://stackoverflow.com/questions/104799/why-arent-java-collections-remove-methods-generic http://smallwig.blogspot.ch/2007/12/why-does-setcontains-take-object-not-e.html
I wholeheartedly agree. I assigned the bug to me, but I won't start right away, so if s.o. else has cycles to spare and is interested in this bug please let me know.
*** Bug 358209 has been marked as a duplicate of this bug. ***
*** Bug 389014 has been marked as a duplicate of this bug. ***
Stephan, do you expect to work on this for Luna ? ISTM, this is best addressed in the start of Mars ? If you agree, please retarget. Thanks.
Since an optional warning requires additional API it is too late already for 4.4, right? (Other than that, implementation _might_ be straight forward and simple).
Linking to bug 415306, which requests special/similar warnings for equals() of certain well known types.
I have a draft implementation in my workspace. An example warning message would be: ---------- 1. WARNING in X.java (at line 8) if (set.contains("ONE")) // bad ^^^^^ Discouraged invocation of method contains(Object), this argument should have type Short to match the declaring type Collection<Short> ---------- Here's another example I used for experiments: void test(Map<? extends Number, Number> m) { if (m.containsKey("ONE")) // bad m.remove("ONE"); // bad } Recognizing this as bad is easy, too. Question is: what would a user have to write to avoid the new warning? Literally, the "expected" type would be a capture, a value of which is quite difficult to obtain. My initial feeling was, that we may want to silence the warning if the mismatch between "expected" and actual type is only a matter of captures. OTOH, if those methods were declared using a type variable, the strict interpretation would be mandatory, of course. But users will not like the warning if it creates situations that are hard to get right and may still do the right thing. What do you think: is it a relevant concern to avoid warnings involving captures? More questions: What should be the default value for this new diagnostic? Warning? Ignore? I think we want to make it suppressable with @SW, right? Which token? Should we introduce a new token? (Bug 389014 proposed "rawtypes" :-/). Bug 358209 requests a distinction of potentially incompatible vs. incompatible. Should we? I don't have a strong opinion on this, but surfacing both levels as separate options in the UI feels like overkill to me.
(In reply to Stephan Herrmann from comment #7) > Here's another example I used for experiments: > > void test(Map<? extends Number, Number> m) { > if (m.containsKey("ONE")) // bad > m.remove("ONE"); // bad > } > > Recognizing this as bad is easy, too. Question is: what would a user have to > write to avoid the new warning? Literally, the "expected" type would be a > capture, a value of which is quite difficult to obtain. This particular example is obviously wrong because a String cannot ever be a Number. If you replaced "ONE" with 1, then it becomes more interesting, because the ? could be Integer for some invocations. Still, it seems unclean for a generic method to include specific functionality, so I wouldn't object to inflicting some SuppressWarnings pain on people who are doing this. I'm curious, what are the rules about casting - would a compile-time cast from the capture type [or equivalent type variable] to Integer be accepted? What about the other way? This is relevant if the 'incompatible' vs 'potentially incompatible' distinction is preserved (see further down). > My initial feeling was, that we may want to silence the warning if the > mismatch between "expected" and actual type is only a matter of captures. > OTOH, if those methods were declared using a type variable, the strict > interpretation would be mandatory, of course. But users will not like the > warning if it creates situations that are hard to get right and may still do > the right thing. > > What do you think: is it a relevant concern to avoid warnings involving > captures? Personally I would say not, because this (your example tweaked to use only Numbers): void test(Map<? extends Number, Number> m) { if (m.containsKey(1)) // [potentially?] bad m.remove(1); // [potentially?] bad } has exactly the same level of potential wrongness as this: void test(Map<Short, Number> m) { if (m.containsKey(1)) // bad m.remove(1); // bad } And in fact, it's less obvious that the first example is wrong, so perhaps the warning is even more useful there. In a real use case, perhaps test() should be taking the '1' as an extra argument (and using a type parameter): void test<T extends Number>(Map<T, Number> m, T removeMe) > More questions: > > What should be the default value for this new diagnostic? Warning? Ignore? Warning!!! It's so rare to need to pass a strange type deliberately. Oh by the way though, perhaps if the method is just a wrapper like the following, then you might want to suppress the warning (I don't see any need to make this optional). class MySet<T> implements Set<T> { public bool contains(Object t) { //implements Set<T> return wrappedList.contains(t); //bad but unavoidable } } > I think we want to make it suppressable with @SW, right? Which token? Should > we introduce a new token? (Bug 389014 proposed "rawtypes" :-/). Not rawtypes :) Those methods were declared in terms of Object instead of T for a non-rawtypes-related reason: for example, HashSet<T>.equals(TreeSet<T>) can return true, and therefore so can HashSet<HashSet<T>>.contains(TreeSet<T>). It's just that those cases are much rarer than the common case of passing the wrong key by accident. I can't see an existing token that's correct - not even "unchecked" since this has nothing to do with heap pollution. Meanwhile, "serial" and "sync-override" are very specific, so it looks as if it's fine to make a new very specific one. Perhaps something like "keytypes"? > Bug 358209 requests a distinction of potentially incompatible vs. > incompatible. Should we? I don't have a strong opinion on this, but > surfacing both levels as separate options in the UI feels like overkill to > me. That was my proposal, and it's to do with the HashSet<T>.equals(TreeSet<T>) case above. So if you write this: HashSet<HashSet<T>>.Contains(TreeSet<T>) then the new warning will kick in because a TreeSet<T> cannot be a HashSet<T>. However, my reasoning was, if you cast it like this: HashSet<HashSet<T>>.Contains((Set<T>)TreeSet<T>) then it can be classed as only 'potentially incompatible', and perhaps users will disable the 'potentially' warning if they need to do this. Surfacing the two options might not be overkill if you do it using an indented checkbox: Incorrect key type passed to Map.get et al [Ignore/Warning/Error] [X] Allow arguments that could be cast to the correct type Then it still looks like a single concept and doesn't use up too much space. Here's another thought actually: perhaps we don't even need the 'potentially' case, i.e. if the argument could be cast to the correct key type then no warning. That would also save you having to write a special case for wrappers.
Created attachment 251444 [details] draft (In reply to Missing name from comment #8) Thanks, Anonymous, for detailed comments. I see that my considerations about captures were a bit off. Just because it's hard to supply an argument of the required type doesn't make for a good excuse :) Instead I now included a distinction between - argument can be casted to the "expected" type -> softly discouraged - argument cannot be casted to the "expected" type -> strongly discouraged In deviation from standard casting rules I'm admitting a boxing operation before the compatibility check. > > What should be the default value for this new diagnostic? Warning? Ignore? > > Warning!!! It's so rare to need to pass a strange type deliberately. That's my take, too. Let's see if anyone objects. > Oh by > the way though, perhaps if the method is just a wrapper like the following, > then you might want to suppress the warning (I don't see any need to make > this optional). > > class MySet<T> implements Set<T> { > public bool contains(Object t) { //implements Set<T> > return wrappedList.contains(t); //bad but unavoidable > } > } That's actually a situation for which we need a good story, if we want to enable the warning by default. * It doesn't make much sense to insert an unchecked cast "(T) t". * @SW would be the natural answer, if this occurs infrequently. * We could also treat j.l.Object as a tolerable catch-all (In that case users should know that they don't know what they are doing). Would that be part 3 of the option?? > > I think we want to make it suppressable with @SW, right? Which token? Should > > we introduce a new token? (Bug 389014 proposed "rawtypes" :-/). > > Not rawtypes :) Those methods were declared in terms of Object instead of T > for a non-rawtypes-related reason: for example, > HashSet<T>.equals(TreeSet<T>) can return true, and therefore so can > HashSet<HashSet<T>>.contains(TreeSet<T>). It's just that those cases are > much rarer than the common case of passing the wrong key by accident. > > I can't see an existing token that's correct - not even "unchecked" since > this has nothing to do with heap pollution. Meanwhile, "serial" and > "sync-override" are very specific, so it looks as if it's fine to make a new > very specific one. Perhaps something like "keytypes"? I agree that none of the existing tokens really match. "keytypes" would cover only the K from Map, not the V and T arguments. Still taking bets. Let me brainstorm a bit what it really *is*, before condensing into a short token: the library methods are "over generalized", resulting in "dangerous signatures", which falsely admit arguments that semantically don't match. "problem-signature" > That was my proposal, and it's to do with the HashSet<T>.equals(TreeSet<T>) > case above. > > So if you write this: > HashSet<HashSet<T>>.Contains(TreeSet<T>) > then the new warning will kick in because a TreeSet<T> cannot be a > HashSet<T>. However, my reasoning was, if you cast it like this: > HashSet<HashSet<T>>.Contains((Set<T>)TreeSet<T>) > then it can be classed as only 'potentially incompatible', and perhaps users > will disable the 'potentially' warning if they need to do this. We're slightly mixing two things here: the reason that both types are admissible is in the implementation of their equals methods. The fact that both have a non-trivial common super type has little to do with this (except, that Set<T> indeed happens to establish this contract of equality). Obviously, inserting a cast to j.l.Object is the escape door, allowing to soften all warnings to "potential". That's in fact similar to how nested casts can circumvent the checks for uncastable types. > Surfacing the two options might not be overkill if you do it using an > indented checkbox: > > Incorrect key type passed to Map.get et al [Ignore/Warning/Error] > [X] Allow arguments that could be cast to the correct type > > Then it still looks like a single concept and doesn't use up too much space. Looks good. At the bottom line, to me the wording seems to be the hard part. This concerns the @SW token and also the warning messages. Tests in the attachment show the following messages in action: 1200 = Discouraged argument for method {0}, should be of type {1} to match the declaring type {2} 1201 = Discouraged argument for method {0}, provided type {1} is definitely incompatible to type {2}. Declaring type is {3} Here's an alternative pair of proposals: 1200 = Invoking method {0} with an argument of type {1} is discouraged. Should use {2} to meet the semantics of declaring type {3} 1201 = Invoking method {0} with an argument of type {1} cannot meet the semantics of declaring type {3}. Recommended type: {2} I really want to show: (a) what's wrong, (b) what should it be instead, and (c) why. For the "why" I'm including the declaring type (that's where we see the effective parameterization).
(In reply to Stephan Herrmann from comment #9) > Instead I now included a distinction between > - argument can be casted to the "expected" type -> softly discouraged > - argument cannot be casted to the "expected" type -> strongly discouraged > In deviation from standard casting rules I'm admitting a boxing operation > before the compatibility check. Sounds good on the face of it; I'm sure you've thought about boxing in more depth than I have. By the way, not sure if you used it in any messages, but we just say 'cast', not 'casted' (it's irregular). > > class MySet<T> implements Set<T> { > > public bool contains(Object t) { //implements Set<T> > > return wrappedList.contains(t); //bad but unavoidable > > } > > } > > That's actually a situation for which we need a good story, if we want to > enable the warning by default. > > * It doesn't make much sense to insert an unchecked cast "(T) t". > > * @SW would be the natural answer, if this occurs infrequently. > > * We could also treat j.l.Object as a tolerable catch-all (In that case > users should know that they don't know what they are doing). Would that be > part 3 of the option?? The Object solution would affect people who have Map<String,Object> - which does happen, though it's not that common. Still, what I had in mind is you could check if the argument is a field access expression which resolves to an affected parameter of an affected method. If that's not too onerous then I think it is better. I can't think of any cases it doesn't catch; can you? > I agree that none of the existing tokens really match. "keytypes" would > cover only the K from Map, not the V and T arguments. Still taking bets. Good point. > Let me brainstorm a bit what it really *is*, before condensing into a short > token: the library methods are "over generalized", resulting in "dangerous > signatures", which falsely admit arguments that semantically don't match. > > "problem-signature" Too general I'd say, and it also draws attention to the signature (not in the user's control) rather than the invocation (which is). Generalising directly from "keytypes", "argtypes" is closer but it's still too general. What these methods all have in common is that they never store the Object you pass in, they just use it for comparisons. Not sure if that helps us at all. I just had a read of http://stackoverflow.com/questions/857420/what-are-the-reasons-why-map-getobject-key-is-not-fully-generic too in the hope of some ideas, perhaps some terminology we haven't thought of. I haven't found a great deal, but terms like liberal, restrictive... "liberal-arg-type"? Another way we could look at it is, while there is a reason why these strange argument types are accepted, they are "unlikely" to be correct. "unlikely-arg-type"? If more cases are found in the future that have nothing to do with this but are still examples of someone passing an argument of a type that seems unlikely to be right, then I think it could share @SW quite validly with this one. Aside: it's reassuring to see how many people have upvoted the comments we agree with :) > > So if you write this: > > HashSet<HashSet<T>>.Contains(TreeSet<T>) > > then the new warning will kick in because a TreeSet<T> cannot be a > > HashSet<T>. However, my reasoning was, if you cast it like this: > > HashSet<HashSet<T>>.Contains((Set<T>)TreeSet<T>) > > then it can be classed as only 'potentially incompatible', and perhaps users > > will disable the 'potentially' warning if they need to do this. > > We're slightly mixing two things here: the reason that both types are > admissible is in the implementation of their equals methods. The fact that > both have a non-trivial common super type has little to do with this > (except, that Set<T> indeed happens to establish this contract of equality). Not sure I understand which two things we're mixing. I think the Set<T> example is merely to explain why someone decided to make these methods take Object instead of T in the first place. It's also definitely the case that 'type cannot be implicitly cast' and 'ditto explicitly' are heuristics, and that they don't take Set<T> into account. (We could do something messy like check if Set<T>.equals() exists and HashSet<T>.equals() doesn't, but that's not foolproof since HashSet<T>.equals() might exist to optimise the 'two HashSets' case without changing semantics, for example. I think our heuristics plus @SW are good enough.) > Obviously, inserting a cast to j.l.Object is the escape door, allowing to > soften all warnings to "potential". That's in fact similar to how nested > casts can circumvent the checks for uncastable types. Yep :) but it's messy if it happens a lot, so @SW is still useful - someone might have a valid reason to use it on a whole method or class. > > Surfacing the two options might not be overkill if you do it using an > > indented checkbox: > > > > Incorrect key type passed to Map.get et al [Ignore/Warning/Error] > > [X] Allow arguments that could be cast to the correct type > > > > Then it still looks like a single concept and doesn't use up too much space. > > Looks good. Cool :) Though now I'm thinking, 'Unlikely key type' may be better? > At the bottom line, to me the wording seems to be the hard part. This > concerns the @SW token and also the warning messages. > > Tests in the attachment show the following messages in action: > > 1200 = Discouraged argument for method {0}, should be of type {1} to match > the declaring type {2} > 1201 = Discouraged argument for method {0}, provided type {1} is definitely > incompatible to type {2}. Declaring type is {3} These are much easier to understand with examples, so I guess that would be: 1200 = Discouraged argument for method get(Object), should be of type String to match the declaring type Map<String, Integer> 1201 = Discouraged argument for method get(Object), provided type Class is definitely incompatible to type String. Declaring type is Map<String, Integer> > Here's an alternative pair of proposals: > > 1200 = Invoking method {0} with an argument of type {1} is discouraged. > Should use {2} to meet the semantics of declaring type {3} > 1201 = Invoking method {0} with an argument of type {1} cannot meet the > semantics of declaring type {3}. Recommended type: {2} 1200 = Invoking method get(Object) with an argument of type Object is discouraged. Should use String to meet the semantics of declaring type Map<String, Integer> 1201 = Invoking method get(Object) with an argument of type Class cannot meet the semantics of declaring type Map<String, Integer>. Recommended type: String > I really want to show: (a) what's wrong, (b) what should it be instead, and > (c) why. For the "why" I'm including the declaring type (that's where we see > the effective parameterization). Yes, it makes sense. I like the second proposal for 1200 the most (since it includes the current argument type), and would perhaps tweak 1201 so we can get the recommended type out before the declaring type. Here's a new proposal (sorry for leaving the examples in, it just makes it easier to discuss and refine): 1200 = Invoking method get(Object) with an argument of type Object is discouraged. Should use String to meet the semantics of declaring type Map<String, Integer> 1201 = Invoking method get(Object) with an argument of type Class is unlikely to be correct. Should use String to meet the semantics of declaring type Map<String, Integer>, or consider adding a cast to Object* if correct * That's the most specific common supertype, and obviously if they do it then they'll get warning 1200 if enabled. By the way, if someone does this: class MyVerySpecificMap implements Map<String, Integer> then that will obscure the message, so it may be worth degenerating the 'declaring type' (for message purposes) to the built-in collection interface that originally declared the problematic method - i.e. the message says Map<String, Integer> instead of MyVerySpecificMap. Oh, in the interests of succinctness, you could also remove the words 'declaring type' and 'method' if you want - it's pretty obvious once it's got examples. Hope that helps :)
> > > Incorrect key type passed to Map.get et al [Ignore/Warning/Error] > > > [X] Allow arguments that could be cast to the correct type > > Cool :) Though now I'm thinking, 'Unlikely key type' may be better? Make that 'Unlikely argument type'!
(In reply to Missing name from comment #11) > > > > Incorrect key type passed to Map.get et al [Ignore/Warning/Error] > > > > [X] Allow arguments that could be cast to the correct type > > > > Cool :) Though now I'm thinking, 'Unlikely key type' may be better? > > Make that 'Unlikely argument type'! And 'to the expected type' perhaps?
(In reply to Ben Davis from comment #10) > By the way, if someone does this: > class MyVerySpecificMap implements Map<String, Integer> > then that will obscure the message, so it may be worth degenerating the > 'declaring type' (for message purposes) to the built-in collection interface > that originally declared the problematic method - i.e. the message says > Map<String, Integer> instead of MyVerySpecificMap. That's exactly what the implementation in attachment 251444 [details] does :) I don't yet have tests matching your exact situation, but for all tests using Set<Short> the message mentions the 'declaring type Collection<Short>'. This is done by searching the super type that is a parameterization of one of the well-known trouble makers (Collection and Map, so far) > Oh, in the interests of succinctness, you could also remove the words > 'declaring type' and 'method' if you want - it's pretty obvious once it's > got examples. And the previous point is exactly the reason why I say 'declaring type'. Still users will complain: why is the message talking about Map<String,Integer>? I don't see this type anywhere in the flagged code. I want to give at least some hint where to look.
(In reply to Ben Davis from comment #10) > > * We could also treat j.l.Object as a tolerable catch-all (In that case > > users should know that they don't know what they are doing). Would that be > > part 3 of the option?? > > The Object solution would affect people who have Map<String,Object>. I'm not sure which effect you have in mind? For such map asking containsValue(x) is always OK, with or without any special rule for j.l.Object, but anyway I wouldn't include this rule just now. > Still, what I had in mind is you > could check if the argument is a field access expression which resolves to > an affected parameter of an affected method. If that's not too onerous then > I think it is better. too complex, IMHO. > Another way we could look at it is, while there is a reason why these > strange argument types are accepted, they are "unlikely" to be correct. > > "unlikely-arg-type"? I like this best, so far. Accordingly, the UI proposal would now be s.t. like Unlikely argument type for Map.get() et al [Ignore/Warning/Error] [X] Allow arguments that could be cast to the expected type (I don't think we want to say "argument type passed to"). Maybe "allow" has too strong indication of legality wrt JLS? "Ignore" might be more correct, but also more confusing. I think "accept" would strike a good balance. I'd leave the following as a possible future addition: [X] Accept arguments of type java.lang.Object As for the warning messages, we seem to agree on: 1200 = Invoking method {0} with an argument of type {1} is discouraged. Should use {2} to meet the semantics of declaring type {3} For the stronger message, my initial thought was that uncastable types are definitely wrong. As the Set example shows, this isn't even true. Hence, "uncastability" is only an even stronger indication of an error, not a proof. From that, 1201 could either just emphasize like in "strongly discouraged", or harp more on a connection to the UI option, perhaps by saying: 1201 = Discouraged invocation of method {0}. Argument type {1} cannot be cast to the likely type {2} according to the declaring type {3}. If this agreeable, we could assimilate 1200 by saying 1200 = Discouraged invocation of method {0}. Argument type {1} is incompatible to the likely type {2} according to the declaring type {3}. I'll add tests, which spell these out with examples in my next patch. I was also playing with different orders of parts of the message, but I think the current order succeeds in putting the most important parts first. > 1201 = Invoking method get(Object) with an argument of type Class is > unlikely to be correct. Should use String to meet the semantics of declaring > type Map<String, Integer>, or consider adding a cast to Object* if correct > > * That's the most specific common supertype, and obviously if they do it > then they'll get warning 1200 if enabled. The "or" part is too much, IMHO. I'm not even sure if we want to *recommend* that workaround using a cast.
(Replying to both posts at once) > > Map<String, Integer> instead of MyVerySpecificMap. > > That's exactly what the implementation in attachment 251444 [details] does :) Great, I hadn't checked :) > I don't yet have tests matching your exact situation, but for all tests > using Set<Short> the message mentions the 'declaring type > Collection<Short>'. This is done by searching the super type that is a > parameterization of one of the well-known trouble makers (Collection and > Map, so far) I'm sure it will work. > > Oh, in the interests of succinctness, you could also remove the words > > 'declaring type' and 'method' if you want - it's pretty obvious once it's > > got examples. > > And the previous point is exactly the reason why I say 'declaring type'. > Still users will complain: why is the message talking about > Map<String,Integer>? I don't see this type anywhere in the flagged code. I > want to give at least some hint where to look. You're right, it won't entirely solve the problem you're trying to solve. But I think most people will get it with either version (come on, they KNOW about the Map<A,B> they're using that the warning mentioned), and my feelings about keeping/removing the words are as weak as gravity :) > > The Object solution would affect people who have Map<String,Object>. > > I'm not sure which effect you have in mind? For such map asking > containsValue(x) is always OK, with or without any special rule for > j.l.Object, but anyway I wouldn't include this rule just now. I was thinking of the case where someone accidentally passes a value to a function that expects a key. Since the values are Objects, they'd miss out on a warning. Granted they're already subject to the opposite risk (passing a key where a value was expected), but there's no sense in making it worse. > > Still, what I had in mind is you > > could check if the argument is a field access expression which resolves to > > an affected parameter of an affected method. If that's not too onerous then > > I think it is better. > > too complex, IMHO. Shrug. Wouldn't stop me, but OK :) > > Another way we could look at it is, while there is a reason why these > > strange argument types are accepted, they are "unlikely" to be correct. > > > > "unlikely-arg-type"? > > I like this best, so far. > > Accordingly, the UI proposal would now be s.t. like > > Unlikely argument type for Map.get() et al [Ignore/Warning/Error] > [X] Allow arguments that could be cast to the expected type > > (I don't think we want to say "argument type passed to"). More succinctness - good :) > Maybe "allow" has too strong indication of legality wrt JLS? "Ignore" might > be more correct, but also more confusing. I think "accept" would strike a > good balance. I noticed that too but was too tired (and felt I'd spammed enough) to explore it. I agree - change it to 'Accept'. > I'd leave the following as a possible future addition: > [X] Accept arguments of type java.lang.Object Hardcoded to accept those for now? Mainly so we don't screw container wrappers up. > As for the warning messages, we seem to agree on: > > 1200 = Invoking method {0} with an argument of type {1} is discouraged. > Should use {2} to meet the semantics of declaring type {3} > > For the stronger message, my initial thought was that uncastable types are > definitely wrong. As the Set example shows, this isn't even true. Hence, > "uncastability" is only an even stronger indication of an error, not a proof. > > From that, 1201 could either just emphasize like in "strongly discouraged", > or harp more on a connection to the UI option, perhaps by saying: > > 1201 = Discouraged invocation of method {0}. Argument type {1} cannot be > cast to the likely type {2} according to the declaring type {3}. > > If this agreeable, we could assimilate 1200 by saying > > 1200 = Discouraged invocation of method {0}. Argument type {1} is > incompatible to the likely type {2} according to the declaring type {3}. > > I'll add tests, which spell these out with examples in my next patch. That all sounds good, except grammatically it needs to be 'incompatible with'. Or if you want to be clearer, I think 'not convertible to' would convey the right meaning. > I was also playing with different orders of parts of the message, but I > think the current order succeeds in putting the most important parts first. Agreed - that was what motivated my suggestion for 1201. > The "or" part is too much, IMHO. I'm not even sure if we want to *recommend* > that workaround using a cast. Fair point - leave it out. @SW is probably better than casting in such cases. :)
Playing further with the 'not convertible' thing, we could consider saying 'not convertible' for one and 'not convertible even with a cast' for the other? So you'd have 1200 = Discouraged invocation of method {0}. Argument type {1} is not convertible to the likely type {2} according to the declaring type {3}. 1201 = Discouraged invocation of method {0}. Argument type {1} is not convertible to the likely type {2} according to the declaring type {3}, even with a cast. Nah, don't like it. Stick with what you've got. (Posting anyway just in case you do want to explore this.)
New Gerrit change created: https://git.eclipse.org/r/43735
Shifted from linguistics back to engineering :) (In reply to Eclipse Genie from comment #17) > New Gerrit change created: https://git.eclipse.org/r/43735 @Markus, do you want to comment on added API (constants in JavaCore and IProblem - plus new @SW token) and message wording? I also Cc'd you on bug 461999, marked that one as M7, though.
For better accessibility, here's the list of methods recognized by the change: Collection.contains(Object) Collection.remove(Object) Collection.containsAll(Collection<?>) Collection.removeAll(Collection<?>) Collection.retainAll(Collection<?>) List.indexOf(Object) List.lastIndexOf(Object) Map.containsKey(Object) Map.get(Object) Map.containsValue(Object) Methods are also recognized via any sub-type of the above.
A failure in BatchCompilerTest reminded me, that hasTypeBit() may trigger super type traversal, thus making lookup more eager than strictly necessary. I reduced this effect to the minimum by first comparing selectors and only then checking the receiver type.
Gerrit change https://git.eclipse.org/r/43735 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=689526016f6ab442680e718d2760fc44e79dd9b5
Released for 4.5 M6. Let me know if I missed s.t. relevant.
New Gerrit change created: https://git.eclipse.org/r/43851
(In reply to Eclipse Genie from comment #23) > New Gerrit change created: https://git.eclipse.org/r/43851 This changes one of the options from [E/W/I] to [Enable/Disable] to better support the intended way to surface this in the options/preferences UI. We still use two IProblems, just the way they are controlled via JavaCore options is changed.
(In reply to Stephan Herrmann from comment #24) > (In reply to Eclipse Genie from comment #23) > > New Gerrit change created: https://git.eclipse.org/r/43851 > > This changes one of the options from [E/W/I] to [Enable/Disable] to better > support the intended way to surface this in the options/preferences UI. > > We still use two IProblems, just the way they are controlled via JavaCore > options is changed. Pushed directly via http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=27e2724327c75ddbe7646447a4af212947b149ed to accommodate tiny javadoc corrections.
New Gerrit change created: https://git.eclipse.org/r/43884
(In reply to Eclipse Genie from comment #26) > New Gerrit change created: https://git.eclipse.org/r/43884 No, Genie. That commit is not for this bug, it just mentions this bug's number in its title.
Gerrit change https://git.eclipse.org/r/43884 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=92a55fb8c61c817a37bdd13d873f8eba50e25d7d
(In reply to Eclipse Genie from comment #28) > Gerrit change https://git.eclipse.org/r/43884 was merged to [master]. > Commit: > http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/ > ?id=92a55fb8c61c817a37bdd13d873f8eba50e25d7d No, Genie. That commit is not for this bug, it just mentions this bug's number in its title.
We should not enable new experimental options by default and let users be the guinea pigs. The jdt.core repo contains about 20 such problems, and jdt.ui over 100. We first have scrutinize at least these invocations and be sure they are really problematic. That will also tell us if we should provide Quick Fixes for common problem patterns. Stephan, can you please disable the main option for M6?
Some analysis of reported warnings: org.eclipse.jdt.core total: 8 warnings - 2 real bugs - 6 cases pass a value of a super type of the expected type This is a (probably unconscious) shorthand for an instanceof check plus cast Let's check if just flipping the sub-option gives an even better ratio: - 2 warning are of the severe form "cannot be cast" - 6 warnings say "is incompatible with" Unfortunately, only 1 of the 2 bugs is in the "uncastable" group. The other "uncastable" problem concerns a removeAll() invocation, where we could possibly relax the warning by comparing element types instead of collection types. OTOH, we have a bug that escapes the weaker form of the warning. This happens, because an interface is involved, and hence types are considered as castable although totally unrelated. That would mean for the small sample of o.e.j.core by flipping the sub-option we could reduce the number of reports to 1, with 0 false positives but 1 false negative. It probably depends on how you look at the 6 cases that are not strictly bugs (from a cursory look most of the warnings in jdt.ui also fit into this group). I personally think those reports are still interesting, but I know that many users will not agree. ===== When I started working on this bug I didn't expect this to be controversial (otherwise I would have started earlier), but seeing the request to disable by default I will simply revert this feature for Mars and start a discussion early after Mars. This will give us time to sharpen the analysis for the best possible hit-ratio. I don't want to publish yet another warning that is off-by-default and is wasted to most users. Remember, switching the default after initial release doesn't reach any users that already persisted the preferences in their projects.
New Gerrit change created: https://git.eclipse.org/r/43973
New Gerrit change created: https://git.eclipse.org/r/43974
New Gerrit change created: https://git.eclipse.org/r/43976
(In reply to Stephan Herrmann from comment #31) > The other "uncastable" problem concerns a removeAll() invocation, where we > could possibly relax the warning by comparing element types instead of > collection types. Absolutely - it's very common for example to go Set<T>.removeAll(List<T>), e.g. if we were iterating over the set and couldn't modify it until we finished. Definitely worth refining. > OTOH, we have a bug that escapes the weaker form of the warning. This > happens, because an interface is involved, and hence types are considered as > castable although totally unrelated. Presumably because the compiler can't rule out that the class may have a subtype that implements the interface. One possible, pretty simple, refinement could be, "Accept arguments that are a supertype of the expected type." > I don't want to publish yet another warning that is off-by-default and is > wasted to most users. Remember, switching the default after initial release > doesn't reach any users that already persisted the preferences in their > projects. Agreed!
Gerrit change https://git.eclipse.org/r/43973 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.ui.git/commit/?id=cfca412f57f26eae63a2010e2327c439d2554f16
(In reply to Ben Davis from comment #35) > (In reply to Stephan Herrmann from comment #31) > > The other "uncastable" problem concerns a removeAll() invocation, where we > > could possibly relax the warning by comparing element types instead of > > collection types. > > Absolutely - it's very common for example to go Set<T>.removeAll(List<T>), > e.g. if we were iterating over the set and couldn't modify it until we > finished. Definitely worth refining. It's not about Set<T> vs. List<T> (both conform to Collection<T>), but about Collection<Super> vs. Collection<Sub>. A Collection<Super> is not castable to Collection<Sub>, but each contained element Super, is castable to Sub - which may be enough for the invocation to do s.t. useful.
A fix to add when this is resurrected: Javadoc of org.eclipse.jdt.core.JavaCore.COMPILER_PB_DISCOURAGED_INVOCATION_ACCEPT_CASTABLE_ARGUMENT had typos in "Possible values" and "Default": Said e.g. "enable" instead of "enabled".
(In reply to Stephan Herrmann from comment #31) > by default I will simply revert this feature for Mars and start a discussion > early after Mars. This will give us time to sharpen the analysis for the The check in this ticket (including the corresponding "equals"-check) is for the main kind of easily avoidable, common and still hard to find bug that is currently not reported by eclipse (but is reported by intellij, netbeans and of course by findbugs, but this is awkward and slow by comparison to a build-in check in eclipse) It's a shame this didn't make into Mars, but I guess its already past "early after Mars", so maybe you please start the discussion so it could hopefully make into the next milestone :-) Maybe the default level could simply be the new "info"-level?
(In reply to Till Brychcy from comment #39) > ...so maybe you please start the discussion so it could hopefully > make into the next milestone :-) I'm all ears. I think I've put enough information regarding my understanding of the issue into previous comments. Markus, are you still interested in this feature?
(In reply to Stephan Herrmann from comment #40) > Markus, are you still interested in this feature? Markus is on vacation for a week or so. So, it could be a while before we get a response.
(In reply to Jay Arthanareeswaran from comment #41) > (In reply to Stephan Herrmann from comment #40) > > Markus, are you still interested in this feature? > > Markus is on vacation for a week or so. So, it could be a while before we > get a response. Markus, are you back?
I'm back, but I'm afraid I won't have time for bigger feedback rounds or test/fix passes. If you think you have a solution that doesn't trigger many false positives, then I'm OK with releasing it with Info severity by default. Please review it with a code base that actually uses typed collections. In jdt.core, many collections are still raw or the code uses arrays or internal data structures. The platform.text repo could be a good test case. jdt.ui still has unavoidableGenericTypeProblems=disabled and uses raw types from jdt.core and jface APIs.
(In reply to Markus Keller from comment #43) > I'm back, but I'm afraid I won't have time for bigger feedback rounds or > test/fix passes. If you think you have a solution that doesn't trigger many > false positives, then I'm OK with releasing it with Info severity by default. > > Please review it with a code base that actually uses typed collections. In > jdt.core, many collections are still raw or the code uses arrays or internal > data structures. The platform.text repo could be a good test case. jdt.ui > still has unavoidableGenericTypeProblems=disabled and uses raw types from > jdt.core and jface APIs. Thanks. Before I invest more time, I still need to check with you: Raising problems only in situations that are certain not to be false positives and setting severity to Info will be lame, for two reasons: (1) For a small fraction (not-castable types) we can be quite sure of detecting real bugs. Here anything below Warning severity would be wrong IMHO. (Programs could still work due to equals/hashCode hacks, but we have no chance to sift these cases out anyway). (2) For the majority of situations we indeed cannot clearly distinguish "false positives" from useful reports. I'm willing to continue work if we agree that - not-castable types are reported as warnings - at info level it's OK to raise "false positives" Within the latter group, some fine tuning may still be possible, but each match in this group could be an intentional abbreviation for if (k instanceof ElementType) set.remove((ElementType) k); If from this we would conclude, that "set.remove(k)" must not be flagged for castable types, then I don't think the whole thing would be worth the effort and we should close this as WONTFIX (which would only be the last resort if we can't agree better).
I'm very sorry to say: considering the expectations expressed in comment 43 I am not able to provide an implementation of a very useful analysis. This is a great pity after the work that has already gone into it and given that such a validation should be expected from any Java IDE.
What?! Resolved as WONTFIX? Design by committee at its finest. Suuuuurely to goodness, an off-by-default warning is better than just giving up entirely?
Verified for 4.6 M6
New Gerrit change created: https://git.eclipse.org/r/91577
(In reply to Eclipse Genie from comment #48) > New Gerrit change created: https://git.eclipse.org/r/91577 @Stephan, as discussed at EclipseCon Ludwigsburg last year, I think this is badly missing in Eclipse and I have rebased your (great!) patch which already solved all the hard parts and worked on it to find "a solution that doesn't trigger many false positives". (In reply to Stephan Herrmann from comment #31) > > The other "uncastable" problem concerns a removeAll() invocation, where we > could possibly relax the warning by comparing element types instead of > collection types. See 1) > > OTOH, we have a bug that escapes the weaker form of the warning. This > happens, because an interface is involved, and hence types are considered as > castable although totally unrelated. See 2) Patch set 1 just contains the rebased patch with some conflicts resolved (bits & problem ids) I've tested the patch against a few workspaces including a workspace with nearly 500 projects that are in the git repositories checked out in a eclipse.platform.releng.aggregator-build (details see below, more than 40000 classes) and another workspace with more than 800 projects and 30000 classes. 1) As Stephan already noted, applying the type compatibility check directly on the arguments is too restrictive for methods like removeAll because collection of a type is not a supertype of a collection of subtype, so it helps to compare the type arguments in this case, which needed a change of the message reported. 2) In a lot of projects, disabling "Accept arguments that could be cast to the expected type" gives too many false problem reports, e.g. because of collections of a subtype, because of wildcards, or in classes that implement collection interfaces themselves. But as Stephan wrote, enabling "Accept arguments that could be cast to the expected type" disables the check for all invocations, where the collection type is not a final class and the argument type is an "unrelated" interface. But many usages of this kind I have observed where bugs. To improve the situation, I have played with the check for allowed types and looked at the actual bugs found . Many bugs that were found fall into the following categories: - confusion of a type and a "primary key" member of it. This kind of bug is sometimes not reported, if all castable classes are accepted. - confusion of key and value types of maps (or confusion of respective methods). This also happens in generic code where both types are type variables. - confusion of arrays and their elements So I have arrived at the following check, which leads to good results: "Take the erasure of both types (unless both are true type variables, i.e. not captures) and then compare if they are compatible in either direction." This is a bit similar, but different to "Accept arguments that could be cast to the expected type", so I have removed that option for now (we can still introduce a kind of "strict"-mode later) With this new condition and the change for removeAll all actual bugs if have seen are still reported and only very few false alarms appear. Some other things I have added: - Implement the check for method references. This, and the change for removeAll etc. made it necessary to alter the wording of the problem messages (e.g. avoid the word "invocation") - Implement the check for "equals", too (see bug 415306), including java.utils.Objects.equals(a,b). With the same check as described above, many bugs are reported, but there are more false reports, as implementations of equals() that actually allow different argument types are rare, but not completely uncommon. The message for collection methods is now: "Unlikely argument type String for contains(Object) on a Collection<Short>" "Unlikely argument type Set<String> for removeAll(Collection<?>) on a Collection<Number> For equals, not the declaring type of the equals-method is reported but the instance/other argument type: "Unlikely argument type for equals(): Integer seems to be unrelated to String" All of this is contained in patch set 2. The next section contains a detailed comparison of patch set 1 (with "Accept arguments that could be cast to the expected type" enabled) with patch set 2 in a workspace with most projects contained in the git repos checked out in a eclipse.platform.releng.aggregator-build (as of 12. Feb 2017, excluding platform-specific code, embedded test projects and some emf-related code, that need dependencies not contains in those git-repos) - only for the collection methods, not for equals(). I've grouped the reported problems into three groups. A: Locations that are reported by patch set 2, but not by patch set 1. B: Locations reported in patch set1, but are not in patch set 2. They are variations of removeAll. None of them seemed to be a bug for me. C: Locations that are reported both by patch set 1 and patch set 2. -----------Group A: New in patch set 2: 9 additional bugs found, 6 false alarms added-------------------- BUGS: (confusion of key and values or the respective methods): ConsumerReportConvertor.java /org.eclipse.pde.api.tools/src/org/eclipse/pde/api/tools/internal/search line 201 Java Problem EventListeners.java /org.eclipse.osgi/supplement/src/org/eclipse/osgi/framework/eventmgr line 71 Java Problem InternalSearchUI.java /org.eclipse.search/new search/org/eclipse/search2/internal/ui line 117 Java Problem ManyToMany.java /org.eclipse.jdt.compiler.apt/src/org/eclipse/jdt/internal/compiler/apt/util line 314 Java Problem ManyToMany.java /org.eclipse.jdt.apt.core/src/org/eclipse/jdt/apt/core/internal/util line 315 Java Problem RecentSettingsStore.java /org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/javadocexport line 96 Java Problem BUGS: (other) DragHost.java /org.eclipse.e4.ui.workbench.addons.swt/src/org/eclipse/e4/ui/workbench/addons/dndaddon line 138 Java Problem - should remove from getWindows() not from getChildren() ResourceToItemsMapper.java /org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/viewsupport line 125 Java Problem - should remove resource, not list ResourceToItemsMapper.java /org.eclipse.search/search/org/eclipse/search/internal/ui line 138 Java Problem - should remove resource, not list NOT BUGS: ChangeCollector.java /org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/hierarchy line 371 Java Problem - CompilationUnit extends Openable which extends JavaElement and implements IOpenable FragmentExtractHelper.java /org.eclipse.e4.tools/src/org/eclipse/e4/internal/tools/wizards/model line 53 Java Problem ModelAssembler.java /org.eclipse.e4.ui.workbench/src/org/eclipse/e4/ui/internal/workbench line 407 Java Problem - e.g. ApplicationElementImpl implements EObject and MApplicationElement RuntimeClasspathViewer.java /org.eclipse.jdt.debug.ui/ui/org/eclipse/jdt/internal/debug/ui/classpath line 144 Java Problem - ClasspathEntry extends AbstractClasspathEntry which implements IClasspathEntry and implements IRuntimeClasspathEntry SourceReferenceUtil.java /org.eclipse.jdt.ui/core refactoring/org/eclipse/jdt/internal/corext/refactoring/reorg line 54 Java Problem - e.g. Member extends SourceRefElement implements IMember TypeHierarchy.java /org.eclipse.jdt.core/model/org/eclipse/jdt/internal/core/hierarchy line 1045 Java Problem - already guarded by instanceof ClassFile which extends Openable --------- Group B: Reported by patch set 1, not any more in patch set 2---------------- NOT BUGS: removeAll etc for collections of subtypes/supertype AbstractWorkingSetWizardPage.java /org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/workingsets line 482 Java Problem IdentityMapTest.java /org.eclipse.jface.tests.databinding/src/org/eclipse/core/tests/internal/databinding line 342 Java Problem IdentityMapTest.java /org.eclipse.jface.tests.databinding/src/org/eclipse/core/tests/internal/databinding line 346 Java Problem IdentityMapTest.java /org.eclipse.jface.tests.databinding/src/org/eclipse/core/tests/internal/databinding line 383 Java Problem IdentityMapTest.java /org.eclipse.jface.tests.databinding/src/org/eclipse/core/tests/internal/databinding line 393 Java Problem IndexerTest.java /org.eclipse.jdt.core.tests.model/src/org/eclipse/jdt/core/tests/nd/indexer line 255 Java Problem JarManifestWizardPage.java /org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/jarpackager line 638 Java Problem JarManifestWizardPage.java /org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/jarpackager line 643 Java Problem LaunchDelegateTests.java /org.eclipse.jdt.debug.tests/tests/org/eclipse/jdt/debug/tests/launching line 178 Java Problem LaunchModeTests.java /org.eclipse.jdt.debug.tests/tests/org/eclipse/jdt/debug/tests/launching line 191 Java Problem LaunchModeTests.java /org.eclipse.jdt.debug.tests/tests/org/eclipse/jdt/debug/tests/launching line 204 Java Problem MenuManagerRenderer.java /org.eclipse.e4.ui.workbench.renderers.swt/src/org/eclipse/e4/ui/workbench/renderers/swt line 1084 Java Problem MenuManagerRenderer.java /org.eclipse.e4.ui.workbench.renderers.swt/src/org/eclipse/e4/ui/workbench/renderers/swt line 1091 Java Problem MenuManagerRenderer.java /org.eclipse.e4.ui.workbench.renderers.swt/src/org/eclipse/e4/ui/workbench/renderers/swt line 1098 Java Problem ModuleResolver.java /org.eclipse.osgi/container/src/org/eclipse/osgi/container line 1140 Java Problem ModuleResolver.java /org.eclipse.osgi/container/src/org/eclipse/osgi/container line 944 Java Problem ModuleResolver.java /org.eclipse.osgi/container/src/org/eclipse/osgi/container line 945 Java Problem PullUpRefactoringProcessor.java /org.eclipse.jdt.ui/core refactoring/org/eclipse/jdt/internal/corext/refactoring/structure line 947 Java Problem RemoveFromBuildpathAction.java /org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/wizards/buildpaths/newsourcepage line 161 Java Problem RemovedImportCommentReassigner.java /org.eclipse.jdt.core/dom/org/eclipse/jdt/internal/core/dom/rewrite/imports line 150 Java Problem ReorgPolicyFactory.java /org.eclipse.jdt.ui/core refactoring/org/eclipse/jdt/internal/corext/refactoring/reorg line 2233 Java Problem TargetLocationsGroup.java /org.eclipse.pde.ui/src/org/eclipse/pde/internal/ui/shared/target line 381 Java Problem ToolBarManagerRenderer.java /org.eclipse.e4.ui.workbench.renderers.swt/src/org/eclipse/e4/ui/workbench/renderers/swt line 966 Java Problem TypesConfigurationArea.java /org.eclipse.ui.ide/src/org/eclipse/ui/internal/views/markers line 426 Java Problem WorkingSetDropAdapter.java /org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/packageview line 260 Java Problem --- Group C: Reported by both patch set 1 and patch set 2: 17 Bugs, 2 Non-Bugs-------------------- BUGS: (confusion of key and values or the respective methods): BaseMessageRegistry.java /org.eclipse.e4.core.services/src/org/eclipse/e4/core/services/nls line 307 Java Problem LogicalPackagesProvider.java /org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/browsing line 129 Java Problem ProvisioningContext.java /org.eclipse.equinox.p2.engine/src/org/eclipse/equinox/p2/engine line 168 Java Problem SourceLookupFacilityTests.java /org.eclipse.debug.tests/src/org/eclipse/debug/tests/sourcelookup line 313 Java Problem Test_org_eclipse_swt_program_Program.java /org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit line 160 Java Problem BUGS: other AbstractPDELaunchConfiguration.java /org.eclipse.pde.launching/src/org/eclipse/pde/launching line 203 Java Problem - string used as key BaseMessageRegistry.java /org.eclipse.e4.core.services/src/org/eclipse/e4/core/services/nls line 231 Java Problem - "this" is the PrivilegedAction, not the MessageConsumer NdNodeTypeRegistry.java /org.eclipse.jdt.core/search/org/eclipse/jdt/internal/core/nd line 39 Java Problem - should be this.types.containsKey(shortTypeId) ProductFile.java /org.eclipse.equinox.p2.publisher.eclipse/src/org/eclipse/equinox/internal/p2/publisher/eclipse line 437 Java Problem - string used as argument RequiredPluginsClasspathContainer.java /org.eclipse.pde.core/src/org/eclipse/pde/internal/core line 380 Java Problem - string used as argument TestReporter.java /org.eclipse.pde.api.tools.tests/src/org/eclipse/pde/api/tools/search/tests line 59 Java Problem - argument is not a string. maybe String.valueof should be applied to it? BUGS: confusion of array with array element BoundSet.java /org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/lookup line 1259 Java Problem - should be get(iv) instead of get(outerVariables) CallHistory.java /org.eclipse.ui.tests.harness/src/org/eclipse/ui/tests/harness/util line 129 Java Problem - should be methodName BUGS: confusion of type with member of it: LaunchConfigurationManager.java /org.eclipse.debug.ui/ui/org/eclipse/debug/internal/ui/launchConfigurations line 751 Java Problem - should be types.contains(type.getIdentifier()) NewJavaProjectWizardPageTwo.java /org.eclipse.jdt.ui/ui/org/eclipse/jdt/ui/wizards line 337 Java Problem - should be fOrginalFolders.contains(child) Product.java /org.eclipse.pde.core/src/org/eclipse/pde/internal/core/product line 647 Java Problem - should be fFeatures.contains(features[i]) VariableCreationDialog.java /org.eclipse.jdt.ui/ui/org/eclipse/jdt/internal/ui/wizards/buildpaths line 98 Java Problem - should be remove(element) NOT BUGS: (but actually interesting warnings) ApiFiltersPropertyPage.java /org.eclipse.pde.api.tools.ui/src/org/eclipse/pde/api/tools/ui/internal/properties line 340 Java Problem - not a bug, but surprising: CommentChange delegates equals to its filter member - an example for the kind of code that leads to false alarms for the equals()-check. SetBindingTest.java /org.eclipse.jface.tests.databinding/src/org/eclipse/core/tests/databinding line 80 Java Problem - it seems like the remove call intentionally tries to remove an object that cannot be contained in the test. -------------------- Summary: Patch set 2 finds 26 bugs and gives 8 false alarms. The results in my other workspaces were similar, with even fewer false alarms per bug found. For equals(), I haven't done a detailed analysis, but the absolute number of bugs found was lower, and the ratio of false alarms was a bit higher. I have set the default level for both to "INFO" (though i will set them to "WARNING" in my workspaces) The test failures in BatchCompilerTest.test330_warn_options() and BatchCompilerTest.test331_warn_options() are because there is currently no way to disable "INFO"s for the batch compiler. I guess this should be discussed in bug Bug 496137.
Thanks, Till for your huge efforts. With all the real bugs the analysis detects, this may well become a huge success story. With your improvements and the facts to back our claims, it would be a shame to release this with severity below WARNING. Remember we have a @SW token to silence any "unwanted" warnings. @Markus, do you care to comment?
Due to API freeze at M6 we need to make a call now. (In reply to Till Brychcy from comment #49) > So I have arrived at the following check, which leads to good results: > "Take the erasure of both types (unless both are true type variables, i.e. > not captures) and then compare if they are compatible in either direction." > > This is a bit similar, but different to "Accept arguments that could be cast > to the expected type", so I have removed that option for now (we can still > introduce a kind of "strict"-mode later) > > With this new condition and the change for removeAll all actual bugs if have > seen are still reported and only very few false alarms appear. I believe this to be good, just I don't understand *why* this exact way of comparison should be the winner. Is it purely heuristic or do you have a theory to explain why it makes sense? I agree to deferring a potential sub-option to later. > I have set the default level for both to "INFO" (though i will set them to > "WARNING" in my workspaces) I still believe much of the effect will be lost if defaulting to info. Consider a big workspace with lots of existing code. In projects less disciplined than JDT, the number of warnings in the workspace will be overwhelming already. Nobody will ever look past that and into the info section. What I typically do, if the problems view shows too much: switch to "Errors/Warnings on Project". Please see that this option does not show any infos at all. Users could configure their own options for the problems view, but I predict they won't. So in that context infos are only ever visible when you open the editor. An analysis that finds existing bugs at the extent demonstrated here, should be more visible than that. Many of those "unwanted" warnings are still interesting to document via @SW. Last year it was Markus, who requested defaulting to "info" at most. We haven't heard from him since. Let's make a decision tomorrow, if possible.
(In reply to Stephan Herrmann from comment #51) > I believe this to be good, just I don't understand *why* this exact way of > comparison should be the winner. Is it purely heuristic or do you have a > theory to explain why it makes sense? "Purely heuristic" is a good description. By taking the erasures, problems with captures are avoided. Not doing it for "true" type variables lets us still find bugs in generic code. Allowing compatibility in either direction is in the same sprit as allowing castable types, but narrower. I've tried other variations (e.g. just uncapturing captures or by creating a variation of the "castable"-check that doesn't assume that there might be subclasses that implement another interface) but they were more complicated, harder to explain, didn't find any additional bugs and gave more false positives. > I still believe much of the effect will be lost if defaulting to info. > > [...] > > Last year it was Markus, who requested defaulting to "info" at most. We > haven't heard from him since. Let's make a decision tomorrow, if possible. My opinion: +1 for "WARNING" as default, at least for "collection methods" I'm also leaning towards "WARNING" as default for "equals", even though there are a bit more false alarms. Anyway, having these checks with default "INFO" is way better than not having them at all ;-)
(In reply to Till Brychcy from comment #52) > (In reply to Stephan Herrmann from comment #51) > > I believe this to be good, just I don't understand *why* this exact way of > > comparison should be the winner. Is it purely heuristic or do you have a > > theory to explain why it makes sense? > > "Purely heuristic" is a good description. > > By taking the erasures, problems with captures are avoided. Not doing it for > "true" type variables lets us still find bugs in generic code. Allowing > compatibility in either direction is in the same sprit as allowing castable > types, but narrower. > > I've tried other variations (e.g. just uncapturing captures or by creating a > variation of the "castable"-check that doesn't assume that there might be > subclasses that implement another interface) but they were more complicated, > harder to explain, didn't find any additional bugs and gave more false > positives. Sounds fair. A heuristic approach fits the goal to find "unlikely" arguments :) OTOH, doesn't the use of such heuristics actually motivate having a strict variant as an option (off by default)? If people don't trust your heuristics they could still analyse their code in strict mode (maybe only temporarily switching modes). Maybe people are more willing to perform the experiment of using strict mode while the feature is still new. This _could_ give valuable hints about cases that we are missing due to the heuristics. How thoroughly have you removed this option? Can it easily be brought back to life? As to possible wordings of such option, a precise description is overkill, so we could paraphrase it either way as [x] Apply heuristic filters or [ ] Perform strict analysis against the expected type Or similar. Do you still want to postpone this decision?
(In reply to Stephan Herrmann from comment #53) > Sounds fair. A heuristic approach fits the goal to find "unlikely" arguments > :) > > OTOH, doesn't the use of such heuristics actually motivate having a strict > variant as an option (off by default)? If people don't trust your heuristics > they could still analyse their code in strict mode (maybe only temporarily > switching modes). > > Maybe people are more willing to perform the experiment of using strict mode > while the feature is still new. This _could_ give valuable hints about cases > that we are missing due to the heuristics. > > How thoroughly have you removed this option? Can it easily be brought back > to life? [...] Shouldn't be too hard, but i fear still too much work before the api freeze. TODO would be: Add constants for the option, add ui, update all tests, invent an optimized error message. I don't think this option would to be available for the batch compiler. > [...] > [ ] Perform strict analysis against the expected type When we do that: I like the second option better. > Or similar. > > Do you still want to postpone this decision? I'm not against having it, but I think the feature is already extremely useful without it (and even with a default level of INFO), so we can add this later.
(In reply to Stephan Herrmann from comment #53) > Do you still want to postpone this decision? Anyway I'll try if I can add the option within the next two hours!
(In reply to Till Brychcy from comment #55) > (In reply to Stephan Herrmann from comment #53) > > Do you still want to postpone this decision? > > Anyway I'll try if I can add the option within the next two hours! cool, promise to quit when it takes longer? :)
(In reply to Stephan Herrmann from comment #56) > (In reply to Till Brychcy from comment #55) > > (In reply to Stephan Herrmann from comment #53) > > > Do you still want to postpone this decision? > > > > Anyway I'll try if I can add the option within the next two hours! > > cool, promise to quit when it takes longer? :) jdt.core part done, maybe you can already have a look (especially at javadoc and naming) while I'm doing the jdt.ui part?
(In reply to Till Brychcy from comment #57) > jdt.core part done, maybe you can already have a look (especially at javadoc > and naming) while I'm doing the jdt.ui part? Note: I have NOT created a different message for the case when this option is on, I think the current one is ok.
jdt.ui part done :-)
(In reply to Till Brychcy from comment #57) > (In reply to Stephan Herrmann from comment #56) > > (In reply to Till Brychcy from comment #55) > > > (In reply to Stephan Herrmann from comment #53) > > > > Do you still want to postpone this decision? > > > > > > Anyway I'll try if I can add the option within the next two hours! > > > > cool, promise to quit when it takes longer? :) > > jdt.core part done, maybe you can already have a look (especially at javadoc > and naming) while I'm doing the jdt.ui part? I've inspected the core part once more, change one option to warning by default (collection w/ heuristics) and made minor modifications here and there. Will upload in a minute.
Ah, and the test failure in #6 is also fixed in #7 :) You may want to triple check updated documentation in JavaCore. It seems we can await even one more jenkins build. My previous statement (in some bug) about the build schedule confused the timezones, next I-build should be at 8pm EST.
(In reply to Markus Keller from comment #38) > A fix to add when this is resurrected: > > Javadoc of > org.eclipse.jdt.core.JavaCore.COMPILER_PB_DISCOURAGED_INVOCATION_ACCEPT_CASTABLE_ARGUMENT > had typos in "Possible values" and "Default": > Said e.g. "enable" instead of "enabled". This same mistake has crept into the new version of that option. I have it fixed locally, but don't want to interrupt / retrigger the jenkins job. Since it's a comment-only change, I'll probably push directly.
(In reply to Stephan Herrmann from comment #61) > Ah, and the test failure in #6 is also fixed in #7 :) :-) > > You may want to triple check updated documentation in JavaCore. I've looked at all your changes, very good! > > It seems we can await even one more jenkins build. My previous statement (in > some bug) about the build schedule confused the timezones, next I-build > should be at 8pm EST. Good. Let's hope there are no random unrelated failures this time :-/ Could you please do the committing, It's getting too late for me...
(In reply to Till Brychcy from comment #63) > Good. Let's hope there are no random unrelated failures this time :-/ If there are random unrelated failures I'll just ignore them :) (will push directly anyway) > Could you please do the committing, It's getting too late for me... This tonight, and I'll trigger the two UI changes tomorrow morning.
Gerrit change https://git.eclipse.org/r/91577 was merged to [master]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=e7d6d8a91612da3f3cae0b22ddd55df43d4ee554
Failure in APIDocumentationTests signalled that our javadoc had unexpected linebreaks. Fixed. I ran the tests that were skipped on jenkins locally => green. (In reply to Eclipse Genie from comment #65) > Gerrit change https://git.eclipse.org/r/91577 was merged to [master]. > Commit: > http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=e7d6d8a91612da3f3cae0b22ddd55df43d4ee554 > Released for 4.7 M6
*** Bug 415306 has been marked as a duplicate of this bug. ***
@Stephan, do you have time to make a N&N entry for this? If not, I can at least make a draft. Obviously you're way better at writing than I am...
(In reply to Till Brychcy from comment #68) > @Stephan, do you have time to make a N&N entry for this? If not, I can at > least make a draft. Obviously you're way better at writing than I am... After the deadline is before the deadline, right? :p So, this is due tomorrow evening, right? As time is short, maybe we can share the load: if you could provide a nice screenshot by tomorrow early evening, I'll write the text. Deal?
(In reply to Stephan Herrmann from comment #69) > (In reply to Till Brychcy from comment #68) > > @Stephan, do you have time to make a N&N entry for this? If not, I can at > > least make a draft. Obviously you're way better at writing than I am... > > After the deadline is before the deadline, right? :p :-) > So, this is due tomorrow evening, right? > > As time is short, maybe we can share the load: if you could provide a nice > screenshot by tomorrow early evening, I'll write the text. Deal? Deal!
New Gerrit change created: https://git.eclipse.org/r/92572
(In reply to Eclipse Genie from comment #71) > New Gerrit change created: https://git.eclipse.org/r/92572 I had to make multiple screenshots so I have also added a draft text to glue them together. Maybe it is useful, otherwise I won't feel offended if you scrap this and write it completely differently.
Verified for 4.7 M6 with build I20170307-2000. Btw, thanks Stephan and Till for taking this through.
For record, I must clarify that I haven't verified the N&N.
(In reply to Jay Arthanareeswaran from comment #74) > For record, I must clarify that I haven't verified the N&N. Sorry for being late, I'm reviewing / editing the proposed N&N entry only now.
Gerrit change https://git.eclipse.org/r/92572 was merged to [master]. Commit: http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=f101ee5ca04ae5be6a429768e1e1a5efab142d4c
(In reply to Eclipse Genie from comment #76) > Gerrit change https://git.eclipse.org/r/92572 was merged to [master]. > Commit: > http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=f101ee5ca04ae5be6a429768e1e1a5efab142d4c > You did ask for an essay, right? :) https://www.eclipse.org/eclipse/news/4.7/M6/#unlikely-argument-types
(In reply to Stephan Herrmann from comment #77) > (In reply to Eclipse Genie from comment #76) > > Gerrit change https://git.eclipse.org/r/92572 was merged to [master]. > > Commit: > > http://git.eclipse.org/c/www.eclipse.org/eclipse/news.git/commit/?id=f101ee5ca04ae5be6a429768e1e1a5efab142d4c > > > > You did ask for an essay, right? :) > > https://www.eclipse.org/eclipse/news/4.7/M6/#unlikely-argument-types Excellent!
I would welcome a second opinion on this 'unlikely argument' warning in the following line regarding a map 'get': https://git.eclipse.org/c/platform/eclipse.platform.ui.git/tree/bundles/org.eclipse.e4.ui.workbench/src/org/eclipse/e4/ui/internal/workbench/ModelAssembler.java#n407 Variable 'importObject' is declared as an EObject (interface) in line 403. Variable 'importMaps' is declared as an Map<MApplicationElement,MApplicationElement> in line 384 just above (MAE is also an interface). Now it's guaranteed that whatever is an MApplicationElement is also an EObject (all these guys are EMF classes implementing EObject). In fact if you try to cast 'importObject' to MApplicationElement it will tell you it is unnecessary. Under these conditions my opinion is to just suppress this warning. Any alternative insight?
The warning is intended for exactly this scenario, since you wouldn't have been able to add the 'EObject' to the map without casting it, and thus, the compiler doesn't know if you are accidentally passing the wrong argument to the 'get' method. The warning about the cast being unnecessary is happening because the 'get' method is declared to take 'Object' (and this is determined without consideration of the new warning). Maybe it would make sense to suppress the 'unnecessary cast' warning instead in these scenarios? Thanks by the way to the people who finally implemented this warning - I was one of the people originally begging for it :)
(In reply to Sopot Cela from comment #79) > Now it's guaranteed that whatever is an MApplicationElement is also an EObject (all these guys are EMF classes implementing EObject). The fact that all MApplicationElements are also EObjects is developer knowledge, that's not available to the compiler. Fair enough. However, what you'd need is a guarantee that the EObject 'importObject' definitely is an MApplicationElement. Are you sure about that direction, too? Otherwise, 'importObject' may not be a "legal" key to begin with. In this case you'd need an additional "if (importObject instanceof MApplicationElement)", as you don't want the newly added cast to blow up at runtime... Only if you are really worried about one additional pair of instanceof & cast, then adding @SuppressWarnings("unlikely-arg-type") with a comment about your reasoning would be your escape hatchet. (In reply to Ben Davis from comment #80) > The warning about the cast being unnecessary is happening because the 'get' > method is declared to take 'Object' (and this is determined without > consideration of the new warning). Maybe it would make sense to suppress the > 'unnecessary cast' warning instead in these scenarios? => bug 514956
Thanks for the great work here all, this helped us catch a few subtle bugs now! In Photon M5 I see a "test sources" feature has suddenly popped up. (Yay!) Making use of that could filter-out the remaining (60 or so) false-positives that we see from having this check enabled, as these all arise in test code. I've logged bug 533534 for this enhancement idea.
*** Bug 451645 has been marked as a duplicate of this bug. ***
