Introduce SignatureBuilder API

[mkouba]

This pull request supersedes #116.

Usage example:

import io.quarkus.gizmo.SignatureBuilder;
import io.quarkus.gizmo.Type;

// List<String> test();
method.setSignature(SignatureBuilder.forMethod()
   .setReturnType(Type.parameterizedType(Type.classType(List.class), Type.classType(String.class)))
   .build()
)

See also io.quarkus.gizmo.SignaturesTest.

[Ladicek]

Looks great! I added some minor comments, and I have 2 more general here:

1. I'd generally try to keep most of this private. For example, I don't think we need public Type.toSignature() or Type.isClass() / asClass() etc.
2. I didn't think of this before, but it occured to me when reviewing this. Maybe we could split TypeVariable to TypeParameter and TypeVariableArgument for this particular case? A TypeParameter may have bounds, while TypeArgumentVariable has just the identifier. (TypeArgumentVariable is a terrible name...) This would allow a bit more type safety, e.g. a ParameterizedType wouldn't accept a TypeParameter as a type argument. Not sure it's worth it.

[mkouba]

1. I'd generally try to keep most of this private. For example, I don't think we need public Type.toSignature() or Type.isClass() / asClass() etc.

Well, we would have to use some abstract superclass because interfaces cannot declare private members and we use these methods in impls. I don't think that it's a problem.

2. I didn't think of this before, but it occured to me when reviewing this. Maybe we could split TypeVariable to TypeParameter and TypeVariableArgument for this particular case? A TypeParameter may have bounds, while TypeArgumentVariable has just the identifier. (TypeArgumentVariable is a terrible name...) This would allow a bit more type safety, e.g. a ParameterizedType wouldn't accept a TypeParameter as a type argument. Not sure it's worth it.

Hmm. I'm not sure. From user's POV I think that the name "type variable" is more understandable. In any case, I believe that the current version is "good enough".

[Ladicek]

Added one commit that refactors the SignatureBuilder for better encapsulation (per my ideas above). Feel free to drop :-)

The tests could use some improvements -- I'll get to it next week.

[gsmet]

I will have a look on Monday and try to use it in Quarkus GitHub App.

[Sgitario]

Back from my PTO. I will use these changes in some Quarkus extensions to give some feedback asap. Thanks!

[mkouba]

Added one commit that refactors the SignatureBuilder for better encapsulation (per my ideas above). Feel free to drop :-)

@Ladicek I can't say I like all those (TypeImpl) casts and similar but other than the Signature interface it looks very good.

The tests could use some improvements -- I'll get to it next week.

Feel free to improve the coverage ;-).

[Ladicek]

I don't like the TypeImpl class either, but it was Friday afternoon and I was stupid. I can make Type an abstract class instead of an interface and then I can encapsulate properly without those silly casts. Will fix.

[Ladicek]

Done.

[mkouba]

Done.

Ok, I think that we're ready to go. We could either merge this PR and improve the coverage in a follow-up or wait a little... WDYT?

[gsmet]

Maybe wait for this afternoon? I want to test it on Quarkus GitHub App and report back.

[gsmet]

(Working on Sergey's PR this morning so can't work on that now)

[mkouba]

Maybe wait for this afternoon? I want to test it on Quarkus GitHub App and report back.

👍

[Ladicek]

I did add the most interesting test case I wanted (the horror of inner classes). There's more that could be done for methods, but I feel the code by now is correct by construction. I have a very similar piece of code in Jandex with a lot more tests, so I feel pretty good about what we have.

[gsmet]

@Ladicek yeah I was thinking of either that, or have several flavors of superClass() and interfaces() (which would probably need to be singular or it would be annoying) for the ClassCreator builder which would allow to push parameterized types.

We would then push them to the SignatureBuilder to generate the default signature (that you would still be able to override if you want to do more advanced stuff).

Just pushing ideas, I will let you decide what's best.

[mkouba]

Passing a ClassSignatureBuilder to the io.quarkus.gizmo.ClassCreator.Builder seems to be the "best" option but I don't like it very much.

[Ladicek]

I think we could just add an overload of signature() that would accept ClassSignatureBuilder and when that is used, superClass() and interfaces() become optional. Maybe we could even verify that when both are set, they match.

[gsmet]

Another option would be to create a ClassSignature from the builder and have signature(ClassSignature) as an overload of signature(String).

That would probably be a bit better than passing a builder.

[mkouba]

That would probably be a bit better than passing a builder.

Except that it would require one more useless allocation...

[Ladicek]

Also one more [otherwise] useless class. I didn't get to trying it yet (generic signatures and descriptors are so much fun... :-) ), but I suspect using a builder will be just fine.

[gsmet]

Except that it would require one more useless allocation...

I fail to see how it would ever be in the hot path. But I won't fight for it. I can live with what's already there if you all think it's the way to go.
It gets the dark magic away and makes things far more readable for people not familiar with the Java bytecode.

[gsmet]

Didn't want my review to be blocking, I will let you all have fun with it and merge :).

[mkouba]

Didn't want my review to be blocking, I will let you all have fun with it and merge :).

No problem. It's a great feedback. Thanks!

[mkouba]

Also one more [otherwise] useless class. I didn't get to trying it yet (generic signatures and descriptors are so much fun... :-) ), but I suspect using a builder will be just fine.

I have something... will add a commit - feel free to refactor/throw it away.

[Ladicek]

No throwing away, this is pretty much what I thought I'd do. I think it's fine.

[mkouba]

Didn't want my review to be blocking, I will let you all have fun with it and merge :).

@gsmet How about something like this? https://github.com/quarkusio/gizmo/pull/147/files#diff-f3ceec916aa72c0ab7318fa5821d400412118071f45e55d13310c5f4a3796df9R260-R274

[gsmet]

Much better, me likey!

[Ladicek]

Yay! 😆