Add TypeSpec#annotationBuilder overload with retention and target parameters

[Marcono1234]

Problem

When creating an annotation type using TypeSpec#annotationBuilder, you also often want to specify the @Retention and @Target of it. It seems currently you have to do that manually.

Suggested feature

It would be useful if there was an annotationBuilder overload which takes retention and target values, for example:

annotationBuilder(String name, RetentionPolicy retention, List<ElementType> targets)

Open questions:

• how should null be treated
  Should it be forbidden, or should the corresponding meta annotation (e.g. @Target) be omitted, so the implicit default is used by the Java compiler?
  Should probably be forbidden, rest of the API seems to be "null hostile" (?)
• how should an empty targets list be treated
  Should the @Target be omitted? Or should a @Target({}) be generated? Which seems valid and only allows usage in other container-like annotations, but does not permit directly annotating something?
• should targets be a List or Set
Set would prevent duplicate values but it would have to make sure the iteration order is deterministic (for example by wrapping as EnumSet in case it isn't an EnumSet already)

(Maybe also an overload with List<String> targets so that you can use the name of a ElementType added in a future JDK version, which is newer than the one you are building on. Not sure how common that is though; could maybe be added later if there is demand for it.)

[Marcono1234]

I can try to provide a pull request for this in case this won't require signing the CLA.

[skku-daniilkim]

@Marcono1234
Actually, why don't we simply add convenience methods like TypeSpec#Builder#setTargets and TypeSpec#Builder#setRetentionPolicy (we should probably choose better names?). This feels more inline with the rest of the API.

Inside the methods, we can perform the checks that are similar to what records implementation already does (checking for a valid kind):

javapoet/javapoet/src/main/java/com/palantir/javapoet/TypeSpec.java

Lines 731 to 737 in e02bae1

	public Builder recordConstructor(MethodSpec recordConstructor) {
	if (kind != Kind.RECORD) {
	throw new UnsupportedOperationException(kind + " can't have record constructor");
	}
	this.recordConstructor = recordConstructor;
	return this;
	}

[Marcono1234]

Might be an option as well, except that for code completion it might be confusing to see these methods when building non annotation types.

Since you most likely always want to set 'retention' and 'target', I think the API should help / encourage you to specify them, without having to call additional methods (?).

Personally I think the record example might not be a good one, it might be better there as well to have MethodSpec as part of the TypeSpec#recordBuilder parameters because you most likely always want to specify record components (otherwise usage of a record is a bit pointless, unless you repurpose it for something other than a 'data holder').

Though maybe there are also use cases where the Builder for records or annotations is created at one place and then the record components respectively 'retention' and 'target' are specified at a different place. So in these cases separate builder methods might be useful. But I am not sure how common these use cases really are, and if they justify making the Builder API more 'bloated' and error-prone / confusing.

[skku-daniilkim]

Fair enough. However, breaking the "null hostility" still bothers me, and having a bunch of overloads sounds like a pain.

What do you think about creating factory methods in AnnotationSpec? AnnotationSpec#createTargetAnnotation and AnnotationSpec#createRetentionAnnotation (again, these are just example names). This is not exactly typical to the code base, but may be a bit cleaner? Then, the typical use will be TypeSpec#addAnnotation(AnnotationSpec.createTargetAnnotation(...)).

On a side note, after looking at the recordBuilder API, I found it slightly confusing that it accepts MethodSpec? It makes sense from the code-reuse standpoint, but it feels too much (?). After looking through other forks, I think FabricMC's recordBuilder looks much nicer:
https://github.com/FabricMC/javapoet/blob/f1a7bd7a00f62b18dc75b49f6117f22a42e0f9ca/src/test/java/com/squareup/javapoet/TypeSpecTest.java#L354-L368 :

TypeSpec rec = TypeSpec.recordBuilder("KeyValue")
        .addModifiers(Modifier.PUBLIC)
        .addRecordComponent(keyComponent)
        .addRecordComponent(valueComponent)
        .compactConstructor(MethodSpec.constructorBuilder()
            .addModifiers(Modifier.PUBLIC)
            .addCode(
                CodeBlock.builder()
                .beginControlFlow("if ($N.indexOf(':') != -1)", keyComponent)
                .addStatement("throw new $T()", ClassName.get(IllegalArgumentException.class))
                .endControlFlow()
                .build())
            .build())
        .addSuperinterface(Cloneable.class)
        .build();

[Marcono1234]

However, breaking the "null hostility" still bothers me

Supporting null was just an initial idea, but you are right the remaining API seems to be null-hostile, so the target and retention parameters should probably be null-hostile as well.

---

and having a bunch of overloads sounds like a pain

Maybe only annotationBuilder(String name, RetentionPolicy retention, List<ElementType> targets) would be needed, that would probably cover most use cases.

• An overload with ClassName instead of String does not add that much value, the user can simply call ClassName#simpleName()
  Actually, this is what all the other TypeSpec builder methods with ClassName parameter do as well. And I find that slightly misleading because it gives the impression that the package name is considered as well while it is actually ignored and is only later specified when creating a JavaFile.
• An overload with List<String> targets to support future JDK targets is at least currently not needed since the JavaPoet minimum JDK version is 17 and no new ElementType constants have been added since then as far as I now.

Separate AnnotationSpec factory methods might make this rather verbose. Though this is just my personal opinion, the maintainers might think differently about this.

---

On a side note, after looking at the recordBuilder API, I found it slightly confusing that it accepts MethodSpec?

I am not sure whether I prefer this approach here or FabricMC's one. But one advantage with the approach here is that it matches more closely how records work: The record has an implicit canonical constructor with the same parameters as the record components.
(But maybe this also has downsides when you want to customize the constructor signature, e.g. with annotations, without affecting the record components? Not sure if that is possible in Java, and if there are use cases for it.)