Skip to content

Commit

Permalink
Correct the type of "this" (#3659)
Browse files Browse the repository at this point in the history
This pull request corrects the type of both implicit and explicit `this` references by correctly computing the enclosing type of `this`.

This pull request fixes two kinds of bugs: 
1. The type of `Outer.this` when used in an inner class.   (Issues #352, #2208, and #3561.)
2. For the Initialization Checker, the type of `Outer.this` when used in an anonymous class declared in a constructor. (Issues #354, part of #904, and #3408)

#409 (and other duplicate issues) is a related bug in the Initialization Checker, but isn't fixed by this PR.

Closes #352, closes #354, closes #2208, closes #3266, closes #3408 and closes #3561.
  • Loading branch information
smillst committed Sep 15, 2020
1 parent 9713802 commit 0d1de3b
Show file tree
Hide file tree
Showing 15 changed files with 264 additions and 307 deletions.
Expand Up @@ -420,41 +420,49 @@ public AnnotatedDeclaredType getSelfType(Tree tree) {
AnnotatedDeclaredType selfType = super.getSelfType(tree);

TreePath path = getPath(tree);
Tree topLevelMember = findTopLevelClassMemberForTree(path);
if (topLevelMember != null) {
if (topLevelMember.getKind() != Kind.METHOD
|| TreeUtils.isConstructor((MethodTree) topLevelMember)) {

setSelfTypeInInitializationCode(tree, selfType, path);
AnnotatedDeclaredType enclosing = selfType;
while (path != null && enclosing != null) {
TreePath topLevelMemberPath = findTopLevelClassMemberForTree(path);
if (topLevelMemberPath != null && topLevelMemberPath.getLeaf() != null) {
Tree topLevelMember = topLevelMemberPath.getLeaf();
if (topLevelMember.getKind() != Kind.METHOD
|| TreeUtils.isConstructor((MethodTree) topLevelMember)) {
setSelfTypeInInitializationCode(tree, enclosing, topLevelMemberPath);
}
path = topLevelMemberPath.getParentPath();
enclosing = enclosing.getEnclosingType();
} else {
break;
}
}

return selfType;
}

/**
* In the first enclosing class, find the top-level member that contains {@code path}.
*
* <p>TODO: should we look whether these elements are enclosed within another class that is
* itself under construction.
*
* <p>Are there any other type of top level objects?
* In the first enclosing class, find the path to the top-level member that contains {@code
* path}.
*
* @param path the path whose leaf is the target
* @return a top-level member containing the leaf of {@code path}
* @return path to a top-level member containing the leaf of {@code path}
*/
@SuppressWarnings("interning:not.interned") // AST node comparison
private Tree findTopLevelClassMemberForTree(TreePath path) {
private TreePath findTopLevelClassMemberForTree(TreePath path) {
if (TreeUtils.isClassTree(path.getLeaf())) {
path = path.getParentPath();
if (path == null) {
return null;
}
}
ClassTree enclosingClass = TreeUtils.enclosingClass(path);
if (enclosingClass != null) {

List<? extends Tree> classMembers = enclosingClass.getMembers();
TreePath searchPath = path;
while (searchPath.getParentPath() != null
&& searchPath.getParentPath().getLeaf() != enclosingClass) {
searchPath = searchPath.getParentPath();
if (classMembers.contains(searchPath.getLeaf())) {
return searchPath.getLeaf();
return searchPath;
}
}
}
Expand Down
Expand Up @@ -1096,8 +1096,9 @@ public Void visitIdentifier(IdentifierTree tree, Void p) {
Tree parent = getCurrentPath().getParentPath().getLeaf();
// If the parent is not a member select, or if it is and the field is the expression,
// then the field is accessed via an implicit this.
if (parent.getKind() != Kind.MEMBER_SELECT
|| ((MemberSelectTree) parent).getExpression() == tree) {
if ((parent.getKind() != Kind.MEMBER_SELECT
|| ((MemberSelectTree) parent).getExpression() == tree)
&& !ElementUtils.isStatic(TreeUtils.elementFromUse(tree))) {
AnnotationMirror guardedBy =
atypeFactory
.getSelfType(tree)
Expand Down
19 changes: 19 additions & 0 deletions checker/tests/nullness/Issue354.java
@@ -0,0 +1,19 @@
class Issue354 {

String a;

{
Object o =
new Object() {
@Override
public String toString() {
// :: error: (dereference.of.nullable)
return a.toString();
}
}.toString();

// This is needed to avoid the initialization.fields.uninitialized warning.
// The NPE still occurs
a = "";
}
}
1 change: 1 addition & 0 deletions checker/tests/nullness/ThisLiteral.java
Expand Up @@ -9,6 +9,7 @@ void test() {
// :: error: (assignment.type.incompatible)
@Initialized ThisLiteral l2 = ThisLiteral.this;

// :: error: (method.invocation.invalid)
ThisLiteral.this.foo();
// :: error: (method.invocation.invalid)
foo();
Expand Down
2 changes: 0 additions & 2 deletions checker/tests/nullness/ThisLiteralQualified.java
@@ -1,8 +1,6 @@
// Test case for Issue #2208:
// https://github.com/typetools/checker-framework/issues/2208

// @skip-test until the issue is fixed

import org.checkerframework.checker.initialization.qual.UnderInitialization;

public class ThisLiteralQualified {
Expand Down
20 changes: 20 additions & 0 deletions checker/tests/nullness/init/Issue3407.java
@@ -0,0 +1,20 @@
// @below-java9-jdk-skip-test
class Issue3408 {
final String foo;

String getFoo() {
return foo;
}

Issue3408() {
var anon =
new Object() {
String bar() {
// :: error: (method.invocation.invalid)
return Issue3408.this.getFoo().substring(1);
}
};
anon.bar(); // / WHOOPS... NPE, `getFoo()` returns `foo` which is still null
this.foo = "Hello world";
}
}
2 changes: 0 additions & 2 deletions checker/tests/nullness/init/Issue904.java
@@ -1,8 +1,6 @@
// Test case for Issue 904:
// https://github.com/typetools/checker-framework/issues/904

// @skip-test

public class Issue904 {
final Object mBar;
final Runnable mRunnable =
Expand Down
1 change: 0 additions & 1 deletion checker/tests/tainting/Issue352.java
@@ -1,4 +1,3 @@
// @skip-test
// Test case for Issue 352:
// https://github.com/typetools/checker-framework/issues/352

Expand Down
16 changes: 16 additions & 0 deletions checker/tests/tainting/Issue3561.java
@@ -0,0 +1,16 @@
import org.checkerframework.checker.tainting.qual.*;

public class Issue3561 {
void outerMethod(@Untainted Issue3561 this) {}

class Inner {
void innerMethod(@Untainted Issue3561.@Untainted Inner this) {
Issue3561.this.outerMethod();
}

void innerMethod2(@Tainted Issue3561.@Untainted Inner this) {
// :: error: (method.invocation.invalid)
Issue3561.this.outerMethod();
}
}
}
Expand Up @@ -444,14 +444,15 @@ private List<BaseTypeChecker> instantiateSubcheckers(
return Collections.unmodifiableList(immediateSubcheckers);
}

/*
* Get the list of all subcheckers (if any). via the instantiateSubcheckers method.
* This list is only non-empty for the one checker that runs all other subcheckers.
* These are recursively instantiated via instantiateSubcheckers the first time
* the method is called if subcheckers is null.
* Assumes all checkers run on the same thread.
/**
* Get the list of all subcheckers (if any). via the instantiateSubcheckers method. This list is
* only non-empty for the one checker that runs all other subcheckers. These are recursively
* instantiated via instantiateSubcheckers the first time the method is called if subcheckers is
* null. Assumes all checkers run on the same thread.
*
* @return the list of all subcheckers (if any)
*/
private List<BaseTypeChecker> getSubcheckers() {
public List<BaseTypeChecker> getSubcheckers() {
if (subcheckers == null) {
// Instantiate the checkers this one depends on, if any.
LinkedHashMap<Class<? extends BaseTypeChecker>, BaseTypeChecker> checkerMap =
Expand Down

0 comments on commit 0d1de3b

Please sign in to comment.