Add Contract annotation to nullaway-annotations#1569
Conversation
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds a new annotation type com.uber.nullaway.annotations.Contract with Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
msridhar
left a comment
There was a problem hiding this comment.
Thanks for this! I have a few comments on the annotation itself. Also, please add a unit test showing this annotation has the intended effect with NullAway
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1569 +/- ##
=========================================
Coverage 88.40% 88.40%
Complexity 2881 2881
=========================================
Files 103 103
Lines 9637 9637
Branches 1940 1940
=========================================
Hits 8520 8520
Misses 535 535
Partials 582 582 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
msridhar
left a comment
There was a problem hiding this comment.
Meant to request changes before
|
Also @codingkiddo we need you to sign the CLA before we can accept this contribution |
Signed-off-by: Vinod Kumar <codingkiddo@gmail.com>
a5efb4b to
3fd1d2e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@annotations/src/main/java/com/uber/nullaway/annotations/Contract.java`:
- Around line 34-43: Update the Contract annotation so it's compatible with
common JetBrains-style usages by making the existing String value() optional
(give it a default, e.g., ""), and add a boolean pure() default false element;
modify the annotation declaration in Contract (the value() method and add
pure()) so usages like `@Contract`(pure = true) and `@Contract`("...") both compile
without changes to callers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ff49900f-3ab3-4bb5-809f-ecbc4fd7e7c2
📒 Files selected for processing (2)
annotations/src/main/java/com/uber/nullaway/annotations/Contract.javanullaway/src/test/java/com/uber/nullaway/ContractsTests.java
| public @interface Contract { | ||
|
|
||
| /** | ||
| * Contains the contract clauses describing relationships between call arguments and the returned | ||
| * value or exceptional behavior. | ||
| * | ||
| * @return the contract string | ||
| */ | ||
| String value(); | ||
| } |
There was a problem hiding this comment.
Make @Contract signature compatible with common JetBrains-style usages.
value() is currently mandatory and pure() is missing, so usages like @Contract(pure = true) won’t compile with this new annotation. That undermines the “drop-in NullAway-owned Contract” goal for existing users.
Suggested API adjustment
public `@interface` Contract {
@@
- String value();
+ String value() default "";
+
+ /**
+ * Whether the annotated method has no side effects.
+ *
+ * `@return` {`@code` true} if the method is pure
+ */
+ boolean pure() default false;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@annotations/src/main/java/com/uber/nullaway/annotations/Contract.java` around
lines 34 - 43, Update the Contract annotation so it's compatible with common
JetBrains-style usages by making the existing String value() optional (give it a
default, e.g., ""), and add a boolean pure() default false element; modify the
annotation declaration in Contract (the value() method and add pure()) so usages
like `@Contract`(pure = true) and `@Contract`("...") both compile without changes to
callers.
|
Thanks for the review. Updated the PR to address the comments:
Tested with:
|
Done, I have signed the CLA. I also noticed the CLA check was tied to my commit author email, so I have updated the commit author/signoff to use my GitHub-linked email and force-pushed the branch. Could you please recheck the CLA status when you get a chance? Thanks! |
Fixes #1543.
This PR adds
com.uber.nullaway.annotations.Contractto thenullaway-annotationsartifact.NullAway already recognizes annotations whose simple name is
Contract, so this provides a NullAway-owned annotation users can depend on directly.Changes:
@Contractannotation undercom.uber.nullaway.annotationsRetentionPolicy.CLASSMETHODandCONSTRUCTORtargetsvalue()andpure()Tested with:
./gradlew :annotations:build./gradlew :annotations:javadoc./gradlew spotlessCheckSummary by CodeRabbit
@Contractannotation for methods and constructors with a required contract string; it's documented and retained in compiled classes, with Javadoc describing the contract grammar and examples.@Contractannotation is recognized and influences nullness reasoning at call sites.