Proposal: Add UserType to process method of TypeProcessor for more flexible code generation

[timvlaer]

I'm writing a TypeProcessor that adds a method in generated code for a thrift union type. Since the processor only gets the generated class as input parameter, it's rather hard to figure out if the generated Thrifty code reflects a union in thrift.

I came up with the following which seems rather fragile to me:

boolean isUnion = type.typeSpecs.stream()
        .filter(e -> "Builder".equals(e.name))
        .findFirst()
        .flatMap(t -> t.methodSpecs.stream().filter(m -> "build".equals(m.name)).findFirst())
        .map(m -> m.code.toString().contains("Invalid union;"))
        .orElse(false);

Would it be a good idea to add the userType as a second argument in the process method? It would make code generation in general a lot easier since generated code can be based upon the thrift schema AST instead of the generated code.

I can draft a PR if this seems a good idea. Changing the method will break the TypeProcessor api.

[benjamin-bader]

It's a nice idea! Upsides are, obviously, this use case becomes simple, as do a number of structurally-similar problems I've run in to myself.

The downside is that it couples processor implementations to specific Thrifty versions, which hasn't historically been true. We've been less careful about backwards-compatibility with UserType and friends than we have with the runtime types.

On the whole that particular tradeoff seems OK to me, but it isn't quite as simple as adding a parameter to the processor interface and calling it a day. There are more than structs and unions that get passed in - IIRC, services, enums, and even constant-holding classes become TypeSpec objects that get processed, so there isn't necessarily a UserType, or even a single thrift entity, that corresponds with the TypeSpec seen by the processor.

Given that, what do you think a more-capable processor interface should look like?

[timvlaer]

Interesting viewpoint, thanks for the insightful answer.

I was thinking of changing the signature to the following, although I understand it makes the UserType implementations part of to the public api. I cannot judge the impact of that.

TypeSpec process(UserType spec, TypeSpec type);

I overlooked the Constants class which is indeed directly generated without UserType. I think that's the only case where code is generated without a UserType. I don't know if it makes any sense to create a ConstantsType, just thinking out loud.

An alternative could be to annotate the generated code with references to the original thrift idl. I see however two drawbacks to this approach: it's either thrift idl in comments which requires the processors to basically re-parse the definition. Or it's an annotation or something but that's probably never as complete as the original definition.

What do you think?