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

[Maurizio Cimadamore]

On Fri, 25 Oct 2024 12:02:36 GMT, Jan Lahoda <jlahoda at openjdk.org> wrote:

>> 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.)

@lahodaj  that's my feeling too - but if that's the case, I think all our dedup visitors should be more transparent that that's the case (which you seem to agree with).

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

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