Skip to content

Review - JSpecify#1293

Merged
Jakubk15 merged 16 commits intomigrate-to-jspecifyfrom
migrate-to-jspecify-pr
Feb 7, 2026
Merged

Review - JSpecify#1293
Jakubk15 merged 16 commits intomigrate-to-jspecifyfrom
migrate-to-jspecify-pr

Conversation

@CitralFlo
Copy link
Member

My understanding of JSpecify for api - mostly used in Java-Kotlin communication. With NullMarked as clear way to display notnull by default. Nullmarks checked to second layer of services. Additional NotNull s from jetbrains required by intelliJ to not screen about parameter marks as notNull

…munication. With NullMarked as clear way to display notnull by default. Nullmarks checked to second layer of services. Additional `NotNull` s from jetbrains required by intelliJ to not screen about parameter marks as notNull
@CitralFlo CitralFlo requested a review from a team as a code owner February 7, 2026 01:07
@CitralFlo CitralFlo requested a review from Jakubk15 February 7, 2026 01:07
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces JSpecify annotations across the API to improve null-safety, which is a great step forward for code quality and Kotlin interoperability. The use of @NullMarked correctly sets non-null as the default for many interfaces and classes.

My review focuses on a few key areas:

  • I've pointed out several places where collection return types are incorrectly marked as containing @Nullable elements, while their implementations guarantee non-null elements. This misrepresents the API contract and should be corrected.
  • I've also raised a concern about removing runtime null checks from a public event constructor. While static analysis is helpful, runtime checks provide an essential safeguard against incorrect API usage from un-checked code.

Overall, the changes are positive, and addressing these points will further solidify the null-safety of the API.

CitralFlo and others added 5 commits February 7, 2026 02:33
…boy/CatboyService.java

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…inchat/event/AdminChatEvent.java

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…l/JailService.java

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…rp/WarpServiceImpl.java

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Copy link
Member

@Jakubk15 Jakubk15 left a comment

Choose a reason for hiding this comment

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

Overall good PR, minor tweaks required

@Jakubk15 Jakubk15 changed the title Review Review - JSpecify Feb 7, 2026
@Jakubk15 Jakubk15 merged commit fc22303 into migrate-to-jspecify Feb 7, 2026
2 checks passed
@Jakubk15 Jakubk15 deleted the migrate-to-jspecify-pr branch February 7, 2026 10:30
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.

2 participants