Cleanup Types interface / add Type.ofClass(String)

[graemerocher]

This cleans up the Types interface and also adds forName method to resolve a type by the canonical name if possible. Not sure how doable that is in Quarkus, but we need some way to create types by name.

[Ladicek]

Implementing forName in Quarkus would be relatively straightforward (we wouldn't even have to resolve it, though that would obviously cause issues later :-) ), but we'd need a binary name. Unfortunately, it is not really possible to map a fully qualified name (or a canonical name) to a binary name -- not without looking up whether given simple name corresponds to a class or a package. Mapping in the opposite direction suffers from the same problem. (In practice, $ is seldom used in class names, so it should usually be safe to convert a binary name to a fully qualified name by just replacing $ with .. I won't bet my life on it though :-) )

Also, is forName supposed to work for void, primitive types and arrays? Primitive types and arrays do have a canonical name, so it probably should, but I'd expect the javadoc to say something about that. (Something like "for all types that have a canonical name, as defined by the JLS".)

And finally, while I'm not against adding forName in principle, it feels like it shouldn't be necessary? If we have of(Class), who would call forName(String) anyway?

[Ladicek]

Ah and one nit: I started using must not be {@code null} for @param clauses, because that's a guarantee we expect the caller to make. On the other hand, never {@code null} sounds like a guarantee we make, so I use it in @return clauses.

I can rewrite it in a later PR myself, so no need to worry about that.

[graemerocher]

Ok if it has to be binary name I guess we will need to deal with it though these are slightly awkward for arrays

In terms of why it is necessary you may want to enhance based on the presence of a type on the user classpath and not on the extension classpath (psuedo code):

@Enhancement
    @ExactType(type = FaultToleranceInterceptor.class)
    void addMetrics(ClassConfig<?> clazz, Types types) {
          if (types.forName("some.metrics.MetricsBackend") != null) {
                              clazz.addAnnotation(AnnotationBuilder.of(SomeAnnotation.class).member("metricsEnabled", true).build());
          }
    }

[Ladicek]

Ah I see. Maybe it would be best to limit this kind of thing to classes, because you can create array types easily? Or even better, maybe we could have an API to find a ClassInfo for given binary name? You can then create Type from it easily, and you can even inspect the class if you wish. (Technically, this might already be possible using existing @Enhancement facilities, but not exactly nice.)

[graemerocher]

An API for creating ClassInfo would be good, not sure where that would go. ClassInfo.of(String name) would be nice but that will require another service loader

[Ladicek]

We could generalize the existing service provider interface to support this as well, no need to create a second one.

[graemerocher]

Good idea 👍

[graemerocher]

@Ladicek I started looking at adding a BuildServices interface (see last commit). One complication with ClassInfo.get(..) is that the service loader is package private in the build extensions package so that method will have to go somewhere else?

[Ladicek]

Ah ClassInfo is in language model, forgot about that! Well... I was also thinking maybe we could move AnnotationBuilder to the language model, as well as Types perhaps (that would also have to become a bag of static methods and not a simple "injectable" type), so that would be one solution. Another would be to not add ClassInfo.get(), but add the method to obtain ClassInfo by name somewhere else. Not sure exactly where...

I think I'd prefer the latter -- just to keep the language model potentially easily reusable and/or implementable by everyone else.

Now, where would it go...

Maybe you were right the first time, maybe we should add this to Types. Getting from ClassType to ClassInfo is straightforward. If all methods on Types had a more specific return type (e.g. ofClass(Class<?>) should return ClassType instead of just Type), then we could easily add ofClass(String) returning ClassType and it would be simple.

Sorry for the noise I cause -- I'm investigating a performance issue in parallel to this work, so I have to context switch a bit and it shows.

[graemerocher]

@Ladicek how about this? I am not sure we can make ofClass(Class<?>) return ClassType since if you pass a primitive class you would expect a PrimitiveType back

[Ladicek]

We have of(Class<?>) which returns Type, and we have ofClass(ClassInfo), which I think will always return ClassType. But yes, this would work IMHO.

[graemerocher]

OK marking this as ready for review, I left the changes to switch to BuildServices for service loading as I think we may want to keep that (also couldn't this return the extensions since they also loaded via service loader?)

[Ladicek]

My take is that I'd prefer to have as little of implementation code as possible in the spec API. Whatever can be left to implementers, should be -- including the lookup of extension instances.