RFR [16/java.xml] 8248348: Regression caused by the update to BCEL 6.0

[Joe Wang]

Hi Peter,

Yes, they must agree: o1.equals(o2) => o1.hashCode() == o2.hashCode(). I 
think that was the original motivation in BCEL 6.0 to add the hashCode() 
method since before that, the Instruction class overrode equals() but 
not hashCode(), so two Instructions that were equal had different 
hashcode. The InstructionComparator compares opcode in general, and adds 
conditions for certain subtypes, e.g. BranchInstruction (BI) among 
others. What the InstructionComparator does was a complicated matter 
originated from how the whole process was designed. They had given it a 
lot of thoughts, replacing HashSet among other things for example. The 
current form was a compromise reached in BCEL 6.0 after a long 
discussion. One such compromise was that BIs were treated uniquely 
(unequal), allowing targeters to be added to a HashSet as unique items, 
and then be able to be removed by themselves (references). For the later 
to happen, the hashcode must remain the same for the same object.

Regards,
Joe

On 6/25/2020 11:53 PM, Peter Levart wrote:
> Hi Joe,
>
> For an object to correctly function as an element of a HashSet, its 
> hashCode() and equals() methods must agree: o1.equals(o2) => 
> o1.hashCode() == o2.hashCode(). I see equals() method delegates to a 
> global InstructionComparator instance which can even change. Suppose 
> it does not change. But what does such InstructionComparator compare? 
> opcode (among other things)?
>
> Regards, Peter
>
>
> On Fri, 26 Jun 2020, 01:56 Joe Wang, <huizhe.wang at oracle.com 
> <mailto:huizhe.wang at oracle.com>> wrote:
>
>     Hi,
>
>     Please review a fix to a BCEL regression. At issue was the
>     addition of
>     hashCode() method to Instruction.java in BCEL 6.0. This hashCode()
>     method was implemented to return the instruction's opcode that
>     unfortunately is mutable. The problem hasn't showed up until the code
>     path led to calls to remove from a HashSet a target that has been
>     changed since it was added to the HashSet. The proposed patch is
>     to make
>     the hashCode final/immutable.
>
>     This patch implies that a target object is considered the same one
>     even
>     if its field values may have been changed. It therefore may not be
>     appropriate in other situations (or may cause problems). However,
>     since
>     it had always had no hashCode() override before BCEL 6.0, thereby
>     relying on Object's identity hash code, its use case has been limited
>     and time-tested. It can benefit from this patch in that it
>     provides the
>     same function as Object's hash code, and then serves as a reminder
>     (to
>     any who might read the code) how it was used (objects considered
>     to be
>     the same over the course as far as the hashCode is concerned).
>
>     JBS: https://bugs.openjdk.java.net/browse/JDK-8248348
>     webrevs: http://cr.openjdk.java.net/~joehw/jdk16/8248348/webrev/
>
>     Thanks,
>     Joe
>