NettyChannelBuilder.build() flagged as @Internal API

[ericgribkoff]

I enabled the grpc-java-api-checker tool on the grpc-java repo's examples directory, and found that this line triggers an @Internal API usage error. This is because NettyChannelBuilder (and OkHttpChannelBuilder) extend io.grpc.internal.AbstractManagedChannelImplBuilder but do not override it's build() method, and according to the package-info.java for io.grpc.internal, everything in this package has the @Internal annotation.

I don't think we want this to trigger the error, although technically it is use of an @Internal API - but it's via the non-internal NettyChannelBuilder class, so it seems like it should be "allowed" by grpc-java-api-checker.

@jyane any thoughts? The ASTHelper class does have methods to check for the declaration of super methods. E.g., we could "solve" this particular problem with something like:

  /**
   * Returns the description if tree is annotated.
   */
  private Description match(Tree tree, VisitorState state) {
    Symbol symbol = ASTHelpers.getSymbol(tree);
    if (symbol == null) {
      return NO_MATCH;
    }
    if (symbol instanceof MethodSymbol) {
      MethodSymbol methodSymbol = (MethodSymbol) symbol;
      Optional<MethodSymbol> superSymbol = 
          ASTHelpers.findSuperMethod(methodSymbol, state.getTypes());
      if (superSymbol.isPresent()) {
        symbol = superSymbol.get();
      }
    }
    AnnotationMirror annotation = findAnnotatedApi(symbol);
    if (annotation == null) {
      return NO_MATCH;
    }
    return describe(tree, annotation);
  }

But this would need to also check if the direct method being invoked had an @Internal annotation, e.g., if NettyChannelBuilder.build() was explicitly annotated as @Internal then we'd probably still want to have the tool match this, even though the super method on ManagedChannelBuilder is not internal. (hasDirectAnnotationWithSimpleName would probably be helpful here)

[jyane]

@ericgribkoff Thanks!
I didn't think this pattern, It seems a bug.
I'm now busy so still have not dig into it, but hasDirectAnnotationWithSimpleName seems to be fine.

[jyane]

I've checked this again, now I understand what is the problem.
It seems not a bug.

In this case, detecting as a super method invocation is "natural" but we don't want this to trigger the error. It's the problem.
Changing the behavior of this tool seems to be a workaround, removing a package-info and re-assigning annotations to APIs would be better. However, it is quite difficult and will be costly.

[jyane]

@ericgribkoff
I've checked your codes but It didn't work. I think I've got something wrong :(
I said that hasDirectAnnotationWithSimpleName seems to be fine, but It didn't work. Because the tool detects as a super type's method invocation :(

I think it will happen not only method but also variable.
In Java, all methods and variables belong to class so both will match MemberSelectTree.

I've created a method as below:

  private boolean detectMemberSelectFromSubtype(MemberSelectTree tree, VisitorState state) {
    Type receiverType = ASTHelpers.getReceiverType(tree);
    Symbol owner = ASTHelpers.getSymbol(tree).owner;
    if (owner == null) {
      throw new IllegalStateException("In MemberSelectTree, symbol has an owner, expected");
    }
    return !ASTHelpers.isSameType(receiverType, owner.type, state);
  }

It works :)
The receiver's type will be SubType because the method will be called from SubType but the owner of this method is SuperType so it works.
But I am not an expert of AST, I might be wrong.

If you want to "solve" and this approach is acceptable and I have more time, I will create a PR.
But... it seems to be a workaround...

edit:
The receiver means:

SubType subType = new SubType();
// Receiver is subType
subType.foo();

[ericgribkoff]

@jyane I think the behavior we want is that methods cannot be "hidden" via an @Internal annotation on a subclass. So since ManagedChannelBuilder#build is not @Internal, the annotation on AbstractManagedChannelImpl has no effect on its override of the build() method.

I shouldn't have used ASTHelpers.findSuperMethod in the earlier code, as that returns the first override, whereas we really want the last override (the top-level method declaration). I switched to using ASTHelpers.findSuperMethods, and that seems to work - it also works if the original method declaration is in an interface. This is without trying to do anything with hasDirectAnnotationWithSimpleName, which is unnecessary if we don't allow subclasses to hide APIs. I also don't think we need to do any kind of workaround for member variables, so just doing this super-check for method invocations should be sufficient.

I don't quite follow how detectMemberSelectFromSubtype would be used here. But I'm also not an expert on AST, so if that seems preferable to ASTHelpers.findSuperMethod (just sent you a PR), then let me know. Thanks!

[ericgribkoff]

fixed by #14