Constant descriptor resolved too soon during constant resolution?

[Brian Goetz]

Here's my version, similar but with a different error handling strategy:

@Override public Optional<?extends ConstantDesc<ConstantDesc<T>>> describeConstable() {
     ConstantDesc<?>[] args =new ConstantDesc<?>[bootstrapArgs.length +5];
     args[0] =bootstrapMethod.owner().descriptorString();
     args[1] =bootstrapMethod.methodName();
     args[2] =bootstrapMethod.methodType().descriptorString();
     args[3] =constantName;
     args[4] =constantType.descriptorString();
     for (int i=0; i<bootstrapArgs.length; i++)
         args[i+5] = (ConstantDesc<?>) ((Constable)bootstrapArgs[i]).describeConstable().orElseThrow();
     return Optional.of(DynamicConstantDesc.of(BSM_DYNAMICCONSTANTDESC, ConstantDescs.DEFAULT_NAME,
                                               CR_DynamicConstantDesc, args));
}


All that said, I'm not sure that we land in the right place here; it is probably more work to LDC this DCD from the constant pool than to construct a fresh one.  Its possible we want some more control knobs in Constable to feed back hints into the compiler as to whether or not constantizing is likely to be profitable, or perhaps we need to revisit the constantBoostrap() protocol for DCD.

So, excellent bug report -- four issues in one!

  - Bug -- compiler incorrectly optimizes away static initializer
  - Bug -- DynamicConstantDesc.describeConstable() needs to quote its static args
  - API sharp edge -- varargs version of DCD.of(...)
  - Possible API insufficiency -- constant-izing DCD may be an anti-optimization


On 8/4/2018 5:36 AM, Jorn Vernee wrote:
>> Brian told me in an earlier email that `ConstantDesc`s should also be
>> able to describe themselves, so I think the only missing puzzle piece
>> here is to add a method to `ConstantDesc` for that:
>
> Or I guess a much simpler solution is to cast the descriptors to 
> `Constable`, since any `ConstantDesc` _should_ implement it:
>
> ```
> diff -r 8ba8e64cc9c2 
> src/java.base/share/classes/java/lang/constant/DynamicConstantDesc.java
> --- 
> a/src/java.base/share/classes/java/lang/constant/DynamicConstantDesc.java 
>   Thu Jul 26 22:07:46 2018 +0200
> +++ 
> b/src/java.base/share/classes/java/lang/constant/DynamicConstantDesc.java 
>   Sat Aug 04 11:29:34 2018 +0200
> @@ -364,7 +364,21 @@
>          args[2] = bootstrapMethod.methodType().descriptorString();
>          args[3] = constantName;
>          args[4] = constantType.descriptorString();
> -        System.arraycopy(bootstrapArgs, 0, args, 5, 
> bootstrapArgs.length);
> +
> +        int i = 5;
> +        for(ConstantDesc<?> bsArg : bootstrapArgs) {
> +            if(bsArg instanceof Constable) { // should always be true
> +                Constable<?> constable = (Constable<?>) bsArg;
> +                Optional<? extends ConstantDesc<?>> descOp = 
> constable.describeConstable();
> +                if(descOp.isPresent()) { // should always have a value
> +                    args[i] = descOp.get();
> +                    i++;
> +                    continue;
> +                }
> +            }
> +            return Optional.empty(); // fallback just in case
> +        }
> +
>          return 
> Optional.of(DynamicConstantDesc.of(BSM_DYNAMICCONSTANTDESC, 
> ConstantDescs.DEFAULT_NAME,
> CR_DynamicConstantDesc, args));
>      }
> ```
>
> That patch fixes the bug. It generates this BSMT entry for the 
> `DynamicConstantDesc`:
>
> ```
>   0: #26 REF_invokeStatic 
> java/lang/constant/DynamicConstantDesc.constantBootstrap:(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/Class;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;[Ljava/lang/constant/ConstantDesc;)Ljava/lang/constant/DynamicConstantDesc;
>     Method arguments:
>       #27 Ljava/lang/invoke/ConstantBootstraps;
>       #28 invoke
>       #29 
> (Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/Class;Ljava/lang/invoke/MethodHandle;[Ljava/lang/Object;)Ljava/lang/Object;
>       #30 _
>       #31 Ljava/lang/Object;
>       #32 #2:_:Ljava/lang/constant/DirectMethodHandleDesc;
>       #33 Hello,
>       #34 world!
> ```
>
> Which looks right to me. It has a dynamic constant in there to 
> reconstitute the method handle descriptor.
>
> Sorry about my earlier email, I was a little too eager to find a 
> solution and was just kind of thinking out loud in the email and 
> probably being a little incoherent.
>
> Jorn