Skip to content

#4118 Fix collections no longer copied for JSpecify @NonNull sources #4119

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
adityaanikam wants to merge 1 commit into
base: main
Choose a base branch
from
Open

#4118 Fix collections no longer copied for JSpecify @NonNull sources #4119

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 17 additions & 10 deletions processor/src/main/java/org/mapstruct/ap/internal/model/CollectionAssignmentBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 ) ) {
Expand Down Expand Up @@ -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;
Expand All @@ -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;
}

Expand Down
3 changes: 2 additions & 1 deletion ...ruct/ap/internal/model/assignment/ExistingInstanceSetterWrapperForCollectionsAndMaps.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,8 @@ public ExistingInstanceSetterWrapperForCollectionsAndMaps(Assignment decoratedAs
thrownTypesToExclude,
targetType,
typeFactory,
fieldAssignment
fieldAssignment,
false
);
this.nvcs = nvcs;
this.nvpms = nvpms;
Expand Down
3 changes: 2 additions & 1 deletion ...mapstruct/ap/internal/model/assignment/NewInstanceSetterWrapperForCollectionsAndMaps.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ public NewInstanceSetterWrapperForCollectionsAndMaps(Assignment decoratedAssignm
thrownTypesToExclude,
targetType,
typeFactory,
fieldAssignment
fieldAssignment,
false
);
this.instanceVar = decoratedAssignment.createUniqueVarName( targetType.getName() );
}
Expand Down
14 changes: 13 additions & 1 deletion ...pstruct/ap/internal/model/assignment/SetterWrapperForCollectionsAndMapsWithNullCheck.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Type> thrownTypesToExclude,
Type targetType,
TypeFactory typeFactory,
boolean fieldAssignment) {
boolean fieldAssignment,
boolean skipNullCheck) {
super(
decoratedAssignment,
thrownTypesToExclude,
Expand All @@ -43,6 +45,7 @@ public SetterWrapperForCollectionsAndMapsWithNullCheck(Assignment decoratedAssig
this.targetType = targetType;
this.typeFactory = typeFactory;
this.newInstance = NewInstanceCreation.forType( targetType );
this.skipNullCheck = skipNullCheck;
}

@Override
Expand Down Expand Up @@ -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;
}

}
2 changes: 1 addition & 1 deletion ...apstruct/ap/internal/model/assignment/SetterWrapperForCollectionsAndMapsWithNullCheck.ftl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}.</#if>${ext.targetWriteAccessorName}<@lib.handleWrite><#if directAssignment><@wrapLocalVarInCollectionInitializer/><#else><@lib.handleWithAssignmentOrNullCheckVar/></#if></@lib.handleWrite>;
</@lib.handleLocalVarNullCheck>
</#macro>
Expand Down
7 changes: 5 additions & 2 deletions processor/src/main/resources/org/mapstruct/ap/internal/model/macro/CommonMacros.ftl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -73,13 +73,16 @@
<#nested>
</#if>
}
<#elseif skip_null_check>
<@includeModel object=nullCheckLocalVarType/> ${nullCheckLocalVarName} = <@lib.handleAssignment/>;
<#nested>
<#else>
<@includeModel object=nullCheckLocalVarType/> ${nullCheckLocalVarName} = <@lib.handleAssignment/>;
if ( ${nullCheckLocalVarName} != null ) {
<#nested>
}
</#if>
<#if ext.defaultValueAssignment?? >
<#if ext.defaultValueAssignment?? && !skip_null_check>
else {
<@handeDefaultAssigment/>
}
Expand Down
5 changes: 4 additions & 1 deletion ...xtures/org/mapstruct/ap/test/nullcheck/jspecify/JSpecifyCollectionPropertyMapperImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -19,7 +21,8 @@ public NullMarkedCollectionTargetBean map(NullMarkedCollectionSourceBean source)

NullMarkedCollectionTargetBean nullMarkedCollectionTargetBean = new NullMarkedCollectionTargetBean();

nullMarkedCollectionTargetBean.setValues( source.getValues() );
List<String> list = source.getValues();
nullMarkedCollectionTargetBean.setValues( new ArrayList<>( list ) );

return nullMarkedCollectionTargetBean;
}
Expand Down