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

[Jan Lahoda]

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

>> 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?

FWIW, I doubt we meaningfully deduplicate lambdas with local class definitions. I suspect we deduplicate them only if the declarations are not used, like:

         Runnable r1 = () -> {class C {}};
         Runnable r2 = () -> {class C {}};


But not when they are used, like:

         Runnable r1 = () -> {class C {} new C();};
         Runnable r2 = () -> {class C {} new C();};


because the use site (`new C();`) uses different `Symbol`s, and these `Symbol`s are not `equiv`-ed.

So, possibly, simply using `false` once we see a `JCClassDecl` might be a more direct, open, answer.

(Besides aliasing local variables, there are probably other cases where we don't detect semantically equivalent duplicates. Like `Object` and `java.lang.Object`. It is not clear to me if those are important in practice, though.)

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

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