Nothing in the build catches a new `ASTHelpers.getSymbol` call that misses a parenthesized operand

Open
  • Is something wrong?
#2,133 1 comment 0 reactions 0 assignees View on GitHub

Maintainers usually reply within 1 day

Nobody has claimed this yet.

Assessment

Difficulty
4/5
Estimated time
3-5 days
Newbie friendliness
47/100
Issue type
Feature
Clarity
Clearly specified
Activity status
Active
Tech stack
java
Domain
build-system, devtools

Research direction

Read the acceptance criterion and the existing ASTHelpers.getSymbol call sites in nullaway/src/main/java, especially the two fixes in #2010 and #2002. Choose a build enforcement mechanism, then review each call site individually because some callers rely on the original tree shape. Done means a new ASTHelpers.getSymbol(Tree) call under nullaway/src/main fails the build with a message naming the required helper, while the allowed overloads still work.

Written by the indexing model from the issue text.

Description

ASTHelpers.getSymbol returns null for a ParenthesizedTree, so a NullAway check that resolves the symbol of an operand the user wrote stops seeing it once the user adds redundant parentheses. The metamorphic oracle of #2009 found two such call sites, and each got its own fix:

  • #2010: GrpcHandler reported passing @Nullable parameter 'h.get(K)' where @NonNull is required for if (h.containsKey((K))) { take(h.get(K)); }.
  • #2002: StreamNullabilityPropagator did not treat filter((Objects::nonNull)) as null-rejecting in JSpecify mode.

Both sites now call ASTHelpers.getSymbol(ASTHelpers.stripParentheses(...)). The other 138 ASTHelpers.getSymbol calls in nullaway/src/main/java pass the tree as it is. Many of them get their tree from a dataflow node or a declaration, which carries no parentheses, but nothing records which ones, and nothing makes the author of a new call decide.

Error Prone declined to make getSymbol look through parentheses (https://github.com/google/error-prone/issues/6223#issuecomment-6629111350). Its reason holds inside NullAway too: code that reads getSymbol(tree) != null as proof that tree is an identifier, a member select, or an invocation then casts tree or passes it to ASTHelpers.getReceiver, which throws IllegalStateException for a ParenthesizedTree. So stripping parentheses inside one shared helper for every caller is not safe either; each call site needs a decision.

Acceptance criterion

A call to ASTHelpers.getSymbol(Tree) added to nullaway/src/main fails the build unless the author marks it, and the failure message names the helper to use instead.

One possible shape

This is a sketch, and the mechanism is yours to choose:

  1. Add two helpers to NullabilityUtil. One strips parentheses and returns the symbol of the stripped expression; its Javadoc says that a caller inspects the stripped tree and not the original. The other resolves the tree as written, for trees that come from dataflow nodes or declarations.
  2. Forbid the getSymbol(com.sun.source.tree.Tree) overload in nullaway/src/main with the forbidden-apis Gradle plugin (de.thetaphi.forbiddenapis). It checks bytecode, so a static import does not get past it. The overloads for ClassTree, MethodTree, VariableTree, MethodInvocationTree, NewClassTree, MemberReferenceTree, and PackageTree cannot receive a ParenthesizedTree and stay allowed.
  3. Move the existing calls to one helper or the other one call site at a time, not with search and replace, because of the hazard above.

This adds a build plugin and touches 31 files. A custom Error Prone check in buildWithNullAway would do the same job without a new plugin, at the cost of writing and testing the check.

Out of scope

#2002 also fixed handlers that tested instanceof on the tree as written, such as a parenthesized lambda passed to filter. Neither a forbidden-API rule nor a helper catches that pattern, so the #2009 oracle remains the check for it.

I can send the pull request if this direction works for you.

Dominant language
Java
Stars
4.1k
Forks
379
Avg merge
1d 2h
Merged PRs (30d)
106

Getting set up

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from uber/NullAway

All issues in uber/NullAway

Similar issues

More Java issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.