Conversation
hduelme
marked this pull request as draft
September 19, 2026 16:13
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.
Implementation
The PR adds nullability as a state of type instances. It represents whether a return type, parameter or field can be
nullor whethernullcan be considered as a value.How nullability is used by MapStruct:
NullabilityResolveris made accessible inDefaultModelElementProcessorContextfor all processors. TheNullabilityResolveris the main point for determining nullability.SourceMethods is discovered and stored inMethodRetrievalProcessor.MapperCreationProcessorcreates the mappings according to the nullability values discovered in step 2.NullabilityAnnotationProcessor. I have decided to always add them if available, to ensure the highest possible compatibility with the checking frameworks. The way they are added is a reverse resolve approach (resolve outside-in). First the nullability scope of the target package is determined. Annotations from the base mapper are then inherited if they do not match the outer nullability scope. Finally@Nullable/@NonNullis added if necessary in its scope. This closes Decide whether or not we should mark generated code with JSpecify annotations #4036 and JSpecify: Propagate nullability annotations to mapper implementation #4076. This is an alternative solution to #4076 Propagate JSpecify nullability annotations to the mapper implementation #4112In order to match the specified nullability requirements as often as possible, the built-in methods were modified so that they no longer accept and return
nullas a value.The exceptions are the following, as they can also returnnullunder other conditions:JaxbElemToValueXmlGregorianCalendarToJodaDateTimeXmlGregorianCalendarToJodaLocalDateXmlGregorianCalendarToJodaLocalDateTimeXmlGregorianCalendarToJodaLocalTimeXmlGregorianCalendarToLocalDateTimeIn addition the new strict matching of nullability requirements causes some changes:
SetterWrapper)NullSafe2StepMappingMethodwas added.IterableMappingMethodmight null-check inside the loop. Breaking change the current implementation will filter null values if the used method does not support themStreamMappingMethodmight null-check inside the stream. Breaking change the current implementation will filter null values if the used method does not support them.MapMappingMethodmight null-check inside the loop for map keys and/or map values. Breaking change the current implementation will filter null values if the used method does not support themValueMappingcan now skip the null-check.ValueMappingnow warns if null can't be used as a valid sourceValueMappingnow checks if the return type is@NonNulland fails ifnullcan be returned.needsParameterNullCheck()was added toAssignment. With this, JSpecify: Missing null check before reusing non-nullable method #4077, JSpecify: False positive error for nullable source with nullable-param non-null-return method #4086 and JSpecify: Missing compile error when reused method returns@Nullableto@NonNullconstructor #4081 are closed. Be able to generate the nullable mapping from the NotNull mapping #4106 is also closed. Thanks to @RaniAgus for providing the tests. This is an alternative solution to Honor JSpecify nullability of reused mapping methods #4110 and #4077 Add missing null check before reusing non-nullable methods #4078.NumberFormatExceptionfornullvalues.As mentioned some changes are breaking and should be discussed or at least mentioned in the release notes.
Testing
Some tests are worth mentioning.
For built-in methods I added
JspecifyBuildInTest. Here I wanted three types of tests for each conversion. One where the source values are@NonNull, one where the target values are@NonNulland one where both are@NonNull. Instead of tripling the test classes, I decided to move them into a package and use@WithPackageInfoto mark them as nullable or non nullable. While testing this I discovered #4132. That's why I only added fixture comparison tests.@NullMarkedcan also be applied at module scope, instead of marking the whole processor tests with@NullMarkedI decided to create a dedicated integration test for it.JSpecifyContainerPropertyTestnow checks if collections/maps/streams are null-checked if needed. Thanks to @zanarellidev for finding this. Alternative solution to #4103In the integration test I found two JDK8 bugs resulting in the exclusion of some JSpecify tests from
fullFeatureTest()DestinationClassNameTest/DestinationPackageNameTestnow always run withmapstruct.disableJSpecify. The problem is that eclipse 1.6 has an issue when resolving a non-existing package and creating it afterward.Other
I left all commits in here for now. If I should squash them, I will do so.