Nullness Checker: Manually scan expression and reference type in visitInstanceOf (#7341)

[Gaoyan1999]

Summary

This PR addresses issue #7341 by modifying visitInstanceOf in the NullnessVisitor.

Problem

In the NullnessVisitor, the existing visitInstanceOf only checks the type part of the instanceof expression,
but does not scan the expression itself. As a result, nullness issues in the expression could be missed.

Solution

• Not calling super.visitInstanceOf() because, as noted in the existing comment,
  the superclass BaseTypeVisitor would also check the type and trigger incorrect
  instanceof.unsafe warnings.
• Instead, I manually scan the expression (super.scan(tree.getExpression(), p))
  in NullnessVisitor to ensure proper nullness analysis without invoking the superclass logic.

References

• Issue: false negative when indexing an array that is followed by instanceof #7341

[coderabbitai]

📝 Walkthrough

Walkthrough

The pull request updates NullnessVisitor.visitInstanceOf to call super.scan on the instance expression before returning (instead of skipping superclass scanning), changing how the left-hand expression is analyzed for instanceof. It adds a new test checker/tests/nullness/Issue7341.java that uses a nullable Integer as an array index and exercises an unboxing/nullness path. It also adds "Daniel Gao" to docs/manual/contributors.tex.

Suggested reviewers

• smillst

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2f1ab28 and e816d13.

📒 Files selected for processing (3)

• checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java (1 hunks)
• checker/tests/nullness/Issue7341.java (1 hunks)
• docs/manual/contributors.tex (1 hunks)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)

• GitHub Check: typetools.checker-framework (misc_jdk25)
• GitHub Check: typetools.checker-framework (typecheck_part1_jdk25)
• GitHub Check: typetools.checker-framework (typecheck_part2_jdk25)
• GitHub Check: typetools.checker-framework (nonjunit_jdk25)
• GitHub Check: typetools.checker-framework (junit_jdk25)
• GitHub Check: typetools.checker-framework (inference_part1_jdk25)
• GitHub Check: typetools.checker-framework (inference_part2_jdk25)

🔇 Additional comments (3)

docs/manual/contributors.tex (1)

35-35: LGTM!

The contributor addition is correctly placed in alphabetical order.

checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java (2)

416-429: LGTM! Reference type validation is correct.

The code properly checks the reference type (after "instanceof") for nullness annotations and reports appropriate errors/warnings for @Nullable and redundant @NonNull annotations.

---

431-433: Good fix! Expression scanning now works correctly.

The implementation correctly addresses issue #7341 by manually scanning the expression part of the instanceof check. Calling super.scan(tree.getExpression(), p) ensures nullness issues in the expression (like the array access in the test case) are detected, while avoiding the incorrect instanceof.unsafe warnings that would result from calling super.visitInstanceOf(). The comment clearly explains the rationale.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 11a45eb and 3d8dc01.

📒 Files selected for processing (1)

• checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java (1 hunks)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)

• GitHub Check: typetools.checker-framework (misc_jdk25)
• GitHub Check: typetools.checker-framework (typecheck_part2_jdk25)
• GitHub Check: typetools.checker-framework (inference_part1_jdk25)
• GitHub Check: typetools.checker-framework (typecheck_part1_jdk25)
• GitHub Check: typetools.checker-framework (inference_part2_jdk25)
• GitHub Check: typetools.checker-framework (nonjunit_jdk25)
• GitHub Check: typetools.checker-framework (junit_jdk25)

🔇 Additional comments (1)

checker/src/main/java/org/checkerframework/checker/nullness/NullnessVisitor.java (1)

415-437: LGTM! The logic correctly addresses the issue.

The implementation properly fixes issue #7341 by manually scanning the expression for nullness issues while avoiding the incorrect instanceof.unsafe warning from the superclass. The control flow is sound:

1. super.scan(tree.getExpression(), p) ensures the expression is analyzed for nullness by dispatching to the appropriate visitor method
2. The reference type annotations are still checked (lines 423-433)
3. Returning null without calling super.visitInstanceOf() prevents the incorrect warning

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between af54b0e and c5f24c7.

📒 Files selected for processing (1)

• framework/tests/all-systems/Issue7341.java (1 hunks)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)

• GitHub Check: typetools.checker-framework (typecheck_part2_jdk25)
• GitHub Check: typetools.checker-framework (nonjunit_jdk25)
• GitHub Check: typetools.checker-framework (inference_part1_jdk25)
• GitHub Check: typetools.checker-framework (junit_jdk25)
• GitHub Check: typetools.checker-framework (misc_jdk25)
• GitHub Check: typetools.checker-framework (typecheck_part1_jdk25)
• GitHub Check: typetools.checker-framework (inference_part2_jdk25)

[smillst]

Thanks!

[Gaoyan1999]

Thanks!

No worries. Thanks for your reply and review! I noticed you enabled auto-merge yesterday, but the pipeline failed because it ran out of CPU resources on AWS. I’ve rerun the pipeline, and everything looks good to be merged now. @smillst

[mernst]

@Gaoyan1999 Please do not force-push to GitHub. Force-pushing has no benefit, since we squash-and-merge pull requests. Force-pushing has negative consequences, such as removing code review comments on any deleted commits. Thanks again for your contribution.

[Gaoyan1999]

@Gaoyan1999 Please do not force-push to GitHub. Force-pushing has no benefit, since we squash-and-merge pull requests. Force-pushing has negative consequences, such as removing code review comments on any deleted commits. Thanks again for your contribution.

Thanks for the explanation! I didn’t realize force-pushing would remove review comments. I’ll avoid doing that in the future. Appreciate your patience!