Constant descriptor resolved too soon during constant resolution?

[Vicente Romero]

On 08/14/2018 06:59 AM, Jorn Vernee wrote:
> Thanks!
>
> I had sent some other general code comments to Brian as well, (I 
> thought he was working on it at that moment), but I should probably 
> just have sent them here. So here they are again:

sure np, I wrapped out the fix proposed by Brian with a fix for the 
compiler issue.

>
> These are comments about `DynamicConstantDesc.java`:
>
> - I noticed that in a couple of places the javadoc links to 
> `DirectMethodHandleDescImpl`, but I think that should be 
> `DirectMethodHandleDesc` instead? (The interface type, not the 
> implementation type).
>
> - Also, the javadoc mentions the concept of a "bootstrap method" 
> several times, but there's no explanation given of what that is 
> exactly. Maybe that could be included in the javadoc for the class? 
> e.g. what signatures are valid for bootstrap methods (I recently found 
> out having `Lookup, String, Class` as first 3 arguments is not 
> actually required).
>
> - Thirdly, I noticed that when calling `withArgs` you often have to 
> manually specify the type parameter. For instance in 
> `Enum.EnumDesc#describeConstable`:
>
> ```
> DynamicConstantDesc.<ConstantDesc<E>>of(BSM_ENUMDESC, CR_EnumDesc)
> .withArgs(constantType().descriptorString(), constantName())
> ```
>
> I've been running into that myself as well when writing descriptor 
> code. The fix I have for this in my local copy is to allow `withArgs` 
> to return a `DynamicConstantDesc` with a different type parameter. i.e.:
>
> ```
>     @Foldable
>     public <R> DynamicConstantDesc<R> withArgs(ConstantDesc<?>... 
> bootstrapArgs) {
>         return DynamicConstantDesc.of(bootstrapMethod, constantName, 
> constantType, bootstrapArgs);
>     }
> ```
>
> Adding the type parameter `R` there. This removes the need to manually 
> specify the type parameter, and also makes sense semantically since 
> the return type of the bootstrap method could depend on one of the 
> arguments (e.g. a generic method who's return type is the type of one 
> of it's arguments).
>
> - Lastly, as a suggestion for solving "API sharp edge -- varargs 
> version of DCD.of(...)". Instead of providing an overload for each 
> combination of arguments you could switch to builder-style methods 
> (which `withArgs` already kinda is). i.e. adding the methods 
> `withName(String constantName)` and `withType(ClassDesc 
> constantType)`, and then dropping the 3 overloads of the `of` method 
> that take a name, type, and name and type (but no bootstrapArgs). 
> Keeping the one that takes just a bootstrap method descriptor, and the 
> one that takes 4 arguments (but change that last one to varargs to 
> allow folding). It's pretty similar, but doesn't have the overload 
> confusion (right?).

as for the above, most of them will for sure be revisited during the 
revision of the sharp edges in the API

>
> ---
>
> Additionally, I wanted to ask if you could add some constants to the 
> `ConstantDescs` class for working with `LambdaMetafactory`. I have 
> these going in my local copy:

the ones below looks useful thanks, we can consider them for addition as 
part of the API review

>
> ```
> diff -r 3b091f0a5df7 
> src/java.base/share/classes/java/lang/constant/ConstantDescs.java
> --- 
> a/src/java.base/share/classes/java/lang/constant/ConstantDescs.java 
> Thu Aug 09 22:07:37 2018 +0200
> +++ 
> b/src/java.base/share/classes/java/lang/constant/ConstantDescs.java 
> Tue Aug 14 12:35:50 2018 +0200
> @@ -35,6 +35,7 @@
>  import java.lang.invoke.MethodType;
>  import java.lang.invoke.VarHandle;
>  import java.lang.invoke.VarHandle.VarHandleDesc;
> +import java.lang.invoke.LambdaMetafactory;
>  import java.util.Collection;
>  import java.util.List;
>  import java.util.Map;
> @@ -150,6 +151,10 @@
>      @Foldable
>      public static final ClassDesc CR_CallSite = 
> ClassDesc.of("java.lang.invoke.CallSite");
>
> +    /** {@link ClassDesc} representing {@link LambdaMetafactory} */
> +    @Foldable
> +    public static final ClassDesc CR_LambdaMetafactory = 
> ClassDesc.of("java.lang.invoke.LambdaMetafactory");
> +
>      /** {@link ClassDesc} representing {@link Collection} */
>      @Foldable
>      public static final ClassDesc CR_Collection = 
> ClassDesc.of("java.util.Collection");
> @@ -264,6 +269,12 @@
>              = ofConstantBootstrap(CR_ConstantBootstraps, "invoke",
>                                    CR_Object, CR_MethodHandle, 
> CR_Object.arrayType());
>
> +    /** {@link MethodHandleDesc} representing {@link 
> LambdaMetafactory#metafactory(MethodHandles.Lookup, String, Class, 
> MethodType, MethodHandle, MethodType)} */
> +    @Foldable
> +    public static final DirectMethodHandleDesc BSM_METAFACTORY_CONDY
> +            = ofConstantBootstrap(CR_LambdaMetafactory, "metafactory",
> +                                  CR_Object, CR_MethodType, 
> CR_MethodHandle, CR_MethodType);
> +
>      /** {@link ClassDesc} representing the primitive type {@code int} */
>      @Foldable
>      public static final ClassDesc CR_int = ClassDesc.ofDescriptor("I");
> @@ -418,4 +429,36 @@
> ClassDesc... paramTypes) {
>          return MethodHandleDesc.of(STATIC, clazz, name, 
> MethodTypeDesc.of(returnType, paramTypes).insertParameterTypes(0, 
> CONDY_BOOTSTRAP_ARGS));
>      }
> +
> +    /**
> +     * Return a {@link DynamicConstantDesc} which resolves to an 
> instance of a
> +     * functional interface type, as if through an invocation of
> +     * {@link LambdaMetafactory#metafactory(MethodHandles.Lookup, 
> String, Class, MethodType, MethodHandle, MethodType)}
> +     *
> +     * @param <T> the type of the functional interface
> +     * @param invokedName The name of the method to implement.
> +     * @param functionalInterface The functional interface type.
> +     * @param samMethodType Signature and return type of method to be 
> implemented
> +     *                      by the function object.
> +     * @param implMethod A direct method handle describing the 
> implementation
> +     *                   method which should be called (with suitable 
> adaptation
> +     *                   of argument types, return types, and with 
> captured
> +     *                   arguments prepended to the invocation 
> arguments) at
> +     *                   invocation time.
> +     * @param instantiatedMethodType The signature and return type 
> that should
> +     *                               be enforced dynamically at 
> invocation time.
> +     *                               This may be the same as {@code 
> samMethodType},
> +     *                               or may be a specialization of it.
> +     * @return a descriptor that resolves to an instance of the 
> functional interface type.
> +     */
> +    @Foldable
> +    public static <T> DynamicConstantDesc<T> 
> ofLambdaCondyBootstrap(String invokedName, ClassDesc functionalInterface,
> + ConstantDesc<MethodType> samMethodType,
> + ConstantDesc<MethodHandle> implMethod,
> + ConstantDesc<MethodType> instantiatedMethodType) {
> +        // Requiring ConstantDesc<...>s here instead of more specific 
> types since arguments might be
> +        // computed at link-time.
> +        return DynamicConstantDesc.<T>of(BSM_METAFACTORY_CONDY, 
> invokedName, functionalInterface)
> +                                  .withArgs(samMethodType, 
> implMethod, instantiatedMethodType);
> +    }
>  }
> ```
>
> (Note that some of the javadoc for that last helper method was taken 
> from the javadoc in `LambdaMetafactory`. I don't know if that matters)
>
> Since I don't have access to `@Foldable` in user code, I found it 
> impossible to use  my own bag-of-constants class, custom descriptor 
> types or other helper methods, since they would inhibit folding. I 
> think it's a good move making it an internal annotation though, some 
> weird problems can occur when using it (e.g. because you're depending 
> on the compiler's class path), but that also means that currently any 
> helper method, global constant or descriptor type that someone might 
> need has to be a part of the standard API.
>
> Best regards,
> Jorn Vernee

Thanks,
Vicente

>
> Vicente Romero schreef op 2018-08-14 02:23:
>> On 08/10/2018 04:25 PM, Brian Goetz wrote:
>>> 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!
>>
>> yep great bug report and analysis too! getting into this after a time 
>> out
>>
>>>
>>>  - 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
>>
>> I have pushed fixes to the condy-folding branch for the bugs listed 
>> above
>>
>> Thanks,
>> Vicente