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.

Sign up for GitHub

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

Jump to bottom

Support unique classes (Fixes #3313) #3336

Merged
merged 35 commits into from
Jul 21, 2020
Merged

Support unique classes (Fixes #3313) #3336

Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
fd536c4
Fix for feature request #3313 in Aliasing Checker
aditya3434 May 26, 2020
126014d
Fix for explicit annotation check in Aliasing Checker
aditya3434 May 28, 2020
b2b91eb
Adding test cases for removal of explicit annotation check
aditya3434 May 28, 2020
1b756c0
Ensuring all framework tests are passed
aditya3434 May 29, 2020
4edfc68
Formatting changes
aditya3434 May 29, 2020
dde03ef
Merge ../checker-framework-branch-master into 3313-explicit-annotatio…
mernst Jun 3, 2020
22462e0
Fix for feature request #3313 in Aliasing Checker
aditya3434 May 26, 2020
017082b
Revert "Fix for feature request #3313 in Aliasing Checker"
aditya3434 May 28, 2020
4b36609
Merge branch 'master' into 3313-explicit-annotation-fix
aditya3434 Jun 6, 2020
fa03246
Merge branch '3313-explicit-annotation-fix' of https://github.com/adi…
aditya3434 Jun 6, 2020
97ff12d
Fix for feature request #3313 in Aliasing Checker
aditya3434 May 26, 2020
629a1a9
Revert "Fix for feature request #3313 in Aliasing Checker"
aditya3434 May 28, 2020
25b602a
Merge remote-tracking branch 'upstream/master'
aditya3434 Jun 16, 2020
91d8372
Merge branch 'master' into 3313-explicit-annotation-fix
aditya3434 Jun 16, 2020
eb424fd
Fix for feature request #3313 in Aliasing Checker
aditya3434 May 26, 2020
c324094
Revert "Fix for feature request #3313 in Aliasing Checker"
aditya3434 May 28, 2020
941cab0
Fix for feature request #3313 in Aliasing Checker
aditya3434 May 26, 2020
d71f608
Revert "Fix for feature request #3313 in Aliasing Checker"
aditya3434 May 28, 2020
53a6c12
Merge remote-tracking branch 'upstream/master'
aditya3434 Jun 19, 2020
ff1b9e4
Merge branch 'master' into 3313-explicit-annotation-fix
aditya3434 Jun 19, 2020
135b266
Resolving review comments
aditya3434 Jun 19, 2020
db149b9
Removing redundant variables
aditya3434 Jun 19, 2020
e64f002
Modifying documentation
aditya3434 Jun 19, 2020
167d9cc
Merge branch 'master' into 3313-explicit-annotation-fix
aditya3434 Jun 27, 2020
cf45fd6
Making requested changes
aditya3434 Jun 27, 2020
f3d2479
Modifying documentation
aditya3434 Jun 27, 2020
9b60d82
Deleting stubfile.astub
aditya3434 Jun 28, 2020
1335ba2
Removing stubfile.astub references
aditya3434 Jun 28, 2020
1da4103
Merge branch 'master' into 3313-explicit-annotation-fix
aditya3434 Jul 11, 2020
7b191e9
Merge branch 'master' into 3313-explicit-annotation-fix
aditya3434 Jul 13, 2020
bd1fe05
Merge branch 'master' into 3313-explicit-annotation-fix
aditya3434 Jul 15, 2020
37976e8
Merge branch 'master' into 3313-explicit-annotation-fix
aditya3434 Jul 17, 2020
1523897
Merge branch 'master' into 3313-explicit-annotation-fix
aditya3434 Jul 18, 2020
79f35f1
Merge branch 'master' into 3313-explicit-annotation-fix
aditya3434 Jul 21, 2020
f6e67cc
Making requested changes
aditya3434 Jul 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
44 changes: 34 additions & 10 deletions framework/src/main/java/org/checkerframework/common/aliasing/AliasingVisitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,11 @@
package org.checkerframework.common.aliasing;

import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.MethodInvocationTree;
import com.sun.source.tree.MethodTree;
import com.sun.source.tree.NewArrayTree;
import com.sun.source.tree.ThrowTree;
import com.sun.source.tree.Tree;
import com.sun.source.tree.*;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not star import.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of star import, I listed the library data types that were required in my latest commit

import com.sun.source.tree.Tree.Kind;
import com.sun.source.tree.VariableTree;
import com.sun.source.util.TreePath;
import java.util.List;
import javax.lang.model.element.ExecutableElement;
import javax.lang.model.element.VariableElement;
import java.util.Set;
import javax.lang.model.element.*;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not use wildcard imports.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had this before: #3336 (comment)
Look how to change your IDE configuration to not make such changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have changed the import statement in my latest commit. Turns out my IDE automatically * imports whenever it imports more than 3 packages.

import org.checkerframework.checker.compilermsgs.qual.CompilerMessageKey;
import org.checkerframework.common.aliasing.qual.LeakedToResult;
import org.checkerframework.common.aliasing.qual.NonLeaked;
Expand Down Expand Up @@ -271,7 +265,37 @@ private boolean canBeLeaked(Tree exp) {
AnnotatedTypeMirror type = atypeFactory.getAnnotatedType(exp);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should also update the javadoc with the change in behavior.
Do you understand what canBeLeaked expresses? The documentation is rather unclear.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have changed the documentation for the canBeLeaked method in my latest commit

boolean isMethodInvocation = exp.getKind() == Kind.METHOD_INVOCATION;
boolean isNewClass = exp.getKind() == Kind.NEW_CLASS;
return type.hasExplicitAnnotation(Unique.class) && !isMethodInvocation && !isNewClass;
boolean isUniqueClassFlag = isUniqueClass(exp);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the Flag part. Notice the style for the other is variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have removed the flag part from the variable name in my latest commit

if (isUniqueClassFlag) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this just be return !isMethodInvocation && !isNewClass && (isUniqueClass || type.hasExplicitAnnotation(Unique.class));

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe extract the (isUniqueClass || type.hasExplicitAnnotation(Unique.class) into a separate local and document why that is the logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have separated the (isUniqueClass || type.hasExplicitAnnotation(Unique.class) into a separate local variable and documented the logic.

return type.hasAnnotation(Unique.class) && !isMethodInvocation && !isNewClass;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need the type.hasAnnotation again, if you already know that isUniqueClass?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I see that was a redundant check. I have not used it in my latest commits

} else {
return type.hasExplicitAnnotation(Unique.class) && !isMethodInvocation && !isNewClass;
}
}

/**
* Returns true if class of tree expression {@code exp} has type {@code @Unique}
*
* @param exp the Tree to check
* @return boolean true if class if unique and false otherwise
*/
private boolean isUniqueClass(Tree exp) {
AnnotatedTypeMirror type = atypeFactory.getAnnotatedType(exp);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The caller already computed type and you don't seem to use exp in this method. So why not just pass the type as parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I replaced exp with type as an argument in the isUniqueClass in my latest commit

Element el = types.asElement(type.getUnderlyingType());
if (el != null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be easier as if (el == null) { return false; } and then the same for annoMirrors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have separated out el and annoMirrors checks instead of the nested if conditions. If either of them is null, the method returns false

Set<AnnotationMirror> annoMirrors = atypeFactory.getDeclAnnotations(el);
if (annoMirrors != null) {
for (AnnotationMirror mirror : annoMirrors) {
if (mirror.getAnnotationType()
.asElement()
.getSimpleName()
.contentEquals("Unique")) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, don't use Strings for things like this. Can you use AnnotationUtils.containsSameByClass https://github.com/typetools/checker-framework/blob/master/javacutil/src/main/java/org/checkerframework/javacutil/AnnotationUtils.java#L230 ? Then you don't need the for loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have used the AnnotationUtils.containtsSameByClass method in my latest commit

return true;
}
}
}
}
return false;
}

private boolean isInUniqueConstructor() {
Expand Down
13 changes: 13 additions & 0 deletions framework/tests/aliasing/ExplicitAnnotationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import org.checkerframework.common.aliasing.qual.Unique;

@Unique class Data {
@SuppressWarnings("unique.leaked")
Data() {} // All objects of Data are now @Unique
}

class Demo {
void check(Data p) { // p is @Unique Data Object
// :: error: (unique.leaked)
Data y = p; // @Unique p is leaked
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the type of the left-hand side important? Can you add another method that does

Object check2(Data p) { return p; }

which I guess should also give a unique.leaked error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the type of the left-hand side isn't important. However, the method doesn't give an error, even when Data is explicitly annotated as @unique.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how you mean this. Does Object z = p; not raise an error? Shouldn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

@Unique
class Data {

    Object check2(Data p) { return p; }

    void check(Data p) {
        Object z = p;
    }
}

In the code given above, Object z = p; does raise an error. Hence, the left hand side isn't important. However, the check2 function (where you are returning a @unique Data as an Object) doesn't raise an error both in the typetools/master and in my modified code, even if the Data p is explicitly annotated as @unique Data p.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, can you expand this test case with the Object local variable and the expected error.
Then file an issue that shows the problem with the return version.

}
}