[code-reflection] RFR: Support storing the code that builds the code model [v18]

[Maurizio Cimadamore]

On Thu, 13 Mar 2025 14:45:51 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> Mourad Abbay has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 20 additional commits since the last revision:
>> 
>>  - Merge branch 'code-reflection' into code-model-storage-option
>>  - Load OpFactory and TypeElementFactory before invocation of opMethod only if the opMethod has there params.
>>  - Fix the remaining compiler tests failures
>>  - Fix some of the test failures (3 remains)
>>  - Fix the remaining test failures of SwitchExpressionTest2
>>  - Fix almost all test failures of SwitchExpressionTest2 (one remaining)
>>  - Pass arrayType instead of eleType in OpBuilder.buildArray
>>  - Ensure that block params are inserted in the correct order
>>  - Add missing imports
>>  - Merge branch 'code-reflection' into code-model-storage-option
>>  - ... and 10 more: https://git.openjdk.org/babylon/compare/a405674e...966005ce
>
> src/jdk.incubator.code/share/classes/jdk/incubator/code/internal/CodeModelToAST.java line 160:
> 
>> 158:                 var argTypes = new ListBuffer<Type>();
>> 159:                 for (Value operand : newOp.operands()) {
>> 160:                     argTypes.add(typeElementToType(operand.type()));
> 
> This seems wrong -- and is related to the point I made above -- the type of the operands here are the types of the actual arguments of the constructor calls. So, if you have:
> 
> class Foo {
>      Foo(String string) { ... }
>  }
>  ```
>  
>  the method type of the constructor call has to say `Foo(String)`. But if you "infer" the type of the constructor argument from the actual argument, if the call is like this:
>  
>  ```
>  new Foo(null)
>  ```
>  
> We should probably use the op's `constructorType` to derive the type of the invoked constructor? (You use that to determine the constructor's owner type)

This can be deferred until we have a better op to model instance creation expression.

> src/jdk.incubator.code/share/classes/jdk/incubator/code/internal/CodeModelToAST.java line 192:
> 
>> 190:                 var type = typeElementToType(fieldLoadOp.resultType());
>> 191:                 var owner = typeElementToType(fieldLoadOp.fieldDescriptor().refType());
>> 192:                 var sym = new Symbol.VarSymbol(flags, name, type, owner.tsym);
> 
> This seems wrong -- the accessed field symbol should be findable in the scope of the class in which the field is declared. It seems to me that we need some piece of code that turns a method/field descriptor into a java symbol. Look at `Lower::lookupMethod` which uses `Resolve::resolveInternalMethod` -- I think here we should do that too.

This is borderline, but again, we can leave it as is for now (the risk is that if we have a bad model javac will just emit a reference to some non-existent field, which will likely cause non-sensical bytecode to be emitted).

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

PR Review Comment: https://git.openjdk.org/babylon/pull/305#discussion_r2013741734
PR Review Comment: https://git.openjdk.org/babylon/pull/305#discussion_r2013755517