[code-reflection] RFR: Cleanup and consolidate `ReflectMethods` [v2]

[Paul Sandoz]

On Fri, 27 Jun 2025 12:28:11 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> This PR applies some minor cleanups to `ReflectMethods` and its companion classes.
>> 
>> * A symbol was moved from `CodeReflectionSymbols` to `Symtab` as it was a method in java.base (MH::lookup)
>> * `CodeModelToAST` was merged with `ReflectMethods` (and renamed to `CodeModelTranslator`)
>> * To facilitate the above, all the conversion from model types to javac types (and back) is centralized in `ReflectMethods`
>> * `ReflectMethod.BodyScanner` is now a regular scanner -- the filtering logic is not needed since all the various relevant trees are supported
>> * removed static methods to create javac diagnostics -- we now use the standard methods generated during the build (but this needed to add a qualified export of such generated classes to `jdk.incubator.code`)
>> 
>> There are several `@@@` comments in the code -- for now I left them in place.
>> 
>> I believe there are three main areas for subsequent cleanup and/or consolidation:
>> 
>> ##### Quotable vs. Quoted
>> 
>> The experiments with lambda to `Quoted` conversions were a first stab on how we might want to support code models in lambdas. This requires ad-hoc type system support (in `Attr`), and `ReflectMethods` does not support method references with this strategy. We should consider whether to keep this, given that we seem to have given `Quotable` a more prominent role in the API (and the language support for `Quotable` is much more in sync with what we do for e.g. serialziable lambdas).
>> 
>> ##### OpBuilder vs. Text
>> 
>> `ReflectMethods` still supports two code generation strategies. This doesn't add a lot of additional code, but I wonder if we really need this.
>> 
>> ##### Inner class support
>> 
>> The support for member inner classes and local/anonymous inner classes is a bit spotty. The generated model will emit correct code when e.g. creating an inner class -- that is, the model will correctly pass the enclosing this and captured values (where needed).
>> 
>> But, when translating e.g. a method in an inner class, references to enclosing this, or captured values will not be treated specially -- e.g. `ReflectMethods` will assume such elements to be "in scope". In other words, I have very little confidence that, e.g. we can take a code model corresponding to a method inside an inner class and e.g. generate bytecode for it.
>> 
>> I wonder if it would be better, for the time being, to just skip reflectable methods/constructors/lambdas defined inside a non-static inner class (so as to provide a cleaner boundary ...
>
> Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Cleanup `BodyScanner::loadVar`

Marked as reviewed by psandoz (Lead).

src/jdk.incubator.code/share/classes/jdk/incubator/code/internal/CodeReflectionSymbols.java line 71:

> 69:                         List.nil(), syms.methodClass),
> 70:                 opParserType.tsym);
> 71:         methodHandlesLookup = new MethodSymbol(PUBLIC | STATIC,

One reason to retain this here is it is only used by the incubating `code` module, thereby minimizing changes to `java.base` and therefore reducing the maintenance when making changes independently to this area of code.

Looking more closely at its use i now realize now this declaration can be entirely removed! due to the changes in #424 we can change the code in reflect methods for structural quoting. So leave it how you wish, and I will submit a PR to remove it where ever it is located.

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

PR Review: https://git.openjdk.org/babylon/pull/468#pullrequestreview-2967693586
PR Review Comment: https://git.openjdk.org/babylon/pull/468#discussion_r2172713846