Bugzilla – Full Text Bug Listing
| Summary: | [1.7] String switch statements break conditional breakpoints | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] JDT | Reporter: | Curtis Windatt <curtis.windatt.public> | ||||||||
| Component: | Debug | Assignee: | Michael Rennie <Michael_Rennie> | ||||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||||
| Severity: | normal | ||||||||||
| Priority: | P3 | CC: | ankur_sharma, daniel_megert, markus.kell.r, Michael_Rennie | ||||||||
| Version: | 3.7 | ||||||||||
| Target Milestone: | 3.7.1 | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | Linux | ||||||||||
| Whiteboard: | |||||||||||
| Bug Depends on: | |||||||||||
| Bug Blocks: | 349400 | ||||||||||
| Attachments: |
|
||||||||||
This might be something to do with my configuration. I have the same problems in detail formatters, expressions, etc. (In reply to comment #1) > This might be something to do with my configuration. I have the same problems > in detail formatters, expressions, etc. The problem is coming from our Snippet source generator, we are not correctly detecting that the switch statement is going to return a boolean value, so we end up with the compiled expression as follows: package a; public class WatchBug { void ___run( java.lang.String test) throws Throwable { switch (test) { case "one": System.out.println("Accept"); return true; default : System.out.println("Fail"); return false; } } Created attachment 198687 [details]
fix
The problem was actually a bit deeper down in the ASTInstructionCompiler - we had to account for the string literal type in the equals expression to make sure we are comparing the string value rather than the ObjectReference value.
Created attachment 198688 [details]
better fix
Here is a better fix that also considers the operands equal if their values are null.
The patch works well for me, changes seem reasonable. The patch does not include copyright updates. applied fix to the branch (with copyright updates). This change is not good, since it converts the EqualEqualOperator into an "Object#equals(Object)"-operator.
E.g. the result of this expression was 'false' in 3.7, and that was correct:
new String("one") == new String("one")
There are 2 caveats with the switch-on-String feature:
1. String comparisons must use equals(Object), not ==. E.g. change comment 0 to use:
String test = new String("one");
=> works with the patch
2. If the switch expression evaluates to null, the SwitchStatement must throw a NullPointerException. E.g. change comment 0 to use
String test = null;
=> broken with the patch
(In reply to comment #7) > There are 2 caveats with the switch-on-String feature: > > 1. String comparisons must use equals(Object), not ==. E.g. change comment 0 to > use: > String test = new String("one"); > > => works with the patch Prior to this patch the == operator was overloaded to do #equals comparisons when the operands are not IJavaPrimitiveType, so I specialized the operator a bit for comparing strings. We could just as easily push a SendMessage("equals", "()Z", 1, "Ljava.lang.Object", counter) node instead of EqualsEqualsOperator in the switch-on-String case. > > 2. If the switch expression evaluates to null, the SwitchStatement must throw a > NullPointerException. E.g. change comment 0 to use > String test = null; > > => broken with the patch That makes sense. > Prior to this patch the == operator was overloaded to do #equals comparisons
> when the operands are not IJavaPrimitiveType [..]
I initially thought the same when I saw
default :
equals= leftOperand.equals(rightOperand);
But this only compares IJavaValue objects with equals(..). AFAICS, JDIValue#equals(Object) then uses Value#equals(Object), and that one is implemented by ObjectReferenceImpl#equals(Object), which really compares the JdwpObjectID.
=> I don't think the EqualEqualOperator ever used Object#equals(Object).
Created attachment 198848 [details] update This patch reverts all changes to the EqualsEqualsOperator and properly sends a Object#equals message for comparing the switch cases. If the example is changed in comment #1 to be null, a NullPointerException is now thrown as well. applied patch to branch verified in I20110613-1736 with Java7Patch |
String test = "one"; switch (test) { case "one": System.out.println("Accept"); return true; default: System.out.println("Fail"); return false; } Code such as the following will hit the first case when executed in a java application, but will fall into the default case when used in a conditional breakpoint.