Skip to content

Fix #4030 remove double getters in null checks #4064

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
stefanos-kalantzis wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix #4030 remove double getters in null checks #4064

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
42 changes: 19 additions & 23 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 @@ -37,7 +37,7 @@

/**
* A builder that is used for creating an assignment to a collection.
*
* <p>
* The created assignments to the following null checks:
* <ul>
* <li>source-null-check - For this the {@link SetterWrapperForCollectionsAndMapsWithNullCheck} is used when a
Expand All @@ -49,7 +49,7 @@
* <li>local-var-null-check - Done in {@link ExistingInstanceSetterWrapperForCollectionsAndMaps}, and
* {@link SetterWrapperForCollectionsAndMapsWithNullCheck}</li>
* </ul>
*
* <p>
* A local-var-null-check is needed in the following cases:
*
* <ul>
Expand Down Expand Up @@ -106,7 +106,6 @@ public CollectionAssignmentBuilder targetAccessorType(AccessorType targetAccesso

/**
* @param assignment the assignment that needs to be invoked
*
* @return this builder for chaining
*/
public CollectionAssignmentBuilder assignment(Assignment assignment) {
Expand All @@ -116,20 +115,19 @@ public CollectionAssignmentBuilder assignment(Assignment assignment) {

/**
* @param sourceRHS the source right hand side for getting the property for mapping
*
* @return this builder for chaining
*/
public CollectionAssignmentBuilder rightHandSide(SourceRHS sourceRHS) {
this.sourceRHS = sourceRHS;
return this;
}

public CollectionAssignmentBuilder nullValueCheckStrategy( NullValueCheckStrategyGem nvcs ) {
public CollectionAssignmentBuilder nullValueCheckStrategy(NullValueCheckStrategyGem nvcs) {
this.nvcs = nvcs;
return this;
}

public CollectionAssignmentBuilder nullValuePropertyMappingStrategy( NullValuePropertyMappingStrategyGem nvpms ) {
public CollectionAssignmentBuilder nullValuePropertyMappingStrategy(NullValuePropertyMappingStrategyGem nvpms) {
this.nvpms = nvpms;
return this;
}
Expand Down Expand Up @@ -178,19 +176,12 @@ else if ( method.isUpdateMethod() && !targetImmutable ) {
ctx.getTypeFactory(),
targetAccessorType.isFieldAssignment()
);
if ( result.getSourceType().isPrimitive() ) {

@filiphr filiphr Jun 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In the mutable-collection update path (ExistingInstanceSetterWrapperForCollectionsAndMaps), the local var is only reset for primitive source types. So a List<String> fed by a hasX() presence check keeps the hoisted local var and is read unconditionally (this is the exact branch the _913 update mappers hit). If the fix in presenceCheckAllowsLocalVar lands, this primitive-only reset may be redundant — but as it stands today it doesn't cover the regression.

result.setSourceLocalVarName( null );
}
Comment on lines +179 to +181

@hduelme hduelme Jun 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why are primitives excluded?

}
else if ( method.isUpdateMethod() && nvpms == IGNORE ) {

result = new SetterWrapperForCollectionsAndMapsWithNullCheck(
result,
method.getThrownTypes(),
targetType,
ctx.getTypeFactory(),
targetAccessorType.isFieldAssignment()
);
}
else if ( setterWrapperNeedsSourceNullCheck( result )
&& canBeMappedOrDirectlyAssigned( result ) ) {
else if ( method.isUpdateMethod() && nvpms == IGNORE
|| setterWrapperNeedsSourceNullCheck( result ) && canBeMappedOrDirectlyAssigned( result ) ) {

result = new SetterWrapperForCollectionsAndMapsWithNullCheck(
result,
Expand All @@ -199,6 +190,7 @@ && canBeMappedOrDirectlyAssigned( result ) ) {
ctx.getTypeFactory(),
targetAccessorType.isFieldAssignment()
);
result.setSourceLocalVarName( null );
}
else if ( canBeMappedOrDirectlyAssigned( result ) ) {
//TODO init default value
Expand All @@ -210,14 +202,17 @@ else if ( canBeMappedOrDirectlyAssigned( result ) ) {
targetType,
targetAccessorType.isFieldAssignment()
);
result.setSourceLocalVarName( null );
}
else if ( hasNoArgsConstructor() ) {
result = new NewInstanceSetterWrapperForCollectionsAndMaps(
result,
method.getThrownTypes(),
targetType,
ctx.getTypeFactory(),
targetAccessorType.isFieldAssignment() );
targetAccessorType.isFieldAssignment()
);
result.setSourceLocalVarName( null );
}
else {
ctx.getMessager().printMessage(
Expand All @@ -244,15 +239,16 @@ else if ( hasNoArgsConstructor() ) {
nvpms,
targetAccessorType.isFieldAssignment()
);
result.setSourceLocalVarName( null );
}

return result;
}

private boolean canBeMappedOrDirectlyAssigned(Assignment result) {
return result.getType() != AssignmentType.DIRECT
|| hasCopyConstructor()
|| targetType.isEnumSet();
|| hasCopyConstructor()
|| targetType.isEnumSet();
}

/**
Expand Down Expand Up @@ -294,8 +290,8 @@ private boolean checkConstructorForPredicate(Predicate<Element> predicate) {
}
else {
Element sourceElement = targetType.getImplementationType() != null
? targetType.getImplementationType().getTypeElement()
: targetType.getTypeElement();
? targetType.getImplementationType().getTypeElement()
: targetType.getTypeElement();
if ( sourceElement != null ) {
for ( Element element : sourceElement.getEnclosedElements() ) {
if ( element.getKind() == ElementKind.CONSTRUCTOR
Expand Down
17 changes: 17 additions & 0 deletions processor/src/main/java/org/mapstruct/ap/internal/model/PropertyMapping.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import org.mapstruct.ap.internal.model.common.FormattingParameters;
import org.mapstruct.ap.internal.model.common.ModelElement;
import org.mapstruct.ap.internal.model.common.Parameter;
import org.mapstruct.ap.internal.model.common.ParameterBinding;
import org.mapstruct.ap.internal.model.common.PresenceCheck;
import org.mapstruct.ap.internal.model.common.SourceRHS;
import org.mapstruct.ap.internal.model.common.Type;
Expand Down Expand Up @@ -660,6 +661,11 @@ private boolean hasDefaultValueOrDefaultExpression() {
private Assignment assignToPlainViaAdder( Assignment rightHandSide) {

Assignment result = rightHandSide;
if ( sourceReference == null || !sourceReference.isNested() ) {
// For non-nested properties, reset the local var name: the adder wrapper handles

@filiphr filiphr Jun 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Minor: the comment says "For non-nested properties", but the guard also covers sourceReference == null (expression/constant mappings). Consider "For non-nested (or absent) source references". Optionally note why nested is excluded — nested references keep their local var to avoid re-invoking the forged accessor method.

// the source reference inline without needing a local variable.
result.setSourceLocalVarName( null );
}

String adderIteratorName = sourcePropertyName == null ? targetPropertyName : sourcePropertyName;
if ( result.getSourceType().isIterableType() ) {
Expand Down Expand Up @@ -767,6 +773,9 @@ else if ( !sourceReference.isNested() ) {
sourceReference,
sourceRHS
) );
if ( presenceCheckAllowsLocalVar( sourceRHS.getSourcePresenceCheckerReference() ) ) {

@filiphr filiphr Jun 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is where the local var gets set for simple (non-nested) properties. Because presenceCheckAllowsLocalVar returns true for SuffixPresenceCheck (see comment on the method below), a hasX()-guarded property ends up with an unconditionally-evaluated local var. The _913 fixtures show the effect.

sourceRHS.setSourceLocalVarName( sourceRHS.createUniqueVarName( propertyEntry.getName() ) );
}
return sourceRHS;
}
// nested property given as dot path
Expand Down Expand Up @@ -821,6 +830,14 @@ else if ( !sourceReference.isNested() ) {
}
}

private boolean presenceCheckAllowsLocalVar(PresenceCheck presenceCheck) {

@filiphr filiphr Jun 8, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🔴 Root cause of the unconditional-getter regression. This guard only excludes MethodReferencePresenceCheck; every other presence-check type (notably SuffixPresenceCheck, i.e. the hasX()/isSetX() idiom) falls through to return true, so a local var is created and the getter is hoisted above the if (source.hasX()) guard — executing it even when the check is false.

The principle here is right ("only hoist when the presence check consumes the source RHS value"), but it's enforced incompletely. Suggest returning false for presence checks that don't read the RHS — SuffixPresenceCheck, and please review JavaExpressionPresenceCheck plus Negate/All/Any wrappers around non-RHS checks. Local-var hoisting is safe/beneficial only for NullPresenceCheck, value-based OptionalPresenceCheck, and MethodReferencePresenceCheck bound to the source RHS.

Minor: this method encodes the subtlest decision in the PR — worth a short comment explaining the intent.

if ( presenceCheck instanceof MethodReferencePresenceCheck ) {
return ((MethodReferencePresenceCheck) presenceCheck).getMethodReference().getParameterBindings()
.stream().anyMatch( ParameterBinding::isForSourceRhs );
}
return true;
}

Comment on lines +833 to +840

@hduelme hduelme Jun 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What is checked here?

private PresenceCheck getSourcePresenceCheckerRef(SourceReference sourceReference,
SourceRHS sourceRHS) {

Expand Down
8 changes: 8 additions & 0 deletions processor/src/main/java/org/mapstruct/ap/internal/model/TypeConversion.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,14 @@ public TypeConversion( Set<Type> importTypes,

@Override
public Set<Type> getImportTypes() {
if ( assignment != null ) {
Set<Type> assignmentImports = assignment.getImportTypes();
if ( !assignmentImports.isEmpty() ) {
Set<Type> combined = new HashSet<>( importTypes );
combined.addAll( assignmentImports );
return combined;
}
}
return importTypes;
}

Expand Down
4 changes: 3 additions & 1 deletion processor/src/main/java/org/mapstruct/ap/internal/model/assignment/ArrayCopyWrapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,9 @@ public ArrayCopyWrapper(Assignment rhs,
super( rhs, fieldAssignment );
this.arraysType = arraysType;
this.targetType = targetType;
rhs.setSourceLocalVarName( rhs.createUniqueVarName( targetPropertyName ) );
if ( rhs.getSourceLocalVarName() == null ) {
rhs.setSourceLocalVarName( rhs.createUniqueVarName( targetPropertyName ) );
}
this.setExplicitlyToDefault = setExplicitlyToDefault;
this.setExplicitlyToNull = setExplicitlyToNull;
}
Expand Down
17 changes: 13 additions & 4 deletions processor/src/main/java/org/mapstruct/ap/internal/model/common/SourceRHS.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.stream.Stream;
Expand All @@ -17,7 +18,7 @@

/**
* SourceRHS Assignment. Right Hand Side (RHS), source part of the assignment.
*
* <p>
* This class contains all information on the source side of an assignment needed for common use in the mapping.
*
* @author Sjaak Derksen
Expand All @@ -35,12 +36,12 @@ public class SourceRHS extends ModelElement implements Assignment {
private final String sourceParameterName;

public SourceRHS(String sourceReference, Type sourceType, Set<String> existingVariableNames,
String sourceErrorMessagePart ) {
String sourceErrorMessagePart) {
this( sourceReference, sourceReference, null, sourceType, existingVariableNames, sourceErrorMessagePart );
}

public SourceRHS(String sourceParameterName, String sourceReference, PresenceCheck sourcePresenceCheckerReference,
Type sourceType, Set<String> existingVariableNames, String sourceErrorMessagePart ) {
Type sourceType, Set<String> existingVariableNames, String sourceErrorMessagePart) {
this.sourceReference = sourceReference;
this.sourceType = sourceType;
this.existingVariableNames = existingVariableNames;
Expand Down Expand Up @@ -100,6 +101,14 @@ public void setSourceLoopVarName(String sourceLoopVarName) {

@Override
public Set<Type> getImportTypes() {
if ( sourceLocalVarName != null ) {
Set<Type> imports = new HashSet<>();
imports.add( sourceType );
if ( sourcePresenceCheckerReference != null ) {
imports.addAll( sourcePresenceCheckerReference.getImportTypes() );
}
return imports;
}
if ( sourcePresenceCheckerReference != null ) {
return sourcePresenceCheckerReference.getImportTypes();
}
Expand All @@ -113,7 +122,7 @@ public List<Type> getThrownTypes() {
}

@Override
public void setAssignment( Assignment assignment ) {
public void setAssignment(Assignment assignment) {
throw new UnsupportedOperationException( "Not supported." );
}

Expand Down
5 changes: 3 additions & 2 deletions .../test/resources/fixtures/21/org/mapstruct/ap/test/bugs/_3591/ContainerBeanMapperImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,9 @@ public ContainerBeanDto mapWithMapMapping(ContainerBean containerBean, Container
return containerBeanDto;
}

Map<String, ContainerBean> beanMap = containerBean.getBeanMap();
if ( containerBeanDto.getBeanMap() != null ) {
Map<String, ContainerBeanDto> map = stringContainerBeanMapToStringContainerBeanDtoMap( containerBean.getBeanMap() );
Map<String, ContainerBeanDto> map = stringContainerBeanMapToStringContainerBeanDtoMap( beanMap );
if ( map != null ) {
containerBeanDto.getBeanMap().clear();
containerBeanDto.getBeanMap().putAll( map );
Expand All @@ -34,7 +35,7 @@ public ContainerBeanDto mapWithMapMapping(ContainerBean containerBean, Container
}
}
else {
Map<String, ContainerBeanDto> map = stringContainerBeanMapToStringContainerBeanDtoMap( containerBean.getBeanMap() );
Map<String, ContainerBeanDto> map = stringContainerBeanMapToStringContainerBeanDtoMap( beanMap );
if ( map != null ) {
containerBeanDto.setBeanMap( map );
}
Expand Down
Loading
Loading