Aliasing: test constructor declarations #3337
Conversation
| package java.lang; | ||
|
|
||
| class Object { | ||
| @Unique Object(); |
There was a problem hiding this comment.
Why is this annotation being added in a jdk.astub file rather than in the annotated JDK, https://github.com/typetools/jdk?
There was a problem hiding this comment.
I added an .astub file to limit my changes to the Aliasing Checker directory itself and not disturb the workings of the other checkers. However, I have made changes in the annotated JDK and the resulting code still fixes the issue. It passed through all the checker tests but gives a "build failed for target 'run-test'" error in the build_jdk11u test when using Azure pipeline (not sure why).
Pull request : typetools/jdk#52
There was a problem hiding this comment.
You should link related PRs from the PR descriptions, so that it is immediately clear that PRs need to be merged together.
You should remove this jdk.astub file from this PR now, as the changes from the related PR should be picked-up automatically.
Make the one suggested update in the jdk project and let's see whether the build just needs to re-run.
There was a problem hiding this comment.
I have removed the jdk.astub file from this PR and provided the link for the PR in description which needs to be merged together.
| @@ -0,0 +1,16 @@ | |||
| import org.checkerframework.common.aliasing.qual.Unique; | |||
|
|
|||
| class ParentUniqueClass { | |||
There was a problem hiding this comment.
How about adding a top-level class around these two classes that has the same name as the file?
There was a problem hiding this comment.
I have added a top-level class around these two classes (with the same name) in my latest commit.
|
|
||
| class ChildUniqueClass extends ParentUniqueClass { | ||
|
|
||
| @Unique ChildUniqueClass() { |
There was a problem hiding this comment.
Can you add another subclass that does raise an error?
There was a problem hiding this comment.
In my latest commit, the ChildUniqueClass will raise an error since the ParentClass is not @unique.
This reverts commit fd536c4.
| import org.checkerframework.common.aliasing.qual.Unique; | ||
|
|
||
| public class UniqueConstructorTest { | ||
| // No need for @SuppressWarnings() |
There was a problem hiding this comment.
That comment should be combined with the one in the body, where you give the reason why no warning is required.
There was a problem hiding this comment.
I have combined the comment with the one in the body.
| public class UniqueConstructorTest { | ||
| // No need for @SuppressWarnings() | ||
| @Unique UniqueConstructorTest() { | ||
| // Doesn't raise unique.leaked error since it's a top parent class (@Unique Object subclass |
There was a problem hiding this comment.
What does top parent class mean?
There was a problem hiding this comment.
The Parent class extends UniqueConstructorTest and the Child class extends the parent class. Hence, I referred to the UniqueConstructorTest class as the "top parent class" (not related to the Parent class in the code) in the inheritance hierarchy. Since, it explicitly doesn't extend any class, it extends java.lang.Object class by default.
| class ParentClass extends UniqueConstructorTest { | ||
|
|
||
| ParentClass() { | ||
| // Doesn't raise unique.leaked error since it's a non unique class with a unique parent |
There was a problem hiding this comment.
I have made the correction in the comment
wmdietl
changed the title
wmdietl
changed the title
|
@mernst Do you want to look at this again? We removed the stub file and this is now only a test case. |
|
Since typetools/jdk#52 is closed, its branch should be deleted. |
|
@aditya3434 Ping: Please delete the merged branch https://github.com/aditya3434/jdk/tree/3313-leak-in-constructor-error-fix. Then re-run CI (for example, by pulling master into this branch). |
|
@aditya3434 Third request: Please delete the merged branch https://github.com/aditya3434/jdk/tree/3313-leak-in-constructor-error-fix. Then re-run CI (for example, by pulling master into this branch). |
I apologize for the delay. I have deleted the merged jdk branch and re-run CI on the pull request. |
|
@smillst I agree that this didn't fix that issue. I've unlinked the issue. |
The Aliasing checker gives a "unique.leaked" error when the super() class of a @unique annotated class is @MaybeAliased. This is useful for subclasses whose unique objects can leak into parent class methods. Classes that don't inherit from other classes, inherit from Object class as default which is @MaybeAliased according to the Aliasing Checker, hence, their constructors give a redundant "unique.leaked" error. The solution is to annotate the Object class as @unique in the annotated JDK (https://github.com/typetools/jdk). This would prevent @unique parent classes to leak their objects in the super() function.
This PR should be merged along with the annotated JDK pull request.
(typetools/jdk#52)