Community
Participate
Working Groups
I20101026-2000 a is marked as unused with this snippet [BAD] package snippet; public class Snippet { public void method() { int a=10; if(a++ > 10) { } } } but not with [Good] package snippet; public class Snippet { public void method() { int a=10; if(a > 10) { } } }
Works correctly in these cases int a=10; for(;a++ > 10;) { } while (a++>10) { } switch(a++) { }
Try this as well - b is flagged [BAD], a is not [GOOD] package snippet; public class Snippet { private int b = 100; public void method(int a) { if(a++>10) { } if(b++>10) { } } }
It also works here: package snippet; public class Snippet { public void method() { int a=10; if(a++ > 10) { System.out.println(); } } }
For: package snippet; public class Snippet { public void method() { int a=10; if(a++ > 10) { } } } a is reported as unused because the if block is empty. So there is no need to generate anything.
It should however be consistent with the second case. I am investigating. Stephan, do you have some time to take a look ?
All these issues are related to empty blocks. In some cases, the condition needs to be generated with valueRequired and in some other cases it can be optimized out. This explains the different diagnostics. I don't consider this as major as it works fine when the code is more "real". The severity should be reduced to normal.
(In reply to comment #5) > It should however be consistent with the second case. > I am investigating. > > Stephan, do you have some time to take a look ? I'll go for it on Friday, OK?
(In reply to comment #7) > I'll go for it on Friday, OK? Sure. You will look at it for M4 anyway. As it stands right now, this is due to the empty blocks. With real code (no empty blocks), it seems to work as expected.
Deepak, do you still consider this as a bug regarding my last comments ?
(In reply to comment #9) > Deepak, do you still consider this as a bug regarding my last comments ? Yes, I do. You will see different behavior in different scenarios as you type, and have not yet filled in the if block. But I agree that it is not a major bug.
(In reply to comment #9) > Deepak, do you still consider this as a bug regarding my last comments ? You already said it in comment 5: >It should however be consistent with the second case.
What we can try to fix is that the two cases: package snippet; public class Snippet { public void method() { int a=10; if(a++ > 10) { } } } package snippet; public class Snippet { public void method() { int a=10; if(a > 10) { } } } are not treated in a consistent matter. Since this diagnostic is based on the code generation, if the code can be optimized out, it might vary. Besides the two cases above, there is no further fix plan.
The following are also not consistent.. public class Snippet { private int a=10; public void method() { if(a++ > 10) { } } } public class Snippet { private int a=10; public void method() { if(a > 10) { } } }
Created attachment 183430 [details] proposed patch (incl. tests) This fixes all issues where IfStatement signals "valueRequired==false" to downstream invocations of generateCode() because of an optimization. Note, that ConditionalExpression does not have this kind of optimization. It feels a bit strange to add a flag (field) in CodeStream for this purpose. Scope would have been another place for the field, but CodeStream is closer to the issue at hand. The alternative would have been to refactor Expression.generateCode(BlockScope,CodeStream,boolean) to Expression.generateCode(BlockScope,CodeStream,int) rename last param 'valueRequired' to 'valueUsage' and use these values: IGNORED - corresponds to current value 'false' USED - corresponds to current value 'true' OPTIMIZED_OUT - new for the special case in this bug After the refactoring (very wide impact) the actual fix would be trivial. Olivier: is this patch OK with you?
I forgot to check if loops need a similar fix. Will check before committing anything.
Stephan, I don't think I agree with this patch for now. What I believe should be improved is the following cases: public class Snippet { public void method() { int a=10; if(a++ > 10) { } } } In this case, 'a' is reported as unused and I think this is right since the condition doesn't need to be generated due to the empty block and the fact that it cannot cause any side-effect. public class Snippet { public void method() { int a=10; if(a > 10) { } } } In this case I would also expect 'a' to be reported as unused. But it is not. With your patch you go in the opposite direction. Now in both cases, 'a' is no longer reported as unused. I think in the following cases: for(;a++ > 10;) { } while (a++>10) { } switch(a++) { } 'a' should not be reported as unused as the generation of the condition is required (possible infinite loop). Only the switch case needs to be investigated. If there is no side-effect and the switch has no cases, there is no need to generate the condition. So this could also be fixed. Daniel, Deepak, what behavior do you expect for the if cases described above ? Do you agree that in both cases, 'a' should be reported as unused ? With this new warning, we always claimed that it is closely related to the code generation. So if the code needs to be generated, the code is used. If the code doesn't need to be generated, it can be reported as unused.
(In reply to comment #16) > public class Snippet { > public void method() { > int a=10; > if(a++ > 10) { > > } > } > } > In this case, 'a' is reported as unused and I think this is right since the > condition doesn't need to be generated due to the empty block and the fact that > it cannot cause any side-effect. Olivier, is this case any different? Here 'a' is not reported as unused, but 'b' is... public void method() { int a=10; int b=20; if(a++ > 10 && b++ > 10) { } } > Daniel, Deepak, what behavior do you expect for the if cases described above ? > Do you agree that in both cases, 'a' should be reported as unused ? I am ok either way, as long as all 3 cases are consistent - a is a local variable - a is a method parameter - a is a field and currently they are not.
I'll look into all details once we're decided whether we want more warnings or fewer warnings. So far I followed my understanding of the initial request (warning is BAD) but reversing the direction shouldn't pose a problem.
(In reply to comment #16) > In this case, 'a' is reported as unused and I think this is right since the > condition doesn't need to be generated due to the empty block and the fact that > it cannot cause any side-effect. > Doesn't the same reasoning hold true in the following cases as well ? void foo1() { int a[] = new int[10]; if (a[0] > 10) { } } void foo2() { int a[] = new int[10]; if (a[0]++ > 10) { } } void foo3() { int a[] = new int[10]; if(a.length>10) { } } @SuppressWarnings("null") void foo4() { String s="test"; if(s!=null) { } }
Array access or accessing length field can have a side-effect (NPE, AIOOBE,...). For: @SuppressWarnings("null") void foo4() { String s="test"; if(s!=null) { } } I guess the condition cannot have a side-effect. Then yes it should be optimized out. Note that all your cases (that don't work as expected) are if statements without any body. So even if we have a bug there, this is not a high priority for me.
Adjusted title (was: "Variable marked unused incorrectly") to reflect the turn that this bug has taken :)
Created attachment 183521 [details] alternative patch - work in progress This patch goes the opposite direction from the previous, i.e., be more aggressive in optimization and warnings. Implementation mostly fits into existing structures. Only for recognizing that conditions do not prevent optimizing-out a local, we need information about this AST nesting already during ASTNode.isFieldUseDeprecated(). For that reason I introduced a new bit IsInsideCondition. In order to initialize this bit already during resolve, a new field BlockScope.isInsideCondition is temporarily set while descending into a condition. Currently these nodes produce isInsideCondition: - IfStatement.condition - ForStatement.condition - WhileStatement.condition - SwitchStatement.expression TODO: - DoStatement.condition The bit is then consumed from SingleNameReference, QualifiedNameReference and FieldReference. The issue of possible infinite loops I didn't quite understand. Even if the condition and the local are not generated we produce at least the following lonely bytecode: 2: goto #2 Why do we still need the local? So far all looks well, however, there are issues in how this interacts with bug 328519: - A specific situation can now cause NONTERMINATION. See disabled test _test0066(). - New test test0068() shows that the same warning/error can be reported twice. Both situations relate to restarting codegen by throwing AbortMethod. _test0066() requires that codegen of the enclosing method is restarted based on findings in a method of a local type. This also touches my analysis in bug 328519 comment 15: the current patch uses "useFlag < 0" also for normal read (rather than only compound as before). If we go that road more work needs to be done. Specifically, I wonder if it is wise to restart codegen when a local is detected late as being used. With the current work this will effect many locals in many situations thus causing a significant number of restarts. Would it be possible & useful to invert the logic, i.e., when in doubt initially do generate and only restart when we know it can be optimized out (when reporting the error/warning)? Another interference is with recent work in bug 314830: I had to weaken a condition in SwitchStatement.generateCode() that was introduced just recently. Olivier, could you check if the change violates any of your intentions? Since this patch has greater impact than just reporting a few more warnings (more optimization, more codegen restarts) I'm taking a break at this point, asking whether and how much of this want. If only consistency is demanded, IMHO my previous patch is much easier to get consistent, while the more aggressive approach naturally bears more risks. Instead of more aggressive optimization/warning we could, e.g., check if we could do a better job on reporting the "root causes", because all cases involved have something like an empty block or a condition that can be optimized into a constant etc. If all of these are reported properly (and at least most already are) then we are actually speaking about secondary problems.
Executive summary of the longish previous comment: Doing more aggressive optimization/reporting inherently bears more risks. Straight-forward implementation trying to re-use existing mechanisms currently causes NONTERMINATION in a specific situation and potentially impacts performance (more codegen restarts). In order to avoid these effects significant restructuring might be necessary. Suggestion: don't force significant restructuring only for handling more secondary problems, but go back to previous patch, which establishes consistency in a more conservative way + check if all primary problems are reported sufficiently. Please comment, thanks.
Created attachment 183528 [details] alternative patch - work in progress (with regression fix) Running jdt.core.tests.compiler detected a regression (NPE). I updated the patch with a trivial fix. Also, BatchCompilerTest would need adjustment (expect more warnings). This is not yet included in the patch.
I prefer that patch. Deepak, please give it a try. It should work as expected.
(In reply to comment #25) > I prefer that patch. You mean the one from comment 24? Did you have a chance to look at _test0066() and test0068()? Since you recently worked on the restart mechanism maybe you see a simple solution I could not see. Otherwise the issue in _test0066() is pretty bad.
(In reply to comment #26) > You mean the one from comment 24? yes. > Did you have a chance to look at _test0066() and test0068()? > Since you recently worked on the restart mechanism maybe you > see a simple solution I could not see. Otherwise the issue in > _test0066() is pretty bad. I didn't check these tests. I'll do it tomorrow.
Deepak, since test _test0066 is showing a bad side-effect with the current patch, this cannot be released as is and I'll go over it one more time. So no need for you to check the actual patch.
Created attachment 183877 [details] Proposed fix + regression tests This takes the previous patch and makes the necessary changes to pass the test0066. The restart strategy is more complex now, but it seems to work fine. Stephan and Srikanth, please take a look as this is now a pretty big patch. I'll rerun all tests to make sure I didn't break anything with latest changes.
I found a problem recompiling the whole SDK. I am investigating.
I realized debugging the problem that with the current patch the restart strategy is used way too many times. This should be exceptional, but the normal mode. Right now, any if statement in which a local is used the first time doesn't tag it anymore as a use. So as soon as the code generation is reached, it has to run multiple times in order to generate the local properly. This is not acceptable. So I need to reconsider the whole strategy for this bug. In order to fix a corner case, we are now penalizing normal cases. In the worse case, this won't be fixed as normal code (with some statments in the block) works fine. I continue to investigate.
If you still want this warning, should I try to get all this out off generateCode? The initial reason why we ended up with this kind of analysis during generateCode was the consistent availability of the valueRequired flag. From a quick glance it appears we could add the same flag also to analyseCode. This would be a big patch as all of the AST is affected, but it wouldn't make the code much more complex. In fact it might even clean up some existing analyses, I would think. Do you want me to perform an experiment in this direction?
(In reply to comment #32) > Do you want me to perform an experiment in this direction? Sure, please do. I'll reapply my changes to get the proper restart in the code generation if needed. Ideally I would like the extra analysis to be done only in case of empty block for the if statement. The switch statement needs a bit of work as explained in comment 16. The current code works fine if the blocks are not empty. So we should trigger some extra work only in this case. This would simplify the problem I believe. Otherwise we will end up impacting all code not just these corner cases.
Let me recap what I would like to see: 1) report the warning only if the value is never generated 2) handle both cases in comment 0 in a consistent manner. In this case both cases should report a warning 3) test cases in comment 13 should also be handled. All these cases with empty blocks are corner cases. I don't consider them as "real" cases. If we can fix them in a consistent and safe way, we should do it. Otherwise this will be closed as WONTFIX.
I looks like doing all of it in analyseCode should do the trick. As this is becoming a big patch I need a bit more time. Indeed cases like int i=10; if (i>0 && false) { System.out.print(i); } will not report i as unused unless we invest some kind of local two-pass analysis, which we don't want. But, hey, this case already triggers a dead code warning. That should suffice :) I didn't quite understand what you said regarding loops in comment 16. If we optimize-out the local we still generate a goto instruction, thus the infinite loop is still there. And yes, implementation handles locals, arguments and fields.
(In reply to comment #35) > I looks like doing all of it in analyseCode should do the trick. > As this is becoming a big patch I need a bit more time. I removed the target milestone. Committing a big patch like that for a minor issue should not be done the week before a milestone week. > Indeed cases like > int i=10; > if (i>0 && false) { > System.out.print(i); > } > will not report i as unused unless we invest some kind of local > two-pass analysis, which we don't want. But, hey, this case already > triggers a dead code warning. That should suffice :) No, we should not add two-pass analysis. > I didn't quite understand what you said regarding loops in comment 16. > If we optimize-out the local we still generate a goto instruction, > thus the infinite loop is still there. I don't want normal cases to be penalized by an extra check. If we can fix all cases with a minor impact on performance, then why not. The restart of code generation should be exceptional.
Created attachment 183982 [details] patch using new strategy Sorry, this has become a long story, but I guess the changes involved indeed deserve a little documenting: Here is a new patch which bases a simpler (IMHO) implementation on a fundamental change regarding the analyseCode methods. It appeared to me that the two overloads of this method were used inconsistently. In this patch I replaced all declarations of Expression.analyseCode(BlockScope,FlowContext,FlowInfo) with Expression.analyseCode(BlockScope,FlowContext,FlowInfo,boolean) thus forcing all callers to pass the additional arg 'valueRequired'. I added and documented argument 'valueRequired' for all relevant callers: - for all expressions used as a statement -> false - for all children of a node with side effects -> true - compute whether value is required or not for these (might be optimized): - all control statements - binary expressions - CastExpression (depending on possibility to throw NPE or CCE) Based on this change I could move all analysis regarding unused variables from generateCode and friends to analyseCode and friends. Only BlockScope.computeLocalVariablePositions(int,int,codeStream) still 'harvests' the usage information for locals. The big benefit from this is that codeGen no longer has to be restarted for unused locals, because all information is available up-front. Effect: Contrary to my prediction this patch realizes ALL warnings and optimizations from previous patches in the bug without the problems of restarting codeGen. Even some 'if-then' with non-empty 'then' now report when the condition can be optimized thus avoiding to read a variable. The following tests have been adjusted: - BatchCompilerTest: one more warning in two cases - FlowAnalysisTest: one more warning - ForeachStatementTest: one test was using str+=""; to ensure that the variable is used, which is no longer true. Added a variant which indeed ensures that the variable is used. Additionally, I simplified how the analysis for locals is implemented: With accurate information in 'valueRequired' we can now determine usage in one step instead of counting useFlag down-and-up (values < 0). This provides for a more uniform implementation for normal access and compoundAssignment (incl. postIncrement). Scope of the patch: * I could observe no inconsistency between locals, arguments and fields. * I also included DoStatement in analogy to WhileStatement. * While walking through all calls to analyseCode it appears that we might now report new warings for these (more tests TODO): - ArrayAllocationExpression - ArrayInitializer - Assignment - ConditionalExpression Final cleanup (optional / work in progress): - Perhaps the restart mechanism from bug 328519 can be reverted now. This new patch doesn't depend on restarting although it covers the issues of bug 328519, too. The current patch reports abortDueToInternalError("Unexpected unresolved local") instead of throwing AbortMethod. I haven't seen this situation during any tests. - Investigate whether analysis of fields can be assimilated and thus no longer need the new fields BlockScope.isInsideCondition, ASTNode.IsInsideCondition and FieldBinding.compoundUseFlag from my previous patch (comment 24). - One more look at optimization of += for strings (s. ForeachStatementTest) - copyright update :)
Created attachment 184075 [details] Updated patch Updated copyrights (fixed the date only). I would like to discuss why something true/false are passed in for the inner calls to analyseCode(..) regardless of the given value for valueRequired. This seems to mimic what was done in the generateCode(..) calls. This is not true for org.eclipse.jdt.internal.compiler.ast.ArrayAllocationExpression. So I'd like to clarify that part. Let me know when you would have time to discuss this. Stephan, you should update all copyrights with your information.
Srikanth, could you please also take a look at these patches and let me know what you think? Thanks.
(In reply to comment #38) > I would like to discuss why something true/false are passed in for the inner > calls to analyseCode(..) regardless of the given value for valueRequired. Consider MessageSend: bar.foo(zork); the value of the message send is unused, yet the values of bar and zork are required, because the message send must be generated for the sake of its side effects. This is an example of: "- for all children of a node with side effects -> true" > This is not > true for org.eclipse.jdt.internal.compiler.ast.ArrayAllocationExpression. So > I'd like to clarify that part. My rationale was: // allocation itself has no side effect => may optimize all child nodes Consider int[] foo() { int i = 0; if (new int[i++] == null) ; if (new int[]{i++} == null) ; return new int[i++]; } for the first array allocations the incoming value for 'valueRequired' is false, because the condition is not needed. In that case the dims and initializer subnodes only need to generate side effects, their values are unused in case the allocation itself is optimized out, which it could. However, the last array allocation is used and thus its child nodes need to compute their values. Thus, used-ness of dims and initializer is driven by the incoming 'valueRequired'. Changing the last line to 'return null;' will render 'i' unused. As I'm speaking I realize that I forgot to add the unboxing check in this location! Thanks for asking! I'll add this testcase: void foo() { Integer i = null; if (new boolean[i] == null); // don't optimize, has a side effect (NPE) } > This seems to mimic what was done in the generateCode(..) calls. I admit that consistency with generateCode is not trivial. It seems generateCode may optimize more, but everything marked unused during analyseCode *must* be optimized out, otherwise we'd be back at restarting codegen :) So for the case of ArrayAllocation we must either change analyse back to always claiming valueRequired of its children (conservative) or add optimization to generateCode (aggressive). I wonder if we should generally remember the incoming 'valueRequired' from analyseCode so we can check for consistency during generateCode? At least for a development version?
Created attachment 184138 [details] updated patch (v7) Some improvements based on feedback and another walk through all affected AST classes: SingleVariableName.generateCompoundAssignment and generatePostIncrement - pull unused-analysis out of switch(localBinding.type.id) -> more consistent - added disassembly to test0058 Surprised about the difference between if (cond) ; and if (cond) {} I added an override for isEmptyBlock() to EmptyStatement. Please comment. I added optimization to generateCode for alignment with analyseCode in these classes: - ArrayAllocationExpression (with test, incl NPE due to unboxing) - ArrayInitializer (with test) - ArrayReference (no test yet) Added entries to contributor section. Two AST classes I'd recommend to have one more look at: - InstanceOfExpression does not optimize, can it cause any sideeffect?? - CombinedBinaryExpression: it might be possible to trigger the abort "Unexpected unresolved local" because generateCode may do less optimization than analyseCode. No test case yet. Tests compiler and model are currently running locally.
Created attachment 184156 [details] updated patch (v8) Please forget what I wrote about ArrayReference, it wasn't tested.. I forgot AIOOBE. I reverted that part. (For the records: this surfaced as a regression in AutoboxingTest.test130(): The regression was due to over-aggressive optimization. If ArrayReference were a candidate for optimization it would have to check UNBOXING, too).
Created attachment 184357 [details] Updated patch (v9) Updated InstanceOfExpression and removed previous restart mechanism. Will see if we can further optimize the string concatenations.
One tiny thing I forgot: the last test in the patch fails in compliance JDK1_6, but only because the generated stack map is not matched in the expected result. I have a fix in my workspace, to be incorporated in the next round.
(In reply to comment #39) > Srikanth, could you please also take a look at these patches and let me know > what you think? I glanced through the latest patch and didn't find anything wrong with it. That said, this issue has gotten complicated enough that a close scrutiny will require much more time - which I don't have right now :( Looking at this bug and at bug 185682, it looks like these are consuming quite a bit of time and the patches look substantial in size (admittedly a lot of changes are due to refactoring needed by the fix) - I hope it is worth it. I can review this in more detail after I finish up with the review of APT memory tuning work.
(In reply to comment #45) > Looking at this bug and at bug 185682, it looks like these are consuming > quite a bit of time and the patches look substantial in size (admittedly > a lot of changes are due to refactoring needed by the fix) - I hope > it is worth it. This is where I am not sure. It seems to work pretty well and this would be consistent with what we always did in the past (optimize out variable that are not used). Now the question is more: do we really need to optimize it out if we report it as unused. With a quickfix it is trivial for the user to remove it completely from the code and then the optimization is useless as the variable is no longer in the code. I'd like to discuss that point at the next team call.
Yep, these issues have escalated more than we initially intended :) So here's my 2c. on where we are currently: - bug 185682 helps to detect more relevant problems, I wouldn't want to revert that. - bug 328519 made optimization consistent with the new warnings. This applied a code gen restart mechanism so that we more aggressively optimize up-front and restart if later we detect that an optimized local is actually used. For the scenarios of bug 185682 this was safe and the penalty was incurred only seldomly (I think) - this bug primarily aimed at making warnings more consistent. Since bug 185682 we reported some corner cases involving, e.g., (i++ > 0) but not (i > 0). When applying mechanisms from bug 185682 also to the new scenarios this caused havoc into the mechanism from bug 328519. Thus I rewrote the mechanisms of bug 185682 to analyze used/unused already during analyseCode thus avoiding to trigger restart, and the restart mechanism could safely be reverted. IMHO the new structure is a general improvement as it allows us to do more analysis during analyseCode so that generateCode will have all necessary information up-front. Of course, the restructuring requires some scrutiny due to its wide impact on all of the AST. Alternatively, I could see the following compromise: - Fully decouple all special "unused" warnings from optimization. These are: - usage only in postfix or compound assignment - usage only in an unnecessary condition - Always generate all locals affected by the above regardless of the warning - Only optimize out locals if they are definitely UNUSED This could be achieved by fully backing out bug 328519 and rewriting the patch from this bug starting from comment 22. I'm leaning towards using the latest patch (incl. the restructuring) because - analysis can now be kept out off generateCode (which looks cleaner to me) - it removes potential confusion due to overloaded analyseCode in the old implementation. - its a patch we already have and which has been challenged by Olivier and by tests. I hope this serves as input for the team call.
(In reply to comment #46) > Now the question is more: do we really need to optimize it out if we report it > as unused. With a quickfix it is trivial for the user to remove it completely > from the code and then the optimization is useless as the variable is no longer > in the code. If you put it that way, the choice is readily made for me. While on the one hand, we don't want to be paralyzed with fear of change, the scale and scope of change does raise some concerns in my mind. So much so that I would be inclined to opt for the quick fix route. If this appears a tad conservative, may be you can blame it on the flow analysis related fires that have been fought in the not so distant past:) > I'd like to discuss that point at the next team call. Sure.
(In reply to comment #48) > > I'd like to discuss that point at the next team call. > Sure. We will decide what we do with this at the next call (January 10th).
When I tried to apply the patch on HEAD, I got into a state that is completely broken. I'll check again tomorrow what is going on.
I think the problems are coming from the fix for bug 318682.
(In reply to comment #49) > (In reply to comment #48) > > > I'd like to discuss that point at the next team call. > > Sure. > We will decide what we do with this at the next call (January 10th). I gave the existing patch another look and here is my 2 cents. The patch is big and it is not the complexity per se that makes me feel nervous - but complexity relative to what the bug fix accomplishes and where we are in 3.6 If it is a really crucial feature or a nasty blocker problem and the fix is complex, we will have to bite the bullet and go ahead, but IMO, here we have some maneuvering room/ scope for discretion. In principle, I want to encourage the fix but given M6 is almost knocking at the door, worry that any issues with the patch may surface late in the game and add pressure we can do without. So, the risk/reward picture suggests to me that we can simply wait until 3.8 M1 and include this fix at that time.
Since the current patch doesn't apply straight on HEAD anymore, I also favor a move to 3.8M1. This will allow us to "improve" diagnosis at this time once for all and let us have more time to test the change. Like this we can focus on fixing other bugs for 3.7 which are changing a fewer number of classes at the same time.
(In reply to comment #53) > Since the current patch doesn't apply straight on HEAD anymore, I also favor > a move to 3.8M1. I agree on the conclusion. The observation that it doesn't apply to HEAD anymore might be indicating that it will become very difficult to update the patch after 6 more months of development. But then again we still have the option to use the compromise strategy outlined in comment 47: "Alternatively, I could see the following compromise: - Fully decouple all special "unused" warnings from optimization. These are: - usage only in postfix or compound assignment - usage only in an unnecessary condition - Always generate all locals affected by the above regardless of the warning - Only optimize out locals if they are definitely UNUSED This could be achieved by fully backing out bug 328519 and rewriting the patch from this bug starting from comment 22." > Like this we can focus on fixing other bugs for 3.7 which are changing a > fewer number of classes at the same time. That sounds very good :)
Apparently I didn't find time for this bug in a long time. If s.o. else wants do take over, early Mars would probably be a good opportunity. One way to tackle this would be by starting at comment 37 and see if the strategy outlined together with the (now stale) patch is a feasible direction in HEAD. My feeling is: the relatively small benefit justifies the comparatively big code changes only if it is felt that this might be a better design also for other concerns. I don't fully recollect the details of the "compromise" mentioned in comment 54, but that should be considered before diving into huge code changes :)
While comment 0 looks like we where raising a false warning, discussion later revealed the opposite: warnings are inconsistent because we raise fewer warnings than theoretically possible. Attempts to address the problem led to patches that made folks very uncomfortable regarding the risk involved. Since this issue never again came to the table, the motivation for change is too weak to compensate for the risk. => Closing WONTFIX.
Verified for Eclipse Version: 2019-12 (4.14) M1 with Build id: I20191008-1800
