Bugzilla – Full Text Bug Listing
| Summary: | [compiler][null] Consider stronger NPE warnings for non-volatile fields | ||
|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Ayushman Jain <amj87.iitr> |
| Component: | Core | Assignee: | Ayushman Jain <amj87.iitr> |
| Status: | VERIFIED INVALID | QA Contact: | |
| Severity: | enhancement | ||
| Priority: | P3 | CC: | amj87.iitr, deepakazad, srikanth_sankaran, stephan.herrmann |
| Version: | 3.8 | ||
| Target Milestone: | 3.8 M6 | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | |||
|
Description
Ayushman Jain
I introduced this example in bug 247564 comment 5 as an example of (temporal) distance. The notion of distance was used to describe situations where we should give no warning at all, because information has "aged". The only reasonable "stronger" that I can think of is: "val is probably null", rather then "potentially". I'm not sure whether this is actually the intention behinds this bug? (In reply to comment #1) > I introduced this example in bug 247564 comment 5 as an example of (temporal) > distance. The notion of distance was used to describe situations where we > should give no warning at all, because information has "aged". > > The only reasonable "stronger" that I can think of is: "val is probably null", > rather then "potentially". I'm not sure whether this is actually the intention > behinds this bug? See https://bugs.eclipse.org/bugs/show_bug.cgi?id=247564#c2 for the rationale for the bug along with an alternate stronger wording. (In reply to comment #1) > The only reasonable "stronger" that I can think of is: "val is probably null", > rather then "potentially". I'm not sure whether this is actually the intention > behinds this bug? We still need to decide upon the measure of a 'distance' here, or do we just assume that the info is valid unless an assingment or method call occurs? We can also consider special treatment withing synch'd blocks which was rejected after https://bugs.eclipse.org/bugs/show_bug.cgi?id=247564#c12. When synching is done on 'this', all fields can behave like locals, and when done on the current type, static fields can behave like locals. (In reply to comment #3) > (In reply to comment #1) > > The only reasonable "stronger" that I can think of is: "val is probably null", > > rather then "potentially". I'm not sure whether this is actually the intention > > behinds this bug? > > We still need to decide upon the measure of a 'distance' here Indeed, I originally used the example from comment 0 as illustration for "big distance - should not give strong warning". Now it's an example for the opposite? > We can also consider special treatment withing synch'd blocks which was > rejected after https://bugs.eclipse.org/bugs/show_bug.cgi?id=247564#c12. When > synching is done on 'this', all fields can behave like locals, and when done on > the current type, static fields can behave like locals. ... and when all other access to the same field is also sync'ed on 'this'? (In reply to comment #4) > Indeed, I originally used the example from comment 0 as illustration for "big > distance - should not give strong warning". Now it's an example for the > opposite? Don't know what you had in mind when you cooked up the example, but IMO you were exemplifying the opposite case :) Looking at https://bugs.eclipse.org/bugs/show_bug.cgi?id=247564#c2, in particular the part about ... "especially since the field is not volatile and there's nothing else that can provide proper synchronization between the null check and the access to the field." ... the example looks like a qualifying case for a stronger warning. (In reply to comment #5) OK, then I could use some help on what you mean here. > ... "especially since the field is not volatile not volatile -> we don't know if per-thread caching is involved > and there's nothing else that can provide proper synchronization between > the null check and the access to the field." ... access is not synchronized -> we don't know what other threads are doing ergo: we have no idea what value the field will have after the loop. I don't see how this motivates a stronger warning. We are basically back in the legacy mode of analysis where all fields are considered as 'unknown' and the analysis just shuts up. (In reply to comment #6) > not volatile -> we don't know if per-thread caching is involved [...] > access is not synchronized -> we don't know what other threads are doing I am sorry, I didn't look at the code carefully. I thought the `for loop' there is just there to consume some cycles to introduce the temporal distance as you put it. I am not sure if the fact that this code triggers a java.lang.ArithmeticException is germane to your view point or not. My point simply was that assuming control reaches the second field reference, an aggressively optimizing jit compiler could very well choose NOT to reload from main memory any modified value and the program would fault with an NPE. > I don't see how this motivates a stronger warning. Again since I don't know if the exception is germane to your point, assuming it is not and the for loop was just a blob of code there that neither involves method calls, or assignments to the field concerned or any other synchronization mechanisms, why doesn't a stronger warning make sense here ? Given thread scheduling at the level of granularity we are talking about is completely out of the programmer's control, I can't see how any body could reasonably expect to write programs this way and not get an NPE "much more than occasionally." If warnings in such situation get categorized as potential ones, they may not be seen by a user who hasn't checked that box. No the ArithmeticException was not intended as part of my message. Let me step back some, to try and find the root of our disagreement. Here's an implicit assumption of mine: for all variables we have max 16 different nullness states as listed in a comment in UnconditionalFlowInfo. Operations for updating and merging null info basically do the same for all variables (locals & fields). I strongly assume so, because I wouldn't dare touch that part of the implementation: the formulae used for the bit-operations in that class are completely undocumented and I see now way to reverse engineer these to anything comprehensible. I.e., changing this underlying model with its basic operations would amount to a complete re-implementation of one of the most complex parts we have in the compiler. From this assumption I conclude that we have two obvious options to record the effect from a check (val == null) a) mark val as def.null in the then-branch b) mark val as pot.null in the then-branch We don't and won't have the bits for "probably null". (We may want to investigate using state "protected null", haven't looked into that, yet. But also the semantics of this state a insufficiently documented for straight-forward reasoning). Based on this vocabulary we cannot express any notion of likelihood, only possibility and impossibility. In the example it would be wrong to say "it is impossible that val will be nonnull at this location", regardless how unlikely a nonnull value. So I don't see how we can say anything but "it is possible that val is null at this location". We could special-case the classic example if (val == null) val.op(); assuming that we can leverage the syntactic proximity and give a specially phrased message here, but beyond that I simply see no means to distinguish potential from probable. Please see my example as saying: it is just too easy to construct a context in which the expected NPE will *not* occur. (In reply to comment #8) > Based on this vocabulary we cannot express any notion of likelihood, only > possibility and impossibility. In the example it would be wrong to say "it is > impossible that val will be nonnull at this location", regardless how unlikely > a nonnull value. So I don't see how we can say anything but "it is possible > that val is null at this location". While that's true, I was under the impression that we don't exactly intend to put a stronger warning saying "NPE:field.. is always null", rather a more cautious warning suggested by Markus in https://bugs.eclipse.org/bugs/show_bug.cgi?id=247564#c2. That warning can be given for all non-constant fields instead of giving the current pot. NPE. So, the whole analysis remains the same -> instead of marking fields as def unknown in markAs.. methods, we mark them null or non null (treat them like locals basically), and at time of reporting the warning, change the wording. However, I still have no idea how we decide the 'distance' when this definitive status begins to age. If we measure distance as the next method call or assignment, I'm not sure how we can change the whole bit stream for fields from being definitely something to potentially something. That boolean algebra gives me nightmares! :P Maybe we can simplify things a bit, treat fields like locals always and just change the warning wording for non-constant fields. Plus, with a method call, wipe out the info as we're doing now. (In reply to comment #9) > [...] So, > the whole analysis remains the same -> instead of marking fields as def unknown > in markAs.. methods, we mark them null or non null (treat them like locals > basically), and at time of reporting the warning, change the wording. Sounds familiar, so let me recap I said in bug 247564 comment 114: > Exactly, that's what we need to make up our mind on. If we say for fields > we want to respect the uncertainty that concurrency brings, do we record > weaker null info right from the beginning or do we just make our warnings > weaker (e.g., right in ProblemReporter). In the sequel I was searching for examples where the exact info would be needed but didn't find any. Then Ayush said in bug 247564 comment 116: >> do we record >> weaker null info right from the beginning or do we just make our warnings >> weaker (e.g., right in ProblemReporter). > If we do that then don't we have the risk of the definite null status for > fields flowing into a local variable? Don't think we want to do that. It seems that after some back and forth I was convinced and concluded in bug 247564 comment 133: > If we agree about this test we can also agree that unknown and pot.null are > correct encodings of nonnull/null checks. If we start the discussion again, I'd like to know what has changed between then and now. (In reply to comment #8) > Here's an implicit assumption of mine: for all variables we have max 16 > different nullness states as listed in a comment in UnconditionalFlowInfo. This is an implementation detail that IMHO neither bolsters nor weakens the case for the comment#0 test case being an example for or against stronger warning: So we are back to square one debating what the comment#0 test makes a case for :) > Please see my example as saying: it is just too easy to construct a context in > which the expected NPE will *not* occur. A good optimizing jit compiler would note that the assignment to otherVal inside the loop constitutes a dead store and eliminate it only to subsequently toss out the entire loop itself ? Ignoring that point for a moment and assuming that the loop introduces a long delay in which time some other thread could change the value of the field: Such a design is so broken that if it works, it does so only fortuitously. IMHO a user would not want that alert to be downgraded because the counterpoint is a possibility under the realms of mathematics. (In reply to comment #9) > (In reply to comment #8) > While that's true, I was under the impression that we don't exactly intend to > put a stronger warning saying "NPE:field.. is always null", rather a more > cautious warning suggested by Markus in > https://bugs.eclipse.org/bugs/show_bug.cgi?id=247564#c2. That warning can be > given for all non-constant fields instead of giving the current pot. NPE. So, > the whole analysis remains the same -> instead of marking fields as def unknown > in markAs.. methods, we mark them null or non null (treat them like locals > basically), and at time of reporting the warning, change the wording. This is precisely what I had in mind. The interactions with locals can be "handled suitably". (In reply to comment #10) > (In reply to comment #9) [...] > If we start the discussion again, I'd like to know what has changed between > then and now. This bug was raised in the context of my review to understand how we could make analysis for fields more effective. While I did go through the comments on the bug cited, there were too many of them and this earlier discussion must have slipped through my mind. (In reply to comment #9) > However, I still have no idea how we decide the 'distance' when this definitive > status begins to age. [...] > Maybe we can simplify things a bit, treat fields like locals always and just > change the warning wording for non-constant fields. Plus, with a method call, > wipe out the info as we're doing now. For the moment, I would lift the 3.9 rug and sweep all these under it :) (In reply to comment #11) > A good optimizing jit compiler would note that the assignment to otherVal > inside the loop constitutes a dead store and eliminate it only to > subsequently toss out the entire loop itself ? I didn't mean this loop as it is, but a time delay loop that a compiler can ascertain will not throw any exceptions or otherwise cause any side effects. (In reply to comment #11) > (In reply to comment #8) > > > Here's an implicit assumption of mine: for all variables we have max 16 > > different nullness states as listed in a comment in UnconditionalFlowInfo. > > This is an implementation detail that IMHO neither bolsters nor weakens > the case for the comment#0 test case being an example for or against > stronger warning: If we had unlimited time and resources, I would agree. But in my current reality the existing implementation restricts the *vocabulary* I can use for reasoning. Using that vocabulary I cannot discuss likelihood. That's why I took the detour and tried to explain why I think the way I do. > Ignoring that point for a moment and assuming that the loop introduces a > long delay in which time some other thread could change the value of the > field: Such a design is so broken that if it works, it does so only > fortuitously. IMHO a user would not want that alert to be downgraded > because the counterpoint is a possibility under the realms of mathematics. In my experience with JDT bugs I learned to never make any assumptions about what users wanted or not wanted :) Yes, we should probably be bold and risk a few error markers against code that actually works. But then we should openly state that we leave the grounds of reporting "just the facts that the compiler detects" towards "educating the user to write better code". > For the moment, I would lift the 3.9 rug and sweep all these under it :) We can agree on that :) (In reply to comment #13) > Yes, we should probably be bold and risk a few error markers against code that > actually works. The proposal is not to make it an error, but a stronger warning ... > > For the moment, I would lift the 3.9 rug and sweep all these under it :) > > We can agree on that :) As the wise have remarked, "Never do today what you can as well do tomorrow" :) (In reply to comment #14) > The proposal is not to make it an error, but a stronger warning ... And if I understand you correctly, this means to (a) re-phrase the message and (b) invent a new Irritant so it can be enabled by default without enabling all pot.null warnings, right? (In reply to comment #15) > (In reply to comment #14) > > The proposal is not to make it an error, but a stronger warning ... > > And if I understand you correctly, this means to (a) re-phrase the message and Certainly this part, not sure about the latter. As I saw it, this could simply be the (reworded) IProblem#NullFieldReference (and the variants) The idea that we can start with stronger assertions for non-volatiles
and downgrade them "by only a notch" may not be as outlandish as it may
seem at first. It could produce potential warnings that a human reader
would readily dismiss, but JLS itself results in *errors* that a human
reader would dismiss as bogus:
E.g:
public class X {
public static void main(String[] args) {
X x1, x2 = null;
if (x2 == null) {
x1 = new X();
}
x1.toString(); // per JLS x1 is uninitialized.
}
}
We need to at least give this some thought.
As the implementation from https://bugs.eclipse.org/bugs/show_bug.cgi?id=247564 has been withdrawn and that bug has been reopened, the current issue has lost its relevance and can be closed as INVALID. Verified for 3.8 M6. |