RLC: warn when @MustCallAlias is written on a return type but no parameter type, and vice versa

[msridhar]

I don't think that we have any special handling for this case. Because @MustCallAlias is a poly annotation, we do get warnings sometimes (if it is placed in a way that is inconsistent with the general poly rules), but (as is clear from this example) not always.

I agree that it would be good for the RLC to warn if there is an @MustCallAlias annotation on a parameter but not a return and vice-versa, because that's the only reasonable way to use the annotation.

Originally posted by @kelloggm in #6375 (comment)

[jyoo980]

I can take this on if it's not too critical for the work being done in the RLC.

It should be a matter of making sure we catch cases like the ones below, right?

Cases to NOT issue a warning:

@MustCallAlias T foo(@MustCallAlias U bar) // no error
T foo(U bar) // no error

Cases to issue a warning:

T foo(@MustCallAlias U bar) // @MustCallAlias missing on return type
@MustCallAlias T foo(U bar) // @MustCallAlias missing on param type

Can we reduce this to saying: if we see a @MustCallAlias in the signature of a method, it should occur on the return type and at least one of the parameter types?

[msridhar]

Thanks @jyoo980! I think your understanding is correct, and there is nothing critical / urgent here. I would expect we could add this check by overriding visitMethod in MustCallVisitor, or something like that.

[kelloggm]

Thanks @jyoo980 for taking this on!

Can we reduce this to saying: if we see a @MustCallAlias in the signature of a method, it should occur on the return type and at least one of the parameter types?

There are (at least) two additional complications that you'll also need to address:

• this rule should also apply to constructors
• its also ok to have an @MustCallAlias annotation on the receiver parameter (assuming there is one on the return type), so naively checking the parameter list won't be sufficient

[jyoo980]

Thanks for the additional context.

I had a question about a test case:

  public static @MustCallAlias MustCallAliasMethodReturnAndParam mcaneFactory(@MustCallAlias InputStream is)
      throws Exception {
    return new MustCallAliasMethodReturnAndParam(is, false);
  }

For the method above, I would expect the return type to be @MustCallAlias (as annotated) and the parameter type to be @MustCallAlias. However, I am seeing that the result of

AnnotatedTypeMirror returnType = atypeFactory.getMethodReturnType(tree);

Where the tree is the MethodTree of mcaneFactory is in fact @PolyMustCall, which is confusing to me. Is there a reason for this?

[kelloggm]

Where the tree is the MethodTree of mcaneFactory is in fact @PolyMustCall, which is confusing to me. Is there a reason for this

Yes. Under the hood, the Must Call Checker treats @MustCallAlias as an alias of @PolyMustCall, because the rules for @MustCallAlias and polymorphic qualifiers are very similar from its perspective. In particular, the Must Call type of an @MustCallAlias parameter should be treated polymorphically with 1) any other @MustCallAlias parameters and 2) the return type. This was an implementation shortcut to avoid needing to write that logic from scratch, but it's worked reasonably well for us so far.

The Resource Leak Checker handles all the @MustCallAlias-specific rules, IIRC.

[jyoo980]

I see, I've been working in the Must Call Visitor for my current work, maybe I should put my logic in the RLC.

…

On Tue, Dec 19, 2023 at 12:51 PM Martin Kellogg ***@***.***> wrote: Where the tree is the MethodTree of mcaneFactory is in fact @PolyMustCall, which is confusing to me. Is there a reason for this Yes. Under the hood, the Must Call Checker treats @MustCallAlias as an alias of @PolyMustCall, because the rules for @MustCallAlias and polymorphic qualifiers are very similar from its perspective. In particular, the Must Call type of an @MustCallAlias parameter should be treated polymorphically with 1) any other @MustCallAlias parameters and 2) the return type. This was an implementation shortcut to avoid needing to write that logic from scratch, but it's worked reasonably well for us so far. The Resource Leak Checker handles all the @MustCallAlias-specific rules, IIRC. — Reply to this email directly, view it on GitHub <#6376 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFZ3EECAFRU7M43SN4MHQGTYKH46HAVCNFSM6AAAAABAZ5GWG6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNRTGQ2TIOJWGM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[jyoo980]

@kelloggm Thanks for the explanation. I have one more question:

its also ok to have an @MustCallAlias annotation on the receiver parameter (assuming there is one on the return type), so naively checking the parameter list won't be sufficient

Could you give me an example of what this means? The draft PR I have open here currently handles constructors and regular method calls, I think.

[kelloggm]

Could you give me an example of what this means? The #6377 I have open here currently handles constructors and regular method calls, I think.

I looked for a test case that shows how this works in the RLC, and we don't appear to have one. So maybe you can safely ignore this situation. I was thinking of something like:

@MustCallAlias Socket openSocket(@MustCallAlias SocketContainer this) { 
 // open a socket, store it in an @Owning field of the current class, and then return it
}

In theory something like that should be verifiable with the RLC, but as I said I can't find a test case - so probably you don't need to worry about that case. Sorry for the confusion!

[msridhar]

I feel like the following method should be verifiable by the RLC:

@MustCallAlias Foo somethingFluent(@MustCallAlias Foo this) {
  [...]
  return this;
}

But, I don't think we've ever tested it before, and I agree we can skip testing of it for now.

[jyoo980]

It looks like there are cases in the E2E tests for the Checker Framework that do test the this parameter, like here, I'll have to account for the case.

[jyoo980]

Relevant test case added in this PR which is also ready for review.