Rlc collections redesign #7166
Rlc collections redesign #7166
Conversation
…ollections and collection ownership checker
…-framework into rlc-collections-redesign
…-framework into rlc-collections-redesign
There was a problem hiding this comment.
This is a partial review. I read the whole manual section and most of the code for the checker itself, although I didn't read all of the changes to MustCallConsistencyAnalyzer or ResourceLeakUtils carefully for lack of time (I'll come back to them later). I also read the test cases for the new checker. Overall, the design seems sensible to me.
I will review the rest of this PR, including the changes to the rest of the CF, later.
| \item Methods that call 'unsafe' methods creating obligations for the resource collection field, such as \texttt{socketList.add(Socket)} in this case, must have a \texttt{@CreatesMustCallFor(``this'')} annotation. | ||
| \end{enumerate} | ||
|
|
||
| This now shifts the burden to the client of the \texttt{Aggregator} class to call \texttt{close()} on instances before they leave scope. |
There was a problem hiding this comment.
This text looks out of place, since it refers to an upcoming example. Should it be deleted or moved?
| @DefaultQualifierInHierarchy | ||
| @DefaultFor({ | ||
| // TypeUseLocation.ALL, | ||
| // TypeUseLocation.CONSTRUCTOR_RESULT, |
There was a problem hiding this comment.
It's odd to see these commented-out, rather than just not present. A comment here explaining why these specific defaults were chosen seems like a good idea.
| * Returns true if the given type is a resource collection: a type assignable from {@code | ||
| * Collection} whose single type var has non-empty MustCall type. | ||
| * | ||
| * <p>This overload should be used before computation of AnnotatedTypeMirrors is completed, in |
There was a problem hiding this comment.
Does this should mean "should only" or "must"? I.e., is it only okay to use it in that context, or is it an error to use something else in that context?
| default: | ||
| } | ||
| } | ||
| // boolean assignmentOfOwningCollectionArrayElement = |
There was a problem hiding this comment.
Why is this code commented out? I assume if its not actually running here then it's not needed; in that case, why not delete it?
|
|
||
| /** | ||
| * Records, in the {@code @CollectionOwnershipAnnotatedTypeFactory}, loops that call a method on | ||
| * entries of an {@code @OwningCollection}. |
There was a problem hiding this comment.
Why does this need to happen in the MC Visitor? This seems like an unnatural place for it, but I presume there is a reason, which should be recorded here. Maybe this has to be complete before the CO checker starts running, for some reason?
| @NotOwningCollection | ||
| Collection<Socket> checkLegalNotOwningReturn() { | ||
| List<Socket> list = new ArrayList<>(); | ||
| closeElements(list); |
There was a problem hiding this comment.
This test case seems overly restrictive, based on my understanding of the type system's rules. In fact, you could have two tests here:
@NotOwningCollection
Collection<Socket> checkLegalNotOwningReturn1() {
List<Socket> l = new ArrayList<>();
l.add(new Socket());
closeElements(l);
return l;
}
@NotOwningCollection
Collection<Socket> checkLegalNotOwningReturn2() {
List<Socket> l = new ArrayList<>();
return l; // OK, because `l` is empty and so should be @OwningCollectionNoObligations
}
| @@ -13,26 +14,34 @@ public void test1(List<@MustCall({}) Socket> l) { | |||
| l.add(s); | |||
| } | |||
|
|
|||
| // :: error: type.argument | |||
| public void test2(List<Socket> l) { | |||
| public List<Socket> test2(@OwningCollection List<Socket> l) { | |||
| // s is unconnected, so no error is expected when it's stored into the list. | |||
| // But, if the list is unannotated, we do get an error at its declaration site | |||
| // (as expected, due to #5912). | |||
There was a problem hiding this comment.
Is this comment still accurate with the collections extension? (I don't think that it is.)
| @@ -901,7 +901,7 @@ protected void addLabelForNextNode(Label l) { | |||
| /* --------------------------------------------------------- */ | |||
|
|
|||
| /** The UID for the next unique name. */ | |||
| protected long uid = 0; | |||
| protected static long uid = 0; | |||
There was a problem hiding this comment.
What's the reasoning for this change? On its surface, this seems like a bad idea: why would different CFGs need to share a uid counter?
Extend RLC to collections of resources.