ExtensionContext.Store#computeifAbsent(...) method signatures - Final hour concern - before 6.0.0 is GA

[kaipe]

Final hour concern - before 6.0.0 is GA!
I'm surprised to see these new method signatures on ExtensionContext.Store ...

<K, V> Object computeIfAbsent(K key,
		Function<? super K, ? extends V> defaultCreator);
<K, V> V computeIfAbsent(K key,
		Function<? super K, ? extends V> defaultCreator,
		Class<V> requiredType);

... because imho it would be sufficient with just this one ...

<K, V> V computeIfAbsent(K key, Function<? super K, ? extends V> defaultCreator);

... having V as return-type.

Current javadoc also pushes for the third parameter Class<V> requiredType as a mean for better type safety:

		 * <p>For greater type safety, consider using
		 * {@link #computeIfAbsent(Object, Function, Class)} instead.

However, I dont agree, in part because this third method-parameter of type Class<V> is not much help when considering generic type-parameters (e.g. collection types). - This example addresses it:

static class Config {
    static List<Config> parse(String value) {return List.of();}

    static List<Config> resolve(Method m) {
        return parse(m.getAnnotation(ConfigData.class).value());
    }
}
@Retention(RetentionPolicy.RUNTIME) @interface ConfigData {  String value();  }

ExtensionContext.Store myStore;

/** alternative hack with proposed return-type V: */
<K,V> V computeIfAbsent(K key, Function<? super K, ? extends V> defaulter) {
    return (V) myStore.computeIfAbsent(key, defaulter, Object.class);
}

List<Config> withReturnType_V_thisCompilesWithoutCast__(ExtensionContext ctx) {
    return computeIfAbsent(  ctx.getRequiredTestMethod(),   Config::resolve);
}
List<StringBuilder> __andPreventsThisFromCompiling_asIntended__(ExtensionContext ctx) {
    return computeIfAbsent(  ctx.getRequiredTestMethod(),   Config::resolve);
}

List<StringBuilder> __but_core_Store_method_allowsThisToCompile/*!!!*/(ExtensionContext ctx) {
     /* ... i.e. the "requiredType" parameter does not enforce the desired type safety! */
     return myStore.computeIfAbsent(  ctx.getRequiredTestMethod(),  Config::resolve,  List.class);
     /* Do note how this compiles - and also
      * generic type-parameter erasure can make it survive quite far downstream during runtime. */
}

Summary

I see poor type safety with the computeIfAbsent(...) methods signatures of version 6.0.0-RC3
and instead I propose a single method having return-type V ...

<K, V> V computeIfAbsent(K key, Function<? super K, ? extends V> defaultCreator);

... as a simplier and more type safe solution.

[marcphilipp]

Thanks for raising this! The reason the Class<V> requiredType parameter provides type safety and allows returning V rather than Object is that the Class object is used to cast the already stored value to V. A prior call to put might have stored an instance of a different type (not V or a subclass thereof) so we can't return V without this parameter.

[marcphilipp]

However, I dont agree, in part because this third method-parameter of type Class<V> is not much help when considering generic type-parameters (e.g. collection types)

To support that, we would need something like Guava's TypeToken

[kaipe]

Sorry but I dont follow here

A prior call to put might have stored an instance of a different type (not V or a subclass thereof) so we can't return V without this parameter.

What is it we cannot return without "this parameter"? Do you have an example? - I cannot see what I'm missing here.
My point is that ...

<K,V> V computeIfAbsent(K key, Function<? super K,? extends V> defaultCreator)

... provides better type safety. - I dont see how an original value of a different type makes any significant difference.

E.g. if original value is of type Integer and we computeIfAbsent(k,f, String.class) then we get ClassCastException, right? Same thing happens with my example code and also if you assign the return value to a String variable. Plus it would also provide improved compile type safety

[marcphilipp]

Sure, here's an example:

store.put("key", "foo");
var value = store.computeIfAbsent("key", __ -> 42);

In this case, computeIfAbsent returns "foo". Therefore, if value was typed Integer here, we would offer "faux" type safety because any prior call to put (or any other method that modified the value stored under key) could have stored an object of a different type. The overload taking an additional Class<T> does an explicit check rather than an unchecked cast.

E.g. if original value is of type Integer and we computeIfAbsent(k,f, String.class) then we get ClassCastException, right? Same thing happens with my example code and also if you assign the return value to a String variable. Plus it would also provide improved compile type safety

Yes, you'd get a ClassCastException. The difference is that we don't pretend to know the return type and trick the compiler into compiling the remainder of the code (after the call to computeIfAbsent) as if value was of type T. Instead, the extra parameter signals that a certain type is required and will be checked for. Thus, if computeIfAbsent returns normally, you can be certain that value is of type T.

[marcphilipp]

Re TypeToken, there's a related PR introducing a similar class. We should consider making it general purpose and adding an overload in ExtensionContext.Store and NamespacedHierarchicalStore that takes a TypeDeclaration<T> (or whatever we end up calling it) instead of a Class<?> to support generics.

That being said, we won't change ExtensionContext.Store#computeifAbsent for the 6.0 release so I'm closing this issue.

[github-actions]

Please assign a milestone to this issue or label it with type: task or type: question.