Skip to content

mark all APIs nullable/non-nullable#1764

Open
asolntsev wants to merge 7 commits intomainfrom
nullability
Open

mark all APIs nullable/non-nullable#1764
asolntsev wants to merge 7 commits intomainfrom
nullability

Conversation

@asolntsev
Copy link
Collaborator

JSpecify is de-facto standard annotations for marking nullability of variables, parameters, return values and fields. Supported by all IDEs.

All remarkable libraries have migrated to JSpecify: Spring, JUnit etc.

  1. By default, we mark everything as non-nullable (see @NullMarked in package-info.java)
  2. When needed, we mark the nullable parameters/fields as @nullable.

JSpecify is de-facto standard annotations for marking nullability of variables, parameters, return values and fields. Supported by all IDEs.

1. By default, we mark everything as non-nullable (see @NullMarked in package-info.java)
2. When needed, we mark the nullable parameters/fields as @nullable.
@asolntsev asolntsev added this to the 2.6.0 milestone Feb 20, 2026
@asolntsev asolntsev added the enhancement New feature or request label Feb 20, 2026
@asolntsev asolntsev self-assigned this Feb 20, 2026
@bodiam
Copy link
Contributor

bodiam commented Feb 20, 2026

Awesome! Just wondering, could this start breaking things for libraries which depend on us, or Kotlin projects? Just wonder if we release this, should this be 2.x release or 3.x ?

@codecov-commenter
Copy link

codecov-commenter commented Feb 20, 2026

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 93.93939% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.55%. Comparing base (4e4a5bc) to head (1362f0e).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...n/java/net/datafaker/annotations/FakeResolver.java 83.33% 2 Missing ⚠️
...ain/java/net/datafaker/providers/base/Vehicle.java 50.00% 0 Missing and 1 partial ⚠️
...java/net/datafaker/service/FakeValuesGrouping.java 80.00% 0 Missing and 1 partial ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1764      +/-   ##
============================================
+ Coverage     92.43%   92.55%   +0.11%     
  Complexity     3450     3450              
============================================
  Files           339      339              
  Lines          6794     6794              
  Branches        670      663       -7     
============================================
+ Hits           6280     6288       +8     
+ Misses          351      348       -3     
+ Partials        163      158       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@asolntsev
Copy link
Collaborator Author

No, it doesn't need to be 3.0
These are just annotations that fix current state of things. They don't change the behavior, they will just help IDE to highlight problematic places in code.

@bodiam
Copy link
Contributor

bodiam commented Feb 20, 2026

Okay. My understanding was that before the code is annotated, Kotlin will treat this as platform types. So, the following code would work:

var name: String? = faker.name().firstName()
var name: String = faker.name().firstName()

Both would work. But if I look here: https://resources.jetbrains.com/storage/products/kotlinconf-2025/may-23/JSpecify_%20Java%20Nullness%20Annotations%20and%20Kotlin%20_%20David%20Baker.pdf, they say:

The Kotlin compiler treats JSpecify-annotated Java code as null-safe types as of version 2.1.0.

So, if we'd add @nullable to something Kotlin clients assumed was non-null, they'll get compile errors if they're using it in a non-null context without null checks.

So, something like this might break Kotlin consumers, I think:

    @Nullable
    public String userId() {

Also, I think the code is perhaps incorrect, I don't think it should ever return a nullable userId. (and yes, I know what the documentation says, but it's also a bit sus) Wdyt?

@asolntsev
Copy link
Collaborator Author

asolntsev commented Feb 20, 2026

The Kotlin compiler treats JSpecify-annotated Java code as null-safe types as of version 2.1.0.
So, something like this might break Kotlin consumers, I think:

@bodiam Hm... Yes, that's right: the new annotations can break compilation of existing Kotlin code.
For example:

  val userId1: String = faker.credentials().userId(null);
  • Previously: it compiled because Kotlin doesn't know if userId(null) can return null.
  • Now: this code doesn't compile because Kotlin knows that userId(null) can return null.

So yes, we need to release it as DataFaker 3.0.0 :)

P.S. And yes, good point: method faker.credentials.userId() never returns null. I've removed @Nullable annotation from this method.

method `faker.credentials.userId()` never returns null.
@asolntsev asolntsev requested a review from kingthorin February 20, 2026 08:49
@asolntsev asolntsev modified the milestones: 2.6.0, 3.0.0 Feb 20, 2026
Copy link
Collaborator

@kingthorin kingthorin left a comment

Choose a reason for hiding this comment

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

It's kind of a lot to review, especially with the unrelated typo fixes etc.

These are the only things I noticed.

@snuyanzin
Copy link
Collaborator

is there any enforcer that could fail the build if missed?
Or otherwise it always be in a phase of "eventual consistency"

@bodiam
Copy link
Contributor

bodiam commented Feb 20, 2026

So yes, we need to release it as DataFaker 3.0.0 :)

P.S. And yes, good point: method faker.credentials.userId() never returns null. I've removed @Nullable annotation from this method.

Just to be clear on this: I do really like the PR, I think we should absolutely do it, just may need a bit of checking on the impact, and how we're communicating this, that's all. I have no issue with releasing DF 3.x either.

@snuyanzin
Copy link
Collaborator

@asolntsev can you explain how you put annotations?
It looks like in some cases it makes some in some cases there are placed very randomly

@bodiam
Copy link
Contributor

bodiam commented Feb 20, 2026

I used Claude Code to review it as well:

Summary

This branch introduces JSpecify (@NullMarked, @nullable) and Error Prone (@CheckReturnValue) annotations across the entire codebase. It adds package-info.java files to 22 packages, making non-null the default for
all parameters and return types, and explicitly marks nullable locations with @nullable. Along the way, it includes several cleanup improvements: thread-safety fixes, dead-code removal, raw type elimination, and
typo corrections.

Findings

Critical

None found.

Improvements

  1. Vehicle.licensePlate(String) - Javadoc still references old behavior (Vehicle.java:147)
    The Javadoc at line 147 still says "If the regex is null or invalid, it returns null" - wait, that's actually on Credentials. For Vehicle.licensePlate, the method now returns @nullable but the null check on
    licensePlatesByState was removed. This is correct since resolve() throws rather than returning null - but this is a subtle contract dependency. A brief comment explaining why the null check was removed would help
    future readers.
  2. Credentials.userId() - stale Javadoc (Credentials.java:146-149)
    The no-arg userId() Javadoc still says "If the regex is null or invalid, it returns null" (line 147), but the method no longer delegates to the nullable overload and will now throw if resolve() fails. The Javadoc
    should be updated to match.
  3. SingletonLocale.get() still returns @nullable (SingletonLocale.java:22-27)
    The get(Locale) method accepts @nullable and returns @nullable. Several call sites now wrap it with requireNonNull(SingletonLocale.get(locale), ...). Consider whether the @nullable accepting overload is still
    needed. Currently FakerContext has a private singletonLocale() helper, and FakeValuesContext has an inline requireNonNull. Since get(null) is only useful for the one place that truly passes a nullable locale, it
    might be cleaner to have callers handle the null check themselves and make get() non-null-in-non-null-out - but this is a judgment call given this is already the chosen direction.
  4. FakeValuesService.addPath accepts @nullable Path (FakeValuesService.java:212)
    The @nullable on the path parameter is odd for a public API since the next line immediately throws on null. Consider keeping it non-null in the signature and letting NullMarked enforce it at compile time, rather
    than accepting null only to immediately reject it. Same goes for addUrl where url is now enforced via requireNonNull but the parameter isn't marked @nullable (which is actually the better approach).
  5. TomlTransformer.writeSchema - Set<@nullable String> (TomlTransformer.java:68)
    The Set<@nullable String> for seen looks intentional (since f.getName() can return null), but adding null to a HashSet and then calling add(null) again to detect duplicates could lead to confusing "duplicate key"
    errors for unnamed fields. This seems pre-existing but worth a thought.
  6. @SuppressWarnings("ConstantValue") in WeightedRandomSelector (WeightedRandomSelector.java:59,67,75)
    These are appropriate since the validation methods guard against null at the API boundary. Just confirming this was intentional and correct.
  7. LazyEvaluated.get() uses requireNonNull (LazyEvaluated.java:26)
    The requireNonNull(value, "Null value not allowed") is correct for the contract (supplier shouldn't return null), but the message "Null value not allowed" could be more descriptive, e.g., "Supplier returned null"
    or "LazyEvaluated value was null after evaluation".

Nitpicks

  • Lorem.java:116: new StringBuilder(wordCount * 9) - the initial capacity heuristic (9 chars per word) is a reasonable optimization. Minor: a comment explaining the magic number would be nice.
  • FakeValuesGrouping.java: Nice cleanup removing raw types and adding the null check before putAll.
  • Internet.BotUserAgent.any: Good simplification using nextEnum().
  • Typo fixes ("estonian" -> "Estonian", "summ" -> "sum", "criterias" -> "criteria", "a algorithm" -> "an algorithm") are all welcome.
  • The CsvTest exception type change from IllegalArgumentException to NullPointerException correctly tracks the behavioral change in SimpleField.transform().

Conclusion

Approved. This is a high-quality, thorough nullability annotation pass. The @NullMarked + @nullable approach with JSpecify is the modern best practice for Java, the provided scope is correct, and the various code
cleanups (thread-safety in SingletonLocale, dead code removal in Vehicle, raw type cleanup in FakeValuesGrouping) are all improvements. The setLocale being made final to avoid virtual dispatch from the
constructor is a particularly nice detail.

The only item I'd suggest addressing before merge is the stale Javadoc on Credentials.userId() (finding #2).

@asolntsev
Copy link
Collaborator Author

@asolntsev can you explain how you put annotations?
It looks like in some cases it makes some in some cases there are placed very randomly

As I explained in the PR description:

  1. By default, we mark everything as non-nullable (see @NullMarked in package-info.java)
  2. When needed, we mark the nullable parameters/fields as @nullable.

So I added @nullable in places where i see that null is actually expected. Of course, I might make a mistake in some of these places (but I hope very few).

@asolntsev
Copy link
Collaborator Author

is there any enforcer that could fail the build if missed?

Yes, there is library "NullAway" (probably some others as well).
I think Spotbugs can also be configured to fail the build on nullability problems.
I just thought it would be too much for one PR. We can enable such enforcer in a separate PR.

@kingthorin
Copy link
Collaborator

I don't feel like my skills/knowledge give me a good place from which to review this PR.

As a contributor how would I know when/if I should be annotating something?

@asolntsev
Copy link
Collaborator Author

asolntsev commented Feb 25, 2026

@kingthorin

I don't feel like my skills/knowledge give me a good place from which to review this PR.

Yes, unfortunately, such PR is hard to review.
If you really want, you can checkout this branch in review IDE warnings about potential nullability problems.

As a contributor how would I know when/if I should be annotating something?
What I did:

  1. Marked everything as non-nullable by default (added @NullMarked in package-info.java files in all packages).
  2. Went through the codebase (reviewed every single file one by one) and looked for IDEA warnings. In every single place where I saw that null is actually passed to variable/parameter/field, I marked this specific var/param/field as @Nullable.

Looks massive, but actually simple.

@kingthorin
Copy link
Collaborator

Seems reasonable to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants