Add JSpecify null annotations#379
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds JSpecify dependency and static module requirement, package-level ChangesJSpecify Nullability Annotations
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
This pull request introduces JSpecify nullness annotations across the main ThreeTen-Extra codebase by applying package-level non-null defaults (@NullMarked) and adding targeted @Nullable annotations where null is permitted (notably equals(Object) parameters, selected isSupported(...) parameters that explicitly allow null, and resolver methods that may return null).
Changes:
- Add
org.jspecifyas an optional/static annotation dependency (Maven + JPMS) to support JSpecify usage. - Apply
@NullMarkedat package level fororg.threeten.extra,org.threeten.extra.chrono, andorg.threeten.extra.scale. - Add
@Nullableannotations to align method signatures/fields with existing “null returns false” and “may return null” behavior.
Reviewed changes
Copilot reviewed 39 out of 39 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/org/threeten/extra/YearWeek.java | Annotates isSupported(...) parameters and equals(...) parameter as nullable. |
| src/main/java/org/threeten/extra/Years.java | Annotates equals(...) parameter as nullable. |
| src/main/java/org/threeten/extra/YearQuarter.java | Annotates isSupported(...) parameters and equals(...) parameter as nullable. |
| src/main/java/org/threeten/extra/YearHalf.java | Annotates isSupported(...) parameters and equals(...) parameter as nullable. |
| src/main/java/org/threeten/extra/Weeks.java | Annotates equals(...) parameter as nullable. |
| src/main/java/org/threeten/extra/TemporalFields.java | Marks resolve(...) return type as nullable. |
| src/main/java/org/threeten/extra/Seconds.java | Annotates equals(...) parameter as nullable. |
| src/main/java/org/threeten/extra/scale/UtcInstant.java | Marks cached toString field and equals(...) parameter as nullable. |
| src/main/java/org/threeten/extra/scale/TaiInstant.java | Annotates equals(...) parameter as nullable. |
| src/main/java/org/threeten/extra/scale/package-info.java | Applies package-level @NullMarked for scale package. |
| src/main/java/org/threeten/extra/Quarter.java | Annotates isSupported(...) parameter as nullable. |
| src/main/java/org/threeten/extra/PeriodDuration.java | Annotates equals(...) parameter as nullable. |
| src/main/java/org/threeten/extra/PackedFields.java | Marks resolve(...) return types as nullable. |
| src/main/java/org/threeten/extra/package-info.java | Applies package-level @NullMarked for main package. |
| src/main/java/org/threeten/extra/OffsetDate.java | Annotates isSupported(...) parameters and equals(...) parameter as nullable. |
| src/main/java/org/threeten/extra/MutableClock.java | Annotates equals(...) parameter as nullable. |
| src/main/java/org/threeten/extra/Months.java | Annotates equals(...) parameter as nullable. |
| src/main/java/org/threeten/extra/Minutes.java | Annotates equals(...) parameter as nullable. |
| src/main/java/org/threeten/extra/LocalDateRange.java | Marks spliterator state and comparator return as nullable; annotates equals(...) parameter. |
| src/main/java/org/threeten/extra/Interval.java | Annotates equals(...) parameter as nullable. |
| src/main/java/org/threeten/extra/Hours.java | Annotates equals(...) parameter as nullable. |
| src/main/java/org/threeten/extra/HourMinute.java | Annotates isSupported(...) parameters and equals(...) parameter as nullable. |
| src/main/java/org/threeten/extra/Half.java | Annotates isSupported(...) parameter as nullable. |
| src/main/java/org/threeten/extra/Days.java | Annotates equals(...) parameter as nullable. |
| src/main/java/org/threeten/extra/DayOfYear.java | Annotates isSupported(...) and equals(...) parameters as nullable. |
| src/main/java/org/threeten/extra/DayOfMonth.java | Annotates isSupported(...), isValidYearMonth(...), and equals(...) parameters as nullable. |
| src/main/java/org/threeten/extra/chrono/Symmetry454Chronology.java | Marks getCalendarType() return as nullable. |
| src/main/java/org/threeten/extra/chrono/Symmetry010Chronology.java | Marks getCalendarType() return as nullable. |
| src/main/java/org/threeten/extra/chrono/package-info.java | Applies package-level @NullMarked for chrono package. |
| src/main/java/org/threeten/extra/chrono/InternationalFixedChronology.java | Marks getCalendarType() return as nullable. |
| src/main/java/org/threeten/extra/chrono/BritishCutoverDate.java | Annotates equals(...) parameter as nullable. |
| src/main/java/org/threeten/extra/chrono/BritishCutoverChronology.java | Marks getCalendarType() return as nullable. |
| src/main/java/org/threeten/extra/chrono/AccountingDate.java | Annotates equals(...) parameter as nullable. |
| src/main/java/org/threeten/extra/chrono/AccountingChronologyBuilder.java | Marks builder fields as nullable where unset until configured. |
| src/main/java/org/threeten/extra/chrono/AccountingChronology.java | Marks getCalendarType() return and equals(...) parameter as nullable. |
| src/main/java/org/threeten/extra/chrono/AbstractDate.java | Annotates equals(...) parameter as nullable. |
| src/main/java/org/threeten/extra/AmPm.java | Annotates isSupported(...) parameter as nullable. |
| src/main/java/module-info.java | Adds requires static org.jspecify; for JPMS builds. |
| pom.xml | Adds optional JSpecify dependency and version property. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@pom.xml`:
- Around line 508-514: Remove the optional=true attribute and the copied
"mandatory in Scala" comment from the JSpecify dependency entry so the
org.jspecify:jspecify artifact is not marked optional; update the dependency
block that contains groupId org.jspecify and artifactId jspecify to omit the
<optional> element (and delete the inline comment) so downstream consumers can
access the annotations.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c458efe7-9b78-4a7e-abb8-c44fb52c9bd9
📒 Files selected for processing (39)
pom.xmlsrc/main/java/module-info.javasrc/main/java/org/threeten/extra/AmPm.javasrc/main/java/org/threeten/extra/DayOfMonth.javasrc/main/java/org/threeten/extra/DayOfYear.javasrc/main/java/org/threeten/extra/Days.javasrc/main/java/org/threeten/extra/Half.javasrc/main/java/org/threeten/extra/HourMinute.javasrc/main/java/org/threeten/extra/Hours.javasrc/main/java/org/threeten/extra/Interval.javasrc/main/java/org/threeten/extra/LocalDateRange.javasrc/main/java/org/threeten/extra/Minutes.javasrc/main/java/org/threeten/extra/Months.javasrc/main/java/org/threeten/extra/MutableClock.javasrc/main/java/org/threeten/extra/OffsetDate.javasrc/main/java/org/threeten/extra/PackedFields.javasrc/main/java/org/threeten/extra/PeriodDuration.javasrc/main/java/org/threeten/extra/Quarter.javasrc/main/java/org/threeten/extra/Seconds.javasrc/main/java/org/threeten/extra/TemporalFields.javasrc/main/java/org/threeten/extra/Weeks.javasrc/main/java/org/threeten/extra/YearHalf.javasrc/main/java/org/threeten/extra/YearQuarter.javasrc/main/java/org/threeten/extra/YearWeek.javasrc/main/java/org/threeten/extra/Years.javasrc/main/java/org/threeten/extra/chrono/AbstractDate.javasrc/main/java/org/threeten/extra/chrono/AccountingChronology.javasrc/main/java/org/threeten/extra/chrono/AccountingChronologyBuilder.javasrc/main/java/org/threeten/extra/chrono/AccountingDate.javasrc/main/java/org/threeten/extra/chrono/BritishCutoverChronology.javasrc/main/java/org/threeten/extra/chrono/BritishCutoverDate.javasrc/main/java/org/threeten/extra/chrono/InternationalFixedChronology.javasrc/main/java/org/threeten/extra/chrono/Symmetry010Chronology.javasrc/main/java/org/threeten/extra/chrono/Symmetry454Chronology.javasrc/main/java/org/threeten/extra/chrono/package-info.javasrc/main/java/org/threeten/extra/package-info.javasrc/main/java/org/threeten/extra/scale/TaiInstant.javasrc/main/java/org/threeten/extra/scale/UtcInstant.javasrc/main/java/org/threeten/extra/scale/package-info.java
Member
Author
|
Unfortunately, JSpecify has no firm conclusion as to what library authors are supposed to do with the My choice is to use |
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 `@src/main/java/org/threeten/extra/chrono/AccountingChronologyBuilder.java`:
- Around line 193-196: The builder's toChronology() currently uses
Objects.requireNonNull on endsOn, end, and division which throws
NullPointerException, violating the method's documented DateTimeException
contract; update toChronology() so it checks the same fields (endsOn, end,
division, and any other required state like
inLastWeek/leapWeekInMonth/yearOffset if applicable) and throws
java.time.DateTimeException with clear messages when a required field is unset,
then call AccountingChronology.create(...) only after all validations pass to
preserve the documented exception behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 72898300-eb70-418b-8563-cac201320160
📒 Files selected for processing (8)
src/main/java/org/threeten/extra/OffsetDate.javasrc/main/java/org/threeten/extra/PeriodDuration.javasrc/main/java/org/threeten/extra/YearHalf.javasrc/main/java/org/threeten/extra/YearQuarter.javasrc/main/java/org/threeten/extra/chrono/AccountingChronology.javasrc/main/java/org/threeten/extra/chrono/AccountingChronologyBuilder.javasrc/main/java/org/threeten/extra/chrono/AccountingDate.javasrc/main/java/org/threeten/extra/chrono/BritishCutoverDate.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/threeten/extra/chrono/AccountingChronology.java
Closed
cpovirk
approved these changes
Jun 11, 2026
cpovirk
left a comment
cpovirk
left a comment
There was a problem hiding this comment.
I checked this against some partial automation that we have for annotating for nullness. Aside from a couple cases in which the automation got unusual cases wrong, all its annotations match yours. I looked at the new requireNonNull calls, and they all look reasonable to me. Then I grepped around a bit in the files you edited and in the partial JSpecify coverage we have for java.time, and then I turned Gemini loose.
I'm kicking off some testing in our depot to run overnight.
I offer a very optional comment below as a little evidence that I did in fact look through some code myself :)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/main/java/org/threeten/extra/chrono/AccountingChronologyBuilder.java (1)
192-195:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
toChronology()throwsNullPointerExceptionbut documentsDateTimeException.Line 190 declares
@throws DateTimeException if the chronology cannot be built, yet whenendsOn,end, ordivisionare null,AccountingChronology.create(...)(lines 177-179 in AccountingChronology.java) throwsNullPointerExceptionviaObjects.requireNonNull. This breaks the documented contract and may break callers that catch onlyDateTimeException. Validate required builder fields and throwDateTimeExceptionwith a descriptive message before delegating tocreate(...).🛠️ Proposed fix
public AccountingChronology toChronology() { - //noinspection DataFlowIssue - nullness checked in the constructor of AccountingChronology + if (endsOn == null || end == null || division == null) { + throw new DateTimeException( + "AccountingChronologyBuilder is incomplete: endsOn, end, and division must be set"); + } return AccountingChronology.create(endsOn, end, inLastWeek, division, leapWeekInMonth, yearOffset); }🤖 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 `@src/main/java/org/threeten/extra/chrono/AccountingChronologyBuilder.java` around lines 192 - 195, The toChronology() method currently delegates to AccountingChronology.create(...) which can throw NullPointerException when required fields are null; update toChronology() to validate the required builder fields (endsOn, end, division and any other non-null parameters used by AccountingChronology.create) and, if any are null, throw a java.time.DateTimeException with a clear descriptive message listing the missing field(s) before calling AccountingChronology.create(...), preserving the documented contract.
🧹 Nitpick comments (1)
src/main/java/org/threeten/extra/chrono/AccountingChronology.java (1)
242-246: ⚡ Quick winInconsistent null-checking in
readResolve().Lines 243-244 explicitly check
endsOnandendwithObjects.requireNonNull, butdivision(obtained viagetDivision()on line 245) is not checked here. SinceAccountingChronology.create(...)validates all three on lines 177-179, the early checks forendsOnandendprovide no additional safety—they only make the code inconsistent. Either check all required fields early inreadResolve(), or remove the redundant checks and rely oncreate(...)to enforce non-null requirements uniformly.🔧 Option 1: Check all fields consistently
private Object readResolve() { Objects.requireNonNull(endsOn, "endsOn"); Objects.requireNonNull(end, "end"); + Objects.requireNonNull(division, "division"); return AccountingChronology.create(endsOn, end, inLastWeek, getDivision(), leapWeekInMonth, yearOffset); }🔧 Option 2: Remove redundant checks (preferred)
private Object readResolve() { - Objects.requireNonNull(endsOn, "endsOn"); - Objects.requireNonNull(end, "end"); return AccountingChronology.create(endsOn, end, inLastWeek, getDivision(), leapWeekInMonth, yearOffset); }🤖 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 `@src/main/java/org/threeten/extra/chrono/AccountingChronology.java` around lines 242 - 246, Remove the redundant early null-checks in AccountingChronology.readResolve(): delete the Objects.requireNonNull(endsOn, "endsOn") and Objects.requireNonNull(end, "end") calls and rely on AccountingChronology.create(endsOn, end, inLastWeek, getDivision(), leapWeekInMonth, yearOffset) to validate endsOn, end and division uniformly (getDivision() is already passed into create); this makes null-checking consistent and avoids duplicate validation.
🤖 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 `@src/changes/changes.xml`:
- Around line 10-16: The release note incorrectly states the exception change;
update the release XML entry so it references that adding JSpecify nullness
annotations causes builder validation in
AccountingChronologyBuilder.toChronology() to throw NullPointerException (or a
null-related runtime exception) which breaks the documented DateTimeException
contract for AccountingChronology construction, rather than claiming a swap from
IllegalStateException; mention the affected symbol AccountingChronology and the
builder method toChronology() and clarify this impacts the documented
DateTimeException behavior.
In
`@src/test/java/org/threeten/extra/chrono/TestAccountingChronologyBuilder.java`:
- Around line 462-485: The tests currently assert NullPointerException for
incomplete builders (see test_nullPassedIn_nearestEndOf and
test_nullPassedIn_inLastWeekOf) but toChronology() in
AccountingChronologyBuilder is documented to throw DateTimeException; update the
JavaDoc on AccountingChronologyBuilder#toChronology to also declare (and
document the conditions for) NullPointerException when required builder
parameters (e.g., values set via endsOn, nearestEndOf, inLastWeekOf,
withDivision, leapWeekInMonth) are null or incomplete so the documentation
matches the observed behavior and the test expectations.
---
Duplicate comments:
In `@src/main/java/org/threeten/extra/chrono/AccountingChronologyBuilder.java`:
- Around line 192-195: The toChronology() method currently delegates to
AccountingChronology.create(...) which can throw NullPointerException when
required fields are null; update toChronology() to validate the required builder
fields (endsOn, end, division and any other non-null parameters used by
AccountingChronology.create) and, if any are null, throw a
java.time.DateTimeException with a clear descriptive message listing the missing
field(s) before calling AccountingChronology.create(...), preserving the
documented contract.
---
Nitpick comments:
In `@src/main/java/org/threeten/extra/chrono/AccountingChronology.java`:
- Around line 242-246: Remove the redundant early null-checks in
AccountingChronology.readResolve(): delete the Objects.requireNonNull(endsOn,
"endsOn") and Objects.requireNonNull(end, "end") calls and rely on
AccountingChronology.create(endsOn, end, inLastWeek, getDivision(),
leapWeekInMonth, yearOffset) to validate endsOn, end and division uniformly
(getDivision() is already passed into create); this makes null-checking
consistent and avoids duplicate validation.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e5568b87-ed85-4549-821e-50f6a186656a
📒 Files selected for processing (23)
src/changes/changes.xmlsrc/main/java/org/threeten/extra/AmPm.javasrc/main/java/org/threeten/extra/DayOfMonth.javasrc/main/java/org/threeten/extra/DayOfYear.javasrc/main/java/org/threeten/extra/Half.javasrc/main/java/org/threeten/extra/HourMinute.javasrc/main/java/org/threeten/extra/Interval.javasrc/main/java/org/threeten/extra/LocalDateRange.javasrc/main/java/org/threeten/extra/OffsetDate.javasrc/main/java/org/threeten/extra/Quarter.javasrc/main/java/org/threeten/extra/YearHalf.javasrc/main/java/org/threeten/extra/YearQuarter.javasrc/main/java/org/threeten/extra/YearWeek.javasrc/main/java/org/threeten/extra/chrono/AccountingChronology.javasrc/main/java/org/threeten/extra/chrono/AccountingChronologyBuilder.javasrc/main/java/org/threeten/extra/chrono/BritishCutoverChronology.javasrc/main/java/org/threeten/extra/chrono/BritishCutoverDate.javasrc/main/java/org/threeten/extra/chrono/CopticChronology.javasrc/main/java/org/threeten/extra/chrono/DiscordianChronology.javasrc/main/java/org/threeten/extra/chrono/EthiopicChronology.javasrc/main/java/org/threeten/extra/chrono/JulianChronology.javasrc/main/java/org/threeten/extra/chrono/PaxChronology.javasrc/test/java/org/threeten/extra/chrono/TestAccountingChronologyBuilder.java
✅ Files skipped from review due to trivial changes (3)
- src/main/java/org/threeten/extra/chrono/DiscordianChronology.java
- src/main/java/org/threeten/extra/DayOfYear.java
- src/main/java/org/threeten/extra/chrono/CopticChronology.java
🚧 Files skipped from review as they are similar to previous changes (6)
- src/main/java/org/threeten/extra/chrono/BritishCutoverChronology.java
- src/main/java/org/threeten/extra/Interval.java
- src/main/java/org/threeten/extra/chrono/BritishCutoverDate.java
- src/main/java/org/threeten/extra/OffsetDate.java
- src/main/java/org/threeten/extra/HourMinute.java
- src/main/java/org/threeten/extra/DayOfMonth.java
Comment on lines
+10
to
+16
| <release version="1.10.0" date="SNAPSHOT" description="v1.10.0"> | ||
| <action dev="jodastephen" type="add" issue="379"> | ||
| Add JSpecify nullness annotations. | ||
| This causes AccountingChronology to throw NullPointerException instead of IllegalStateException when null is passed in. | ||
| See also #170. | ||
| </action> | ||
| </release> |
There was a problem hiding this comment.
Release note inaccurately describes the exception change.
Line 13 states: "This causes AccountingChronology to throw NullPointerException instead of IllegalStateException when null is passed in." However, the builder's toChronology() method (line 190 in AccountingChronologyBuilder.java) documents @throws DateTimeException, not IllegalStateException. The note should clarify that the change affects builder validation and breaks the documented DateTimeException contract.
📝 Suggested correction
<action dev="jodastephen" type="add" issue="379">
Add JSpecify nullness annotations.
- This causes AccountingChronology to throw NullPointerException instead of IllegalStateException when null is passed in.
+ This causes AccountingChronologyBuilder.toChronology() to throw NullPointerException instead of the documented DateTimeException when required fields are null.
See also `#170`.
</action>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <release version="1.10.0" date="SNAPSHOT" description="v1.10.0"> | |
| <action dev="jodastephen" type="add" issue="379"> | |
| Add JSpecify nullness annotations. | |
| This causes AccountingChronology to throw NullPointerException instead of IllegalStateException when null is passed in. | |
| See also #170. | |
| </action> | |
| </release> | |
| <release version="1.10.0" date="SNAPSHOT" description="v1.10.0"> | |
| <action dev="jodastephen" type="add" issue="379"> | |
| Add JSpecify nullness annotations. | |
| This causes AccountingChronologyBuilder.toChronology() to throw NullPointerException instead of the documented DateTimeException when required fields are null. | |
| See also `#170`. | |
| </action> | |
| </release> |
🤖 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 `@src/changes/changes.xml` around lines 10 - 16, The release note incorrectly
states the exception change; update the release XML entry so it references that
adding JSpecify nullness annotations causes builder validation in
AccountingChronologyBuilder.toChronology() to throw NullPointerException (or a
null-related runtime exception) which breaks the documented DateTimeException
contract for AccountingChronology construction, rather than claiming a swap from
IllegalStateException; mention the affected symbol AccountingChronology and the
builder method toChronology() and clarify this impacts the documented
DateTimeException behavior.
Comment on lines
+462
to
+485
| public static Stream<Arguments> data_nullPassedIn() { | ||
| return Stream.of( | ||
| arguments(DayOfWeek.MONDAY, Month.JANUARY, null, 13), | ||
| arguments(DayOfWeek.MONDAY, null, AccountingYearDivision.THIRTEEN_EVEN_MONTHS_OF_4_WEEKS, 13), | ||
| arguments(null, Month.JANUARY, AccountingYearDivision.THIRTEEN_EVEN_MONTHS_OF_4_WEEKS, 13)); | ||
| } | ||
|
|
||
| @ParameterizedTest | ||
| @MethodSource("data_nullPassedIn") | ||
| public void test_nullPassedIn_nearestEndOf(DayOfWeek dayOfWeek, Month ending, AccountingYearDivision division, int leapWeekInMonth) { | ||
| assertThrows(NullPointerException.class, | ||
| () -> new AccountingChronologyBuilder().endsOn(dayOfWeek).nearestEndOf(ending) | ||
| .withDivision(division).leapWeekInMonth(leapWeekInMonth) | ||
| .toChronology()); | ||
| } | ||
|
|
||
| @ParameterizedTest | ||
| @MethodSource("data_nullPassedIn") | ||
| public void test_nullPassedIn_inLastWeekOf(DayOfWeek dayOfWeek, Month ending, AccountingYearDivision division, int leapWeekInMonth) { | ||
| assertThrows(NullPointerException.class, | ||
| () -> new AccountingChronologyBuilder().endsOn(dayOfWeek).inLastWeekOf(ending) | ||
| .withDivision(division).leapWeekInMonth(leapWeekInMonth) | ||
| .toChronology()); | ||
| } |
There was a problem hiding this comment.
Tests confirm NullPointerException but toChronology() documents DateTimeException.
The new test_nullPassedIn_* methods correctly verify that NullPointerException is thrown for null builder inputs. However, this contradicts the @throws DateTimeException contract documented for toChronology() at line 190 in AccountingChronologyBuilder.java. Either the tests should expect DateTimeException, or the JavaDoc contract must be updated to declare NullPointerException for incomplete builders (which would be a breaking API change).
🤖 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 `@src/test/java/org/threeten/extra/chrono/TestAccountingChronologyBuilder.java`
around lines 462 - 485, The tests currently assert NullPointerException for
incomplete builders (see test_nullPassedIn_nearestEndOf and
test_nullPassedIn_inLastWeekOf) but toChronology() in
AccountingChronologyBuilder is documented to throw DateTimeException; update the
JavaDoc on AccountingChronologyBuilder#toChronology to also declare (and
document the conditions for) NullPointerException when required builder
parameters (e.g., values set via endsOn, nearestEndOf, inLastWeekOf,
withDivision, leapWeekInMonth) are null or incomplete so the documentation
matches the observed behavior and the test expectations.
cpovirk
approved these changes
Jun 12, 2026
cpovirk
left a comment
cpovirk
left a comment
There was a problem hiding this comment.
(The overnight testing all went well, too, so I think we'll be able to import this very soon after it lands.)
cpovirk
added a commit
to jspecify/jdk
that referenced
this pull request
Jun 12, 2026
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/main/java/org/threeten/extra/chrono/AccountingChronologyBuilder.java (1)
184-190:⚠️ Potential issue | 🟠 MajorJavaDoc-behavior mismatch for exception contract.
The JavaDoc (lines 184-186) documents
IllegalStateExceptionfor invalid required fields, but the actual behavior throwsNullPointerExceptionwhenendsOn,end, ordivisionare null (viaObjects.requireNonNullinAccountingChronology.create, see context snippet 1). The tests (context snippet 3) expectNullPointerException.Additionally, the nullness suppression comment (line 189) says "constructor" but the null checks occur in the static factory method
AccountingChronology.create, not the private constructor.🤖 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 `@src/main/java/org/threeten/extra/chrono/AccountingChronologyBuilder.java` around lines 184 - 190, The JavaDoc for the toChronology method documents IllegalStateException for invalid required fields, but the actual implementation throws NullPointerException when endsOn, end, or division are null (via Objects.requireNonNull in AccountingChronology.create). Update the `@throws` clause in the JavaDoc to document NullPointerException instead of IllegalStateException to match the actual behavior. Additionally, fix the nullness suppression comment on line 189 to correctly reference AccountingChronology.create static factory method instead of "constructor", since that is where the null checks actually occur.
🤖 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.
Duplicate comments:
In `@src/main/java/org/threeten/extra/chrono/AccountingChronologyBuilder.java`:
- Around line 184-190: The JavaDoc for the toChronology method documents
IllegalStateException for invalid required fields, but the actual implementation
throws NullPointerException when endsOn, end, or division are null (via
Objects.requireNonNull in AccountingChronology.create). Update the `@throws`
clause in the JavaDoc to document NullPointerException instead of
IllegalStateException to match the actual behavior. Additionally, fix the
nullness suppression comment on line 189 to correctly reference
AccountingChronology.create static factory method instead of "constructor",
since that is where the null checks actually occur.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c94b9fff-7405-4284-90aa-17e66bddc36b
📒 Files selected for processing (1)
src/main/java/org/threeten/extra/chrono/AccountingChronologyBuilder.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Introduce JSpecify nullness annotations to ThreeTen-Extra.
JSpecify is becoming the de facto standard for nullness annotations in Java. It allows methods and fields to be marked as accepting null, with a non-null default.
This change introduces the annotations to the main code, not the tests.
See #170
Summary by CodeRabbit
Chores
Tests
NullPointerException.Documentation