diff --git a/processor/src/main/java/org/mapstruct/ap/internal/model/CollectionAssignmentBuilder.java b/processor/src/main/java/org/mapstruct/ap/internal/model/CollectionAssignmentBuilder.java index 5c99b85588..00f3e869d9 100644 --- a/processor/src/main/java/org/mapstruct/ap/internal/model/CollectionAssignmentBuilder.java +++ b/processor/src/main/java/org/mapstruct/ap/internal/model/CollectionAssignmentBuilder.java @@ -197,18 +197,27 @@ else if ( method.isUpdateMethod() && nvpms == IGNORE ) { method.getThrownTypes(), targetType, ctx.getTypeFactory(), - targetAccessorType.isFieldAssignment() + targetAccessorType.isFieldAssignment(), + false ); } else if ( setterWrapperNeedsSourceNullCheck( result ) && canBeMappedOrDirectlyAssigned( result ) ) { + boolean skipNullCheck = sourceJSpecifyNullability == NullabilityResolver.Nullability.NON_NULL; + if ( skipNullCheck ) { + ctx.getMessager().note( 2, + Message.PROPERTYMAPPING_JSPECIFY_SKIP_NULL_CHECK_NON_NULL_SOURCE, + targetPropertyName ); + } + result = new SetterWrapperForCollectionsAndMapsWithNullCheck( result, method.getThrownTypes(), targetType, ctx.getTypeFactory(), - targetAccessorType.isFieldAssignment() + targetAccessorType.isFieldAssignment(), + skipNullCheck ); } else if ( canBeMappedOrDirectlyAssigned( result ) ) { @@ -273,14 +282,6 @@ private boolean canBeMappedOrDirectlyAssigned(Assignment result) { * @return whether to include a null / presence check or not */ private boolean setterWrapperNeedsSourceNullCheck(Assignment rhs) { - // JSpecify: source @NonNull means the value is guaranteed non-null, skip the wrapper - if ( sourceJSpecifyNullability == NullabilityResolver.Nullability.NON_NULL ) { - ctx.getMessager().note( 2, - Message.PROPERTYMAPPING_JSPECIFY_SKIP_NULL_CHECK_NON_NULL_SOURCE, - targetPropertyName ); - return false; - } - if ( rhs.getSourcePresenceCheckerReference() != null ) { // If there is a source presence check then we should do a null check return true; @@ -292,6 +293,12 @@ private boolean setterWrapperNeedsSourceNullCheck(Assignment rhs) { } if ( rhs.getType().isDirect() ) { + // Direct assignment always needs the local-var + copy-constructor wrapper, even when + // the source is JSpecify @NonNull and the runtime null check itself can be skipped + // (see the skipNullCheck flag passed to SetterWrapperForCollectionsAndMapsWithNullCheck + // below). Skipping this wrapper entirely for @NonNull sources would also skip the + // defensive copy, silently changing copy() from returning a mutable collection to + // sharing the source's collection instance. return true; } diff --git a/processor/src/main/java/org/mapstruct/ap/internal/model/assignment/ExistingInstanceSetterWrapperForCollectionsAndMaps.java b/processor/src/main/java/org/mapstruct/ap/internal/model/assignment/ExistingInstanceSetterWrapperForCollectionsAndMaps.java index ce347321c9..6dcb594d7d 100644 --- a/processor/src/main/java/org/mapstruct/ap/internal/model/assignment/ExistingInstanceSetterWrapperForCollectionsAndMaps.java +++ b/processor/src/main/java/org/mapstruct/ap/internal/model/assignment/ExistingInstanceSetterWrapperForCollectionsAndMaps.java @@ -50,7 +50,8 @@ public ExistingInstanceSetterWrapperForCollectionsAndMaps(Assignment decoratedAs thrownTypesToExclude, targetType, typeFactory, - fieldAssignment + fieldAssignment, + false ); this.nvcs = nvcs; this.nvpms = nvpms; diff --git a/processor/src/main/java/org/mapstruct/ap/internal/model/assignment/NewInstanceSetterWrapperForCollectionsAndMaps.java b/processor/src/main/java/org/mapstruct/ap/internal/model/assignment/NewInstanceSetterWrapperForCollectionsAndMaps.java index f0e23470a7..02dea5e192 100644 --- a/processor/src/main/java/org/mapstruct/ap/internal/model/assignment/NewInstanceSetterWrapperForCollectionsAndMaps.java +++ b/processor/src/main/java/org/mapstruct/ap/internal/model/assignment/NewInstanceSetterWrapperForCollectionsAndMaps.java @@ -32,7 +32,8 @@ public NewInstanceSetterWrapperForCollectionsAndMaps(Assignment decoratedAssignm thrownTypesToExclude, targetType, typeFactory, - fieldAssignment + fieldAssignment, + false ); this.instanceVar = decoratedAssignment.createUniqueVarName( targetType.getName() ); } diff --git a/processor/src/main/java/org/mapstruct/ap/internal/model/assignment/SetterWrapperForCollectionsAndMapsWithNullCheck.java b/processor/src/main/java/org/mapstruct/ap/internal/model/assignment/SetterWrapperForCollectionsAndMapsWithNullCheck.java index 8ef71f7c0d..0d96e49eef 100644 --- a/processor/src/main/java/org/mapstruct/ap/internal/model/assignment/SetterWrapperForCollectionsAndMapsWithNullCheck.java +++ b/processor/src/main/java/org/mapstruct/ap/internal/model/assignment/SetterWrapperForCollectionsAndMapsWithNullCheck.java @@ -28,12 +28,14 @@ public class SetterWrapperForCollectionsAndMapsWithNullCheck extends WrapperForC private final Type targetType; private final TypeFactory typeFactory; private final NewInstanceCreation newInstance; + private final boolean skipNullCheck; public SetterWrapperForCollectionsAndMapsWithNullCheck(Assignment decoratedAssignment, List thrownTypesToExclude, Type targetType, TypeFactory typeFactory, - boolean fieldAssignment) { + boolean fieldAssignment, + boolean skipNullCheck) { super( decoratedAssignment, thrownTypesToExclude, @@ -43,6 +45,7 @@ public SetterWrapperForCollectionsAndMapsWithNullCheck(Assignment decoratedAssig this.targetType = targetType; this.typeFactory = typeFactory; this.newInstance = NewInstanceCreation.forType( targetType ); + this.skipNullCheck = skipNullCheck; } @Override @@ -72,4 +75,13 @@ public boolean isEnumSet() { return targetType.isEnumSet(); } + /** + * @return {@code true} if the source is statically known to be non-null (e.g. via JSpecify + * {@code @NonNull}), so the local variable should still be declared and wrapped in the copy + * constructor, but no runtime {@code if ( x != null )} check needs to be emitted around it. + */ + public boolean isSkipNullCheck() { + return skipNullCheck; + } + } diff --git a/processor/src/main/resources/org/mapstruct/ap/internal/model/assignment/SetterWrapperForCollectionsAndMapsWithNullCheck.ftl b/processor/src/main/resources/org/mapstruct/ap/internal/model/assignment/SetterWrapperForCollectionsAndMapsWithNullCheck.ftl index 7fad115bc4..c3394f5b2b 100644 --- a/processor/src/main/resources/org/mapstruct/ap/internal/model/assignment/SetterWrapperForCollectionsAndMapsWithNullCheck.ftl +++ b/processor/src/main/resources/org/mapstruct/ap/internal/model/assignment/SetterWrapperForCollectionsAndMapsWithNullCheck.ftl @@ -15,7 +15,7 @@ assigns the target via the regular target write accessor (usually the setter) --> <#macro callTargetWriteAccessor> - <@lib.handleLocalVarNullCheck needs_explicit_local_var=directAssignment> + <@lib.handleLocalVarNullCheck needs_explicit_local_var=directAssignment skip_null_check=skipNullCheck> <#if ext.targetBeanName?has_content>${ext.targetBeanName}.${ext.targetWriteAccessorName}<@lib.handleWrite><#if directAssignment><@wrapLocalVarInCollectionInitializer/><#else><@lib.handleWithAssignmentOrNullCheckVar/>; diff --git a/processor/src/main/resources/org/mapstruct/ap/internal/model/macro/CommonMacros.ftl b/processor/src/main/resources/org/mapstruct/ap/internal/model/macro/CommonMacros.ftl index 0599e8c0e4..873fce4ea8 100644 --- a/processor/src/main/resources/org/mapstruct/ap/internal/model/macro/CommonMacros.ftl +++ b/processor/src/main/resources/org/mapstruct/ap/internal/model/macro/CommonMacros.ftl @@ -60,7 +60,7 @@ requires: caller to implement String:getNullCheckLocalVarName() caller to implement Type:getNullCheckLocalVarType() --> -<#macro handleLocalVarNullCheck needs_explicit_local_var> +<#macro handleLocalVarNullCheck needs_explicit_local_var skip_null_check=false> <#if sourcePresenceCheckerReference??> if ( <@includeModel object=sourcePresenceCheckerReference targetType=ext.targetType @@ -73,13 +73,16 @@ <#nested> } + <#elseif skip_null_check> + <@includeModel object=nullCheckLocalVarType/> ${nullCheckLocalVarName} = <@lib.handleAssignment/>; + <#nested> <#else> <@includeModel object=nullCheckLocalVarType/> ${nullCheckLocalVarName} = <@lib.handleAssignment/>; if ( ${nullCheckLocalVarName} != null ) { <#nested> } - <#if ext.defaultValueAssignment?? > + <#if ext.defaultValueAssignment?? && !skip_null_check> else { <@handeDefaultAssigment/> } diff --git a/processor/src/test/resources/fixtures/org/mapstruct/ap/test/nullcheck/jspecify/JSpecifyCollectionPropertyMapperImpl.java b/processor/src/test/resources/fixtures/org/mapstruct/ap/test/nullcheck/jspecify/JSpecifyCollectionPropertyMapperImpl.java index 4004bc60fe..2b1ec145c4 100644 --- a/processor/src/test/resources/fixtures/org/mapstruct/ap/test/nullcheck/jspecify/JSpecifyCollectionPropertyMapperImpl.java +++ b/processor/src/test/resources/fixtures/org/mapstruct/ap/test/nullcheck/jspecify/JSpecifyCollectionPropertyMapperImpl.java @@ -5,6 +5,8 @@ */ package org.mapstruct.ap.test.nullcheck.jspecify; +import java.util.ArrayList; +import java.util.List; import javax.annotation.processing.Generated; @Generated( @@ -19,7 +21,8 @@ public NullMarkedCollectionTargetBean map(NullMarkedCollectionSourceBean source) NullMarkedCollectionTargetBean nullMarkedCollectionTargetBean = new NullMarkedCollectionTargetBean(); - nullMarkedCollectionTargetBean.setValues( source.getValues() ); + List list = source.getValues(); + nullMarkedCollectionTargetBean.setValues( new ArrayList<>( list ) ); return nullMarkedCollectionTargetBean; }