Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

@Owning should be a type annotation #4934

Closed
mernst opened this issue Oct 19, 2021 · 1 comment · Fixed by #4989
Closed

@Owning should be a type annotation #4934

mernst opened this issue Oct 19, 2021 · 1 comment · Fixed by #4989
Assignees
Milestone

Comments

@mernst
Copy link
Member

mernst commented Oct 19, 2021

If a fix for #4932 is merged before a fix for this issue is merged, then some tests will need to be re-enabled.

org.checkerframework.checker.mustcall.qual.Owning is currently defined as a declaration annotation (not a type annotation):

@Retention(RetentionPolicy.RUNTIME)
@Target({ElementType.METHOD, ElementType.PARAMETER, ElementType.FIELD})
public @interface Owning {}

This ensures that @Owning is only written on methods, parameters, and fields. However, it is problematic for two reasons. First, its documentation indicates that it is about types rather than parameters. Second, in Java 17 a declaration annotation cannot be written on the this type parameter. Under Java 17, running the tests yields

org.checkerframework.checker.test.junit.MustCallTest > run[/home/mernst/research/types/checker-framework-fork-mernst-branch-jdk17/checker/mustcall] FAILED
    java.lang.AssertionError: 0 out of 40 expected diagnostics were found.
    1 unexpected diagnostic was found
    OwningParams.java:8: error: annotation @org.checkerframework.checker.mustcall.qual.Owning not applicable in this type context
@mernst mernst added this to the Critical milestone Oct 19, 2021
@kelloggm
Copy link
Contributor

it is problematic for two reasons. First, its documentation indicates that it is about types rather than parameters

I disagree with this statement (perhaps the issue is the documentation): an @Owning annotation indicates something about the declaration to which it is attached, not about the type. In particular, @Owning indicates that the annotated declaration is a starting point for resource leak checking. There is no ownership type system to which it belongs.

I think this is most clear if we view @Owning and @NotOwning as directives to the MustCallConsistencyAnalyzer about where to check that the rules of the Resource Leak Checker are followed. Placing either of these annotations merely transfers some checks from e.g. call sites of a method to the method body (when adding @Owning to a parameter); it does not change the type of any object in that checker's type systems.

Second, in Java 17 a declaration annotation cannot be written on the this type parameter

That writing @Owning on a this parameter was ever permitted was an error - it is non-sensical. An instance method of a class never "owns" responsibility for calling the must-call methods of that class: either the instance method that would be so annotated should actually be a must-call method itself, or an @EnsuresCalledMethods annotation with a value argument of this can be used instead. I will make a PR that removes support for this anti-pattern and the associated tests.

As an aside, I looked into how difficult it would be to make @Owning a type annotation rather than a declaration annotation, and concluded that it is in fact used exclusively in the manner of a declaration annotation: the Resource Leak Checker makes decisions about what sort of checks to do based on its presence on declarations. Logically, it isn't part of any type hierarchy, so I'm not sure how we would transition it to being a type annotation in a sensible way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants