Skip to content

Handle class literal CFG nodes #52

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

Open
aosen-xiong wants to merge 2 commits into
base: main-eisop
Choose a base branch
from
Open

Handle class literal CFG nodes #52

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a87e6b3
Handle class literal CFG nodes
aosen-xiong Jun 20, 2026
852ef3a
Handle class literals in dataflow transfer
aosen-xiong Jun 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 61 additions & 52 deletions src/main/java/com/google/jspecify/nullness/NullSpecTransfer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import static org.checkerframework.javacutil.TreeUtils.elementFromDeclaration;
import static org.checkerframework.javacutil.TreeUtils.elementFromTree;
import static org.checkerframework.javacutil.TreeUtils.elementFromUse;
import static org.checkerframework.javacutil.TreeUtils.isClassLiteral;
import static org.checkerframework.javacutil.TreeUtils.typeOf;

import com.sun.source.tree.EnhancedForLoopTree;
Expand Down Expand Up @@ -91,62 +92,74 @@ final class NullSpecTransfer extends CFAbstractTransfer<CFValue, NullSpecStore,
unionNull = util.unionNull;
}

@Override
public TransferResult<CFValue, NullSpecStore> visitNode(
Node node, TransferInput<CFValue, NullSpecStore> input) {
TransferResult<CFValue, NullSpecStore> result = super.visitNode(node, input);
if (isClassLiteral(node.getTree())) {
handleClassLiteral(result);
}
return result;
}

@Override
public TransferResult<CFValue, NullSpecStore> visitFieldAccess(
FieldAccessNode node, TransferInput<CFValue, NullSpecStore> input) {
TransferResult<CFValue, NullSpecStore> result = super.visitFieldAccess(node, input);
if (node.getFieldName().equals("class")) {

@aosen-xiong aosen-xiong Jun 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

We should decide whether we need this special case and long comment if this is handled as a separate ClassLiteralNode.

/*
* TODO(cpovirk): Move our "non-dataflow" special cases out of NullSpecTransfer and into our
* TreeAnnotator.
*
* *Or*: Would an even *better* option be to override a method like fromElement or
* getAnnotatedType? While methods like TreeAnnotator.visitMethodInvocation handle specific
* kinds of usages (like, well, "method invocations"), a method like fromElement or
* getAnnotatedType could handle all kinds of usages (including method references). Or maybe
* that's bad: When we analyze a method implementation in com.google.common itself, we
* probably don't want the checker to ignore the annotations that we've written on, e.g., its
* return type in favor of the "pretend" annotations that we apply for users here. On top of
* all that, another potentially tricky bit is to ensure that we have access to all the
* information we need. In particular, we sometimes need a Tree, not just an Element, such as
* when we look for calls to Throwable.getCause() that happens specifically on an instance of
* ExecutionException.
*
* *Or*: Would we prefer to just define stub files, since we're essentially recreating them
* here? However, we've seen slowness from using -Astubs (perhaps solvable by building the
* stub files into the Checker Framework itself, similar to the JDK stubs), *and* we've seen
* crashes (at least with our custom checker) when we analyze a codebase that we supply stubs
* for. Even if that doesn't work, though, maybe we should look to hook into CF in similar
* places to where the stub-file logic appears?
*
* Anyway, TreeAnnotator (or any other approach discussed above) offers advantages over
* NullSpecTransfer:
*
* - It runs in cases in which NullSpecTransfer (somewhat mysteriously) does not. See
* https://github.com/jspecify/jspecify/blob/ef834e47eb83cdf3fb912d07705aa40808584a6a/samples/GetCauseNonNull.java#L46
*
* - TreeAnnotator lets us change the nullness of a _non-top-level_ type. Currently, we do
* this to change the element type of a Stream when we know that it is non-nullable. (Aside:
* Another piece of that logic -- well, a somewhat different piece of logic with a similar
* purpose -- lives in checkMethodReferenceAsOverride. So that logic is already split across
* files.)
*
* - I haven't measured performance, but I would assume that anything that runs during
* dataflow is not going to be *faster* :) Perhaps more importantly, if we ever try to
* improve performance by skipping dataflow in cases in which we can prove what we need
* without it (as NullAway does), then we'll want to move our handling for as many simple
* cases as we can to run it outside of dataflow.
*
* - Hypothetically, TreeAnnotator would make more sense if we needed to change types that
* appear in "non-dataflow" locations -- perhaps if we needed to change the types of a
* method's parameters or return type before overload checking occurs? But I don't know that
* we'll need to do that.
*/
setResultValueToNonNull(result);
handleClassLiteral(result);
}
return result;
}

private void handleClassLiteral(TransferResult<CFValue, NullSpecStore> result) {
/*
* TODO(cpovirk): Move our "non-dataflow" special cases out of NullSpecTransfer and into our
* TreeAnnotator.
*
* *Or*: Would an even *better* option be to override a method like fromElement or
* getAnnotatedType? While methods like TreeAnnotator.visitMethodInvocation handle specific
* kinds of usages (like, well, "method invocations"), a method like fromElement or
* getAnnotatedType could handle all kinds of usages (including method references). Or maybe
* that's bad: When we analyze a method implementation in com.google.common itself, we probably
* don't want the checker to ignore the annotations that we've written on, e.g., its return type
* in favor of the "pretend" annotations that we apply for users here. On top of all that,
* another potentially tricky bit is to ensure that we have access to all the information we
* need. In particular, we sometimes need a Tree, not just an Element, such as when we look for
* calls to Throwable.getCause() that happens specifically on an instance of ExecutionException.
*
* *Or*: Would we prefer to just define stub files, since we're essentially recreating them here?
* However, we've seen slowness from using -Astubs (perhaps solvable by building the stub files
* into the Checker Framework itself, similar to the JDK stubs), *and* we've seen crashes (at
* least with our custom checker) when we analyze a codebase that we supply stubs for. Even if
* that doesn't work, though, maybe we should look to hook into CF in similar places to where the
* stub-file logic appears?
*
* Anyway, TreeAnnotator (or any other approach discussed above) offers advantages over
* NullSpecTransfer:
*
* - It runs in cases in which NullSpecTransfer (somewhat mysteriously) does not. See
* https://github.com/jspecify/jspecify/blob/ef834e47eb83cdf3fb912d07705aa40808584a6a/samples/GetCauseNonNull.java#L46
*
* - TreeAnnotator lets us change the nullness of a _non-top-level_ type. Currently, we do this
* to change the element type of a Stream when we know that it is non-nullable. (Aside: Another
* piece of that logic -- well, a somewhat different piece of logic with a similar purpose --
* lives in checkMethodReferenceAsOverride. So that logic is already split across files.)
*
* - I haven't measured performance, but I would assume that anything that runs during dataflow
* is not going to be *faster* :) Perhaps more importantly, if we ever try to improve
* performance by skipping dataflow in cases in which we can prove what we need without it (as
* NullAway does), then we'll want to move our handling for as many simple cases as we can to
* run it outside of dataflow.
*
* - Hypothetically, TreeAnnotator would make more sense if we needed to change types that appear
* in "non-dataflow" locations -- perhaps if we needed to change the types of a method's
* parameters or return type before overload checking occurs? But I don't know that we'll need
* to do that.
*/
setResultValueToNonNull(result);
}

@Override
public TransferResult<CFValue, NullSpecStore> visitMethodInvocation(
MethodInvocationNode node, TransferInput<CFValue, NullSpecStore> input) {
Expand Down Expand Up @@ -723,11 +736,7 @@ private boolean isGetClassLoaderClassLiteral(MethodInvocationNode node) {

private boolean isOnClassLiteral(MethodInvocationNode node) {
Node receiver = node.getTarget().getReceiver();
if (!(receiver instanceof FieldAccessNode)) {
return false;
}
FieldAccessNode fieldAccess = (FieldAccessNode) receiver;
return fieldAccess.getFieldName().equals("class");
return isClassLiteral(receiver.getTree());
}

private boolean isGetClassLoaderClassOnThisGetClass(MethodInvocationNode node) {
Expand Down
Loading