Bugzilla – Full Text Bug Listing
| Summary: | [quick fix] Dead code fix forget that conditional has a special way to compute resulting type | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Rémi Forax <forax> | ||||||
| Component: | UI | Assignee: | Markus Keller <markus.kell.r> | ||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||
| Severity: | normal | ||||||||
| Priority: | P3 | CC: | amj87.iitr, daniel_megert, markus.kell.r | ||||||
| Version: | 3.7 | ||||||||
| Target Milestone: | 3.7 M6 | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Whiteboard: | |||||||||
| Attachments: |
|
||||||||
|
Description
Rémi Forax
(In reply to comment #0) > Build Identifier: I20100706-0800 > Object o = true ? new Integer(1) : new Double(0.0); > //Object o = new Integer(3); // wrong fix done by eclipse > //Object o = 1.0 // correct fix > System.out.println(o); > > Reproducible: Always > > Steps to Reproduce: > run the snippet of code and fix the dead code. I dont quite understand what you mean here. Can you give a clear sequence of steps starting with your code, and explain what are the commented lines in your code for? As I see it, when dead code is reported, you apparently chose the quick fix "Remove(including condition)". When you do this, the expression changes to Object o = new Integer(1); Why should the compiler now treat it as a conditional expression and calculate the type of 'o' using both Integer and Double? BTW, even when one doesnt use the quick fix to remove the dead code, and the commented out lines are uncommented, the code compiles and runs fine. I dont see what's the bug here. (In reply to comment #1) > (In reply to comment #0) > > Build Identifier: I20100706-0800 > > Object o = true ? new Integer(1) : new Double(0.0); > > //Object o = new Integer(3); // wrong fix done by eclipse > > //Object o = 1.0 // correct fix > > System.out.println(o); > > > > Reproducible: Always > > > > Steps to Reproduce: > > run the snippet of code and fix the dead code. > > I dont quite understand what you mean here. Can you give a clear sequence of > steps starting with your code, and explain what are the commented lines in your > code for? > > As I see it, when dead code is reported, you apparently chose the quick fix > "Remove(including condition)". When you do this, the expression changes to > > Object o = new Integer(1); and that's wrong. The result should be a double not an Integer. > > Why should the compiler now treat it as a conditional expression and calculate > the type of 'o' using both Integer and Double? > BTW, even when one doesnt use the quick fix to remove the dead code, and the > commented out lines are uncommented, the code compiles and runs fine. I dont > see what's the bug here. The result should be 1.0 and not 1. Please just run: Object o = true ? new Integer(1) : new Double(0.0); System.out.println(o); or perhaps simpler, just: System.out.println(true ? new Integer(1) : new Double(0.0)); and take a look to the result. Also note that if you select the conditional expression and use Refactor > Extract Method, the return type of the resulting method is double. Rémi (In reply to comment #2) > ... > Please just run: > Object o = true ? new Integer(1) : new Double(0.0); > System.out.println(o); > > or perhaps simpler, just: > System.out.println(true ? new Integer(1) : new Double(0.0)); > > and take a look to the result. I did exactly as you suggested, but I obtained the output "1.0" with the latest I build I20100802-1800. Can you please try on this build? Thanks! (In reply to comment #3) > (In reply to comment #2) > > ... > > Please just run: > > Object o = true ? new Integer(1) : new Double(0.0); > > System.out.println(o); > > > > or perhaps simpler, just: > > System.out.println(true ? new Integer(1) : new Double(0.0)); > > > > and take a look to the result. > > I did exactly as you suggested, but I obtained the output "1.0" with the latest > I build I20100802-1800. Can you please try on this build? Thanks! Yes the result is 1.0 but is you use the quickfix to fix the dead code, it will print 1. The quick fix forget that the resulting type of the conditional is a double and not an Integer. Object o = true ? new Integer(1) : new Double(0.0); System.out.println(o); // print 1.0 using the quick fix, we get Object o = new Integer(1); System.out.println(o); // print 1 So there is a bug in the quick fix Rémi (In reply to comment #4) ... > The quick fix forget that the resulting type of the conditional is > a double and not an Integer. > > Object o = true ? new Integer(1) : new Double(0.0); > System.out.println(o); // print 1.0 > > using the quick fix, we get > > Object o = new Integer(1); > System.out.println(o); // print 1 > > So there is a bug in the quick fix Ok. So we come back to the same point that I was making earlier. When you get the dead code warning and you "fix" it using the quickfix, it _removes_ the conditional expression and replaces it with a harmless assignment expression. Now your code looks like Object o = new Integer(1); System.out.println(o); There is no conditional expression in this code. I repeat my question - why do you think the compiler should remember that there "was" a conditional expression in place of the assignment expression? Once you have removed the conditional expression, the code is recompiled afresh. All that the compiler sees now is the above code snippet with a simple assignment. Hence no ambiguity about the type of o. :) (In reply to comment #5) > (In reply to comment #4) > ... > > The quick fix forget that the resulting type of the conditional is > > a double and not an Integer. > > > > Object o = true ? new Integer(1) : new Double(0.0); > > System.out.println(o); // print 1.0 > > > > using the quick fix, we get > > > > Object o = new Integer(1); > > System.out.println(o); // print 1 > > > > So there is a bug in the quick fix > > Ok. So we come back to the same point that I was making earlier. > When you get the dead code warning and you "fix" it using the quickfix, it > _removes_ the conditional expression and replaces it with a harmless assignment > expression. Now your code looks like > > Object o = new Integer(1); > System.out.println(o); > > There is no conditional expression in this code. I repeat my question - why do > you think the compiler should remember that there "was" a conditional > expression in place of the assignment expression? Once you have removed the > conditional expression, the code is recompiled afresh. All that the compiler > sees now is the above code snippet with a simple assignment. Hence no ambiguity > about the type of o. :) Because fixing a dead code should not change the semantics. The intent of the developer who write the conditional is to return a double, that what the conditional does. So the quick fix should remove the dead code and insert a cast to double. Object o = (double)new Integer(1); System.out.println(o); The other solution is to consider that new Double(0.0) is not a dead code because its type is used to infer the type of the conditional. Rémi (In reply to comment #6) ... > Because fixing a dead code should not change the semantics. > The intent of the developer who write the conditional is to return a double, > that what the conditional does. So the quick fix should remove > the dead code and insert a cast to double. > > Object o = (double)new Integer(1); > System.out.println(o); Dani / Markus, what is your take on this? This is just a bug in the quick fix. It should not change the behavior nor should it try to simplify the code, so the right result is (will be):
Object o = (double) new Integer(1);
Created attachment 181627 [details]
Work in Progress
Work in Progress, don't release this patch (it has bugs).
Created attachment 190589 [details]
Fix & tests
(In reply to comment #0) > Object o = true ? new Integer(1) : new Double(0.0); > //Object o = 1.0 // correct fix The quick fix now has this result (which is equivalent, but more general): Object o= (double) new Integer(1); (In reply to comment #10) > Created attachment 190589 [details] [diff] > Fix & tests Sorry, forgot to include the tests. They are in LocalCorrectionsQuickFixTest. Fixed in HEAD. |