RFR: 8342967: Lambda deduplication fails with non-metafactory BSMs and mismatched local variables names [v2]

[Maurizio Cimadamore]

On Fri, 25 Oct 2024 11:33:17 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> Aggelos Biboudis has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Address review
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TreeDiffer.java line 693:
> 
>> 691:                         && scan(tree.init, that.init);
>> 692: 
>> 693:         if (!tree.sym.owner.type.hasTag(TypeTag.METHOD) && result) {
> 
> I would rewrite this (for improved readability) as:
> 
> 
> JCVariableDecl that = (JCVariableDecl) parameter;
>  result =
>                 scan(tree.mods, that.mods)
>                         && scan(tree.nameexpr, that.nameexpr)
>                         && scan(tree.vartype, that.vartype)
>                         && scan(tree.init, that.init);
> 
> if (tree.sym.owner.type.hasTag(TypeTag.CLASS)) {
>     // field names are important!
>     result &= tree.name == that.name;
> }
> 
> if (result) {
>      equiv.put(tree.sym, that.sym);
> }

And, now that I wrote that I realize there's another issue in the code (from before your changes). I don't think `equiv` is really meant to be for non-local variables. IMHO the logic should be:
1. if we're comparing two local variables, do a structural comparison (and ignore the names)
2. if we're comparing fields, just check the symbols are the same.
And... I'm not sure there's ever a case where (2) applies? E.g. do we really want to deduplicate lambdas containing similar local class definitions? @cushon what do you think? What was the original intention for this code?

> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TreeHasher.java line 104:
> 
>> 102:             }
>> 103:         }
>> 104:         if (sym instanceof PoolConstant.Dynamic dynamic) {
> 
> You could in principle add an `hashSymbol` method which does this dance - then you reuse it from both `visitIdent` and `visitSelect`

I'm not sure if `visitVarDef` is ok here. Again, we seem to assume that all variables encountered here are locals - and we use a number to hash them. This is fine for locals, but I think a field should just hash to the field symbol's hash? @cushon what do you think?

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/21687#discussion_r1816523345
PR Review Comment: https://git.openjdk.org/jdk/pull/21687#discussion_r1816527035