Repository navigation
Conversation
As in #168, I identified most of these with https://errorprone.info/bugpattern/ReturnMissingNullable, modified to try to avoid annotating internals. And again I had to revert some of the changes, and again I made some manual additions: Removed annotations: - `ThreadLocal.initialValue` and `CountedCompleter.getRawResult`, examples of ["the `AtomicReference` case](jspecify/jspecify#145) - two `InetAddress` methods, as [already documented](https://github.com/jspecify/jdk/blob/e89cf6d97e5df874be7d9db4f0e14cc0c0a20648/src/java.base/share/classes/java/net/InetAddress.java#L807-L840) - `DecimalFormat.getDecimalFormatSymbols` and `javax.crypto.KDF` ([still](#168) unreachable) - more unreachable cases: `AbstractExecutorService.invokeAny`, `ForkJoinPool.invokeAny`, `Exchanger.exchange` - `CompletableFuture.resultNow`, which returns `null` only for an instance with a nullable type argument, as in [similar code in Guava's `AbstractFuture`](https://github.com/google/guava/blob/52d12e4aa2fe8bd1e09811daa4ced7cc99697f45/guava/src/com/google/common/util/concurrent/AbstractFuture.java#L315) - `query` implementations on `java.time` methods like `Instant.query`, which [are probably like `resultNow`](#125) - [`HashMap.computeIfAbsent` and `TreeMap.computeIfAbsent`](#156) - `java.security.KeyPairGenerator.generateKeyPair()`, on which Gemini has tons to say below I also made manual changes in: - `RecursiveAction`: I adopted `@Nullable Void` throughout and added `@NullMarked`. - I added `@Nullable` manually on `TimeZoneNameProvider.getDisplayName` based on its Javadoc. - I added various annotation in `Provider` based on its Javadoc and on the standard `Map` API. (OK, and in the case of `Service.supportsParameter`, I based it on the implementation and the comments there.) - I added `@Nullable` to the `String comment` parameter of another overload of `Properties.storeToXML` based on its Javadoc and our precedent on the other overloads. On `java.security.KeyPairGenerator.generateKeyPair()`: > ### What the `"special case"` comment means > > It’s a historical artifact of how the Java Cryptography Architecture (JCA) evolved from **JDK 1.1** to **JDK 1.2**: > > 1. **JDK 1.1 (no `*Spi` classes):** > In JDK 1.1, providers subclassed `MessageDigest`, `Signature`, and `KeyPairGenerator` directly. > * In [`MessageDigest`](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/security/MessageDigest.java) and `Signature`, the public API methods (`digest()`, `sign()`) were concrete methods on the base class that called `protected abstract` SPI methods (`engineDigest()`, `engineSign()`). > * [`KeyPairGenerator`](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/security/KeyPairGenerator.java) was the exception: it had no `engine*` methods. Instead, `initialize(int, SecureRandom)` and `generateKeyPair()` were `public abstract` methods directly on `KeyPairGenerator`, serving as **both** the public API and the provider SPI. > > 2. **JDK 1.2 (`*Spi` classes introduced):** > In JDK 1.2, Sun introduced [`KeyPairGeneratorSpi`](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/security/KeyPairGeneratorSpi.java), `MessageDigestSpi`, etc., so providers wouldn't have to subclass the API classes. To keep backwards compatibility with JDK 1.1 providers that already extended the API classes, each `*Spi` class was inserted as the superclass of its API class (`Object` → `KeyPairGeneratorSpi` → `KeyPairGenerator`), and a package-private/private `Delegate` subclass (`KeyPairGenerator.Delegate extends KeyPairGenerator`) was introduced to wrap modern `*Spi` instances. > * For `MessageDigest`, the concrete API method `MessageDigest.digest()` still calls `this.engineDigest()`, and `MessageDigest.Delegate` overrides `engineDigest()` to forward to `digestSpi.engineDigest()`. > * For `KeyPairGenerator`, the API method and the SPI method have the **same name** (`generateKeyPair()`, and similarly `initialize(...)`), so `KeyPairGenerator.generateKeyPair()` couldn't delegate to an `engine*` method on `this`. > * Instead, [`KeyPairGenerator.getInstance(...)`](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/security/KeyPairGenerator.java#L174-L191) always returns either: > 1. A legacy provider's direct subclass of `KeyPairGenerator` (which overrides `generateKeyPair()`—e.g., [`DSAKeyPairGenerator`](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/sun/security/provider/DSAKeyPairGenerator.java#L48) still extends `KeyPairGenerator` today so callers can cast it to `java.security.interfaces.DSAKeyPairGenerator`), or > 2. [`KeyPairGenerator.Delegate`](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/security/KeyPairGenerator.java#L568-L739) (which wraps a `KeyPairGeneratorSpi` and overrides `generateKeyPair()` at [line 721](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/security/KeyPairGenerator.java#L721) to call `spi.generateKeyPair()`). > > Because both paths override `generateKeyPair()` (and `initialize`), the base [`KeyPairGenerator.generateKeyPair()`](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/security/KeyPairGenerator.java#L514-L529) body is never called on any normal `getInstance(...)` object. (Since `KeyPairGenerator` is `abstract` anyway, JDK 1.2 could have just left `initialize` and `generateKeyPair` `abstract` as inherited from `KeyPairGeneratorSpi`, instead of giving them concrete no-op / `return null` bodies.) > > --- > > ### When can `generateKeyPair()` (and `genKeyPair()`) actually return `null`? > > 1. **Never from a built-in JDK provider via `KeyPairGenerator.getInstance(...)`:** Every built-in `KeyPairGeneratorSpi` / `KeyPairGenerator` returns a non-null `KeyPair` or throws a `RuntimeException` (like `ProviderException`). > 2. **A subclass of `KeyPairGenerator` that doesn't override `generateKeyPair()`:** > Because `KeyPairGenerator` replaced `KeyPairGeneratorSpi`'s `abstract` methods (`initialize(int, SecureRandom)` and `generateKeyPair()`) with concrete no-op / `return null` methods, **`KeyPairGenerator` is an `abstract` class with zero `abstract` methods**. > Consequently, the compiler allows a concrete subclass to omit `generateKeyPair()` entirely (for example, in a test stub/fake, or a buggy subclass): > ```java > KeyPairGenerator kpg = new KeyPairGenerator("RSA") {}; > kpg.generateKeyPair(); // returns null > kpg.genKeyPair(); // also returns null (final method that calls generateKeyPair()) > ``` > 3. **A custom provider whose `KeyPairGeneratorSpi.generateKeyPair()` explicitly returns `null`.** > No—outside of a dummy test stub, there is no defensible reason for a custom provider to return `null` from `generateKeyPair()`: > > 1. **It violates the JCA contract:** > Both [`KeyPairGenerator.generateKeyPair()`](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/security/KeyPairGenerator.java#L507-L512) and [`KeyPairGeneratorSpi.generateKeyPair()`](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/security/KeyPairGeneratorSpi.java#L112-L116) specify *"This will generate a new key pair every time it is called. @return the [newly] generated key pair"* with no provision for returning `null`. > > 2. **It breaks JCA provider failover:** > Look at [`KeyPairGenerator.Delegate.generateKeyPair()`](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/security/KeyPairGenerator.java#L721-L738): > ```java > do { > try { > return mySpi.generateKeyPair(); > } catch (RuntimeException e) { > if (failure == null) { > failure = e; > } > mySpi = nextSpi(mySpi, true); > } > } while (mySpi != null); > throw failure; > ``` > When a provider cannot generate a key pair (e.g., an HSM/PKCS#11 error, or parameters that are only discovered to be unsupported at generation time), JCA expects it to throw a `RuntimeException` (typically [`ProviderException`](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/sun/security/provider/DSAKeyPairGenerator.java#L150-L152) or `InvalidParameterException`) so that `Delegate` can automatically fail over to the next registered provider. Returning `null` would silently short-circuit failover and hand `null` to the caller. > > 3. **Even "side-effect-only" or hardware-backed generators return a `KeyPair`:** > One might wonder about hardware tokens (PKCS#11) or `AndroidKeyStore`, where the caller might primarily care about the side effect of creating the key inside the hardware keystore and where the private key material cannot be extracted. Even there, providers (such as [`P11KeyPairGenerator`](https://github.com/openjdk/jdk/blob/master/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11KeyPairGenerator.java#L334) and `AndroidKeyStoreKeyPairGeneratorSpi`) always return a non-null `KeyPair` whose `PrivateKey` is an opaque handle (where `privateKey.getEncoded()` returns `null`, not `generateKeyPair()`).
1 task
msridhar
approved these changes
Oct 8, 2026
…leNameProvider`.
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.
As in #168, I identified most of these with https://errorprone.info/bugpattern/ReturnMissingNullable, modified to try to avoid annotating internals. And again I had to revert some of the changes, and again I made some manual additions:
Removed annotations:
ThreadLocal.initialValueandCountedCompleter.getRawResult, examples of "theAtomicReferencecaseInetAddressmethods, as already documentedDecimalFormat.getDecimalFormatSymbolsandjavax.crypto.KDF(still unreachable)AbstractExecutorService.invokeAny,ForkJoinPool.invokeAny,Exchanger.exchangeCompletableFuture.resultNow, which returnsnullonly for an instance with a nullable type argument, as in similar code in Guava'sAbstractFuturequeryimplementations onjava.timemethods likeInstant.query, which are probably likeresultNowHashMap.computeIfAbsentandTreeMap.computeIfAbsentjava.security.KeyPairGenerator.generateKeyPair(), on which Gemini has tons to say belowI also made manual changes in:
RecursiveAction: I adopted@Nullable Voidthroughout and added@NullMarked.@Nullablemanually onTimeZoneNameProvider.getDisplayNamebased on its Javadoc.Providerbased on its Javadoc and on the standardMapAPI. (OK, and in the case ofService.supportsParameter, I based it on the implementation and the comments there.)@Nullableto theString commentparameter of another overload ofProperties.storeToXMLbased on its Javadoc and our precedent on the other overloads.On
java.security.KeyPairGenerator.generateKeyPair():