Skip to content

Conversation

@smillst
Copy link
Member

@smillst smillst commented Sep 30, 2025

Even with this change, the Nullness Checker takes twice as long as the Index Checker and to check Issue6623.java.

Fixes #6623.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 30, 2025

📝 Walkthrough

Walkthrough

  • checker/src/main/java/org/checkerframework/checker/initialization/InitializationAnnotatedTypeFactory.java: In visitNewClass, the temporary toggling of the shouldCache flag around per-argument annotated type computation was removed. The method now directly obtains the annotated type for each argument without saving/restoring shouldCache. The logic computing allInitialized remains unchanged.
  • checker/tests/nullness/Issue6623.java: Removed two comments in a stream-based test case (Issue6623) with no code or behavioral changes.

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.
✅ Passed checks (2 passed)
Check name Status Explanation
Linked Issues Check ✅ Passed The changes in InitializationAnnotatedTypeFactory precisely remove the temporary disabling and restoring of the shouldCache flag around each constructor argument lookup, which directly reinstates the caching behavior that was inadvertently disabled in versions 3.43.x and 3.44.x, thereby addressing the severe nested‐stream performance regression described in issue #6623 without altering any nullness checking semantics. The minimal modifications align with the reproducer’s requirement to restore compilation times to approximately 1–2 seconds by eliminating only the per‐argument cache toggle. The ancillary comment removals in the test file are non‐functional and do not impact behavior or semantics.
Out of Scope Changes Check ✅ Passed All code changes are limited to reinstating caching in the InitializationAnnotatedTypeFactory and cleaning up comments in the nested streams test, both of which are directly related to fixing the performance regression in issue #6623; no unrelated functionality, configuration, or public API modifications have been introduced.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • 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 6bf93a9 and 3cb9764.

📒 Files selected for processing (2)
  • checker/src/main/java/org/checkerframework/checker/initialization/InitializationAnnotatedTypeFactory.java (1 hunks)
  • checker/tests/nullness/Issue6623.java (0 hunks)
💤 Files with no reviewable changes (1)
  • checker/tests/nullness/Issue6623.java
⏰ 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 (inference_part2_jdk25)
  • GitHub Check: typetools.checker-framework (typecheck_part2_jdk25)
  • GitHub Check: typetools.checker-framework (inference_part1_jdk25)
  • GitHub Check: typetools.checker-framework (junit_jdk25)
  • GitHub Check: typetools.checker-framework (typecheck_part1_jdk25)
  • GitHub Check: typetools.checker-framework (nonjunit_jdk25)
  • GitHub Check: typetools.checker-framework (misc_jdk25)
🔇 Additional comments (1)
checker/src/main/java/org/checkerframework/checker/initialization/InitializationAnnotatedTypeFactory.java (1)

795-813: LGTM — caching enabled for constructor argument annotated types

The Issue6623.java test is present and no other initialization-related tests reference visitNewClass. This change directly addresses issue #6623’s performance regression without altering semantics. Ensure the full test suite passes.


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

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

Copy link
Member

@mernst mernst left a comment

Choose a reason for hiding this comment

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

LGTM

@mernst mernst merged commit fc3a55d into typetools:master Oct 1, 2025
20 checks passed
@mernst mernst deleted the issue6623 branch October 1, 2025 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Performance regression in 3.43.0 with nested streams

2 participants