Skip to content

Conversation

scordio
Copy link
Contributor

@scordio scordio commented Jun 8, 2025

This PR migrates spring-batch-infrastructure to JSpecify annotations and introduces NullAway to verify @NullMarked annotated packages (productive code only).

I used the following strategy to handle cases of null-safety mismatch detected by NullAway:

  • Whenever possible, I refactored the code to be null-safe
  • When refactoring was either not possible or too complex, I annotated the corresponding part with @SuppressWarnings("DataFlowIssue"), which is recognized by both IntelliJ IDEA and NullAway

Many classes could benefit from a more aggressive refactoring, where a constructor with parameters could be favored instead of relying on the default constructor + setters + afterPropertiesSet implementation. I'll work on a proposal in a separate PR.

@fmbenhassine
Copy link
Contributor

This is a huge amount of work.. thank you very much for contributing this PR! I REALLY appreciate your time and effort on this 🙏

I am targeting the migration to jSpecify for v6.0.0-M3 (planned for Sep 17th), so I will start working on this right after releasing 6.0.0-M2. I see that the PR is still in draft status, which is ok, but is there a module I can I start reviewing?

Just as a (self) reminder, we need to get rid of the com.google.code.findbugs:jsr305 dependency as part of this.

@scordio

This comment was marked as outdated.

@fmbenhassine
Copy link
Contributor

Thank you for the update and for narrowing down the PR's scope to spring-batch-infrastructure!

Take your time and let me know when you think it is ready, no rush we can postpone to 6.0.0-M4 if necessary. I can also take it from where it is if needed.

@scordio scordio force-pushed the jspecify branch 4 times, most recently from 348cacc to 2295d49 Compare September 7, 2025 14:25
@scordio scordio force-pushed the jspecify branch 5 times, most recently from 6943f20 to 889cc4a Compare September 21, 2025 17:21
@scordio

This comment was marked as outdated.

@scordio scordio force-pushed the jspecify branch 2 times, most recently from a0d2ce0 to 28ffd60 Compare September 21, 2025 23:41
@scordio
Copy link
Contributor Author

scordio commented Sep 21, 2025

@fmbenhassine sorry for the delay, I'm happy to mention that this is finally ready for review! 🙂

@scordio scordio marked this pull request as ready for review September 21, 2025 23:46
@scordio
Copy link
Contributor Author

scordio commented Sep 22, 2025

Sadly, the CI job went into timeout, but it should be unrelated to the changes (locally, everything looks fine).

@fmbenhassine
Copy link
Contributor

Great thank you for all these updates! I will take a look asap.

Sadly, the CI job went into timeout, but it should be unrelated to the changes (locally, everything looks fine).

Yes, this is a known issue and I will bissect that for RC1.

@fmbenhassine
Copy link
Contributor

LGTM! Thank you very much.

I used the following strategy to handle cases of null-safety mismatch detected by NullAway:

That's how I would do it as well 👍

Many classes could benefit from a more aggressive refactoring, where a constructor with parameters could be favored instead of relying on the default constructor + setters + afterPropertiesSet implementation. I'll work on a proposal in a separate PR.

I came across this in #4984 as well. The approach I try to follow is to use constructor arguments for mandatory dependencies and setters for optional ones. I will take care of this refactoring.

@scordio
Copy link
Contributor Author

scordio commented Sep 24, 2025

Great thank you for all these updates! I will take a look asap.

@fmbenhassine If conflicts arise due to other commits on main, I wouldn't perform any rebases unless you tell me to do so, to avoid interfering with your review.

@fmbenhassine
Copy link
Contributor

Closing this in favour of #4990

@fmbenhassine fmbenhassine added the status: superseded Issues that are superseded by other issues label Sep 24, 2025
@scordio scordio deleted the jspecify branch September 24, 2025 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants