Plugin to check if setters are called on Protobuf object

[stym06]

Hey everyone,
I have a usecase to check if setter has been called on the Protobuf generated builder or not.
eg:

If this is my proto definition

syntax = "proto3";

message Person {
  string name = 1;
  string email = 2;
}

I want to add a check so that

Person person = Person.newBuilder().build();

the above code fails, because setName and setEmail methods are not called.

I looked at RedundantSetterCall but could not find anything like my use-case. Can someone guide me as to where I can find it if it's already been written, if not, I can help contribute it.

[stym06]

I have written my own implementation for this. Please let me know if this is feasible for this. Looks to be working. Have not checked with oneof though!

@AutoService(BugChecker.class)
@BugPattern(name="ProtoChecker", summary = "Needs required fields to be present", severity = BugPattern.SeverityLevel.ERROR)
public class ProtoChecker extends BugChecker implements BugChecker.MethodInvocationTreeMatcher {

  @Override
  public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
    Symbol.MethodSymbol method = getSymbol(tree);
    if(method == null || !method.getSimpleName().toString().equals("build")) {
      return Description.NO_MATCH;
    }
    ExpressionTree receiver = ASTHelpers.getReceiver(tree);
    if (receiver == null) {
      return Description.NO_MATCH;
    }

    Type receiverType = ASTHelpers.getType(receiver);
    if(receiverType == null || !isProtobufBuilder(receiverType)) {
      return Description.NO_MATCH;
    }

    Set<String> declaredSetters = findAllSetMethods(receiverType);
    Set<String> calledSetters = new HashSet<>();
    for (ExpressionTree current = tree; current != null; current = ASTHelpers.getReceiver(current)) {
      if (!(current instanceof MethodInvocationTree)) {
        break;
      }
      Symbol symbol = getSymbol(current);
      if (symbol instanceof Symbol.MethodSymbol) {
        String methodName = symbol.getSimpleName().toString();
        if (methodName.startsWith("set")) {
          calledSetters.add(methodName);
        }
      }
    }

    declaredSetters.removeAll(calledSetters);

    if(!declaredSetters.isEmpty()) {
      String missingSetters = String.join(",", declaredSetters);
      return buildDescription(tree)
          .setMessage("The following setters were not called: " + missingSetters)
          .build();
    }

    return Description.NO_MATCH;
  }

  private Set<String> findAllSetMethods(Type builderType) {
    Set<String> result = new HashSet<>();
    for(Symbol symbol: builderType.asElement().getEnclosedElements()) {
      if(symbol.name.toString().startsWith("set") && symbol.name.length() > 3 && !symbol.name.toString().endsWith("Bytes")) {
        result.add(symbol.name.toString());
      }
    }
    return result;
  }

  private boolean isProtobufBuilder(Type type) {
    return type.asElement().getSimpleName().toString().endsWith("Builder");
  }
}

[graememorgan]

Hi stym06!

I'm a little confused as to what your question is. Is this a check you're interested in adding for a specific proto which you know needs its fields set, or do you want this check for all protos?

It's not a check that exists for all protos currently. It's not universally applicable: omitting fields is pretty common.

[stym06]

Thanks for replying @graememorgan . I had a usecase of checking if the setters are called for all the required fields in a Proto2 message.

[graememorgan]

Ah, that's an interesting problem, thanks for the context. We've discussed a general solution to that quite a lot in the past, because yes, a check that warned you in general that you haven't set required fields would be rather nice.

There are two barriers, one technical and one not:

1. There's no programmatic way at compile-time to determine that a field is required. There's just nothing different in the public interface which gets generated. We've discussed some way to get proto descriptors into ErrorProne at compile-time, but it's proven hard.
2. Quite possibly no one's super motivated by required fields given they're highly discouraged in proto2 and don't exist in proto3. :)

I'd suggest writing a plugin for your own purposes if you have protos where it's worth hardcoding which fields are required, and worth the hassle of maintaining your own plugin. Your implementation looks good modulo some over-matching (you could implement isProtobufBuilder as a type check against MessageLite.Builder, for example), and it'll only work for chained builder expressions.