[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`

> Quotable vs. Quoted

I would like to retain targeting to `Quoted` it for now to see how folks might use it.

> OpBuilder vs. Text

We retained the text version in case we needed to fallback in case of bugs when developing, and it was an easy way to get size comparisons. Now that the feature is stable we could remove it (and possibly the public method it calls).

> Inner class support
> 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 between what we support and what we don't support).

Yes, that seems wise, and revisit later on. If we do that can we make note of the issues so we don't forget why we did it? 

> Annotation processing support
> This is a rather advanced functionality, so it might make sense to move this in a separate module which annotation processors might depend on.

Agreed, or even remove it for now?

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

PR Comment: https://git.openjdk.org/babylon/pull/468#issuecomment-3014180653