Add nullability to MapStruct model - #4131

This makes every conversion require a null check when the source nullability is unknown, which means that for users without JSpecify null elements are now silently dropped from iterables, streams and maps.

public interface Demo {
    ListsTgt map(Lists l); // List<Integer> -> List<String>, Map<Integer, Integer> -> Map<String, String>
}

main:

list1.add( String.valueOf( integer ) );
...
String key = String.valueOf( entry.getKey() );
String value = String.valueOf( entry.getValue() );
map1.put( key, value );

This PR:

if ( integer != null ) {
    list1.add( String.valueOf( integer ) );
}
...
if ( entry.getKey() != null ) {
    String key = String.valueOf( entry.getKey() );
    if ( entry.getValue() != null ) {
        String value = String.valueOf( entry.getValue() );
        map1.put( key, value );
    }
}

So the list size changes and a map entry with a valid key and a null value disappears. There is no warning either.

I think the element null check should rely on the nullability: only filter when we know the element can't be passed (e.g. the target / parameter is JSpecify @NonNull), and keep the current behaviour when the nullability is unknown. Then the TODOs in IterableMappingMethod.ftl, MapMappingMethod.ftl and StreamMappingMethod.ftl can be resolved as well. For the map case I'd also not drop the entry when only the value is null.

methodRefX.getSourceNullability().isNullable() is true for every user method without JSpecify, so every method -> conversion chain now goes through a NullSafe2StepMappingMethod. This causes a couple of problems for plain mappers:

Overloaded methods produce code that does not compile

@Mapper
public interface Overload {
    default Integer map(A a) { return 1; }
    default Integer map(B b) { return 2; }
    HolderT map(Holder h); // Holder { A a; B b; } -> HolderT { String a; String b; }
}

OverloadImpl.java:21: error: incompatible types: B cannot be converted to A
        holderT.setB( mapToString( h.getB() ) );

Both helpers are named mapToString and SupportingMappingMethod#equals only compares the name, so one of them is dropped. The same happens when the same user method is used with two different numberFormat / dateFormat values, in which case it compiles but one property uses the format of the other one.

@Context parameters are lost

@Mapper
public interface Ctx {
    default Integer toInt(A a, @Context StringBuilder log) { return 1; }
    HolderT map(Holder h, @Context StringBuilder log);
}

CtxImpl.java:26: error: cannot find symbol
        Integer integer = toInt( a, log );
  symbol:   variable log

Local variable can clash with the parameter name, as the variable is named decapitalize(sourceType.getName()) and existingVariableNames does not contain the parameter name (e.g. Calendar toCalendar(MethodBuiltInInput methodBuiltInInput)).

Primitive targets now get 0 / false, see the comment on NullSafe2StepMappingMethod.ftl.

Suggestion: only create the helper when we know that X can return null (JSpecify @Nullable) and Y does not accept null, otherwise keep the inline chain as on main. The helper name needs to be unique (reserved names / existing methods) and NullSafe2StepMappingMethod should have a proper equals / hashCode, and it needs to keep all parameters of X (context, target type, etc.).

For primitive targets this returns the default value, which changes the behaviour for plain mappers:

@Named("toInt")
default Integer toInt(String s) { return s == null ? null : Integer.valueOf( s ); }

@Mapping(target = "number", source = "name", qualifiedByName = "toInt") // long number
void update(Src src, @MappingTarget Tgt tgt);

main:

if ( src.getName() != null ) {
    tgt.setNumber( toInt( src.getName() ).longValue() );
}

This PR:

tgt.setNumber( toIntTolong( src.getName() ) );

private long toIntTolong(String s) {
    Integer integer = toInt( s );
    if (integer == null) {
        return 0L;
    }
    return integer.longValue();
}

Apart from gating this on known nullability (previous comment), I think the helper should return the boxed type and let the existing setter null handling (NullValuePropertyMappingStrategy, etc.) decide what to do, instead of picking a default value.

Removing the null guard from the forged subclass method is fine, as all callers from the subclass mapping guarantee a non null value. However, Parameter#equals (and therefore ForgedMethod / MappingMethod equality) does not consider the nullability. AbstractBaseBuilder#createForgedAssignment (line 125) then reuses an existing method without the guard for a caller that can pass null, and the caller does not add a check either.

@Mapper
public interface Reuse {
    @BeanMapping(subclassExhaustiveStrategy = SubclassExhaustiveStrategy.RUNTIME_EXCEPTION)
    @SubclassMapping(source = Source1.class, target = Target1.class)
    AbstractTarget map(AbstractSource s);

    Holder map(HolderSource h); // HolderSource { Source1 child; } -> Holder { Target1 child; }
}

This PR:

holder.setChild( source1ToTarget1( h.getChild() ) );

protected Target1 source1ToTarget1(Source1 source1) {

    Target1 target1 = new Target1();

    target1.setName( source1.getName() );
    ...

map(new HolderSource()) throws an NPE, on main the if ( source1 == null ) return null; is there. The same can happen with JSpecify when a @NonNull Foo and a @Nullable Foo property map to the same type: whichever comes first decides whether the shared method has a guard.

I think the nullability of the parameters (and the return type) needs to be part of the identity when reusing forged methods.

The built-in methods no longer do the null check themselves, but the array branch above (line 67) does not use handleVariableNullCheck like the collection branch here:

@Mapper
public interface Arr {
    ZonedDateTime[] map(Calendar[] in);
}

for ( Calendar calendar : in ) {
    zonedDateTimeTmp[i] = calendarToZonedDateTime( calendar );
    i++;
}
...
private ZonedDateTime calendarToZonedDateTime(Calendar cal) {
    return ZonedDateTime.ofInstant( cal.toInstant(), cal.getTimeZone().toZoneId() );
}

A null element now throws an NPE, on main it resulted in a null slot. It would be good to check the other call sites of built-in methods for the same thing, and to add runtime tests with null values for the built-ins (not only fixtures).

With JSpecify, @Nullable String[] means that the elements are nullable, and String @Nullable [] means that the array itself is nullable. Recursing into the component type reads the element nullability as the nullability of the array, so in a @NullMarked scope String @Nullable [] getValues() is treated as non null and the null check is skipped.

The same applies to Type.ftl line 17, which generates @NonNull String[] (annotating the elements) instead of String @NonNull []. JSpecifyNonNullReturnArrayMapper uses @Nullable NullMarkedSourceBean[] for a nullable array, so the test would need to be adjusted as well. It would be good to have tests for both forms.

Shouldn't this check method.getReturnTypeNullability().isNonNullable() instead of sourceParameterNullability.isNonNullable()? The message says that the return type is @NonNull, and the check at line 429 uses the return type nullability. With the current check a @Nullable source with a @NonNull return type and <ANY_REMAINING> / <ANY_UNMAPPED> mapped to <NULL> is not reported, and the error is reported for a nullable return type when the source is @NonNull.

The messages use source = "NULL", but the constant is MappingConstants.NULL ("<NULL>"). Someone copying the message would end up mapping an enum constant named NULL. Small things as well: source =\"%s\" is missing a space, and the message on line 225 could mention JSpecify / @NonNull like the others. Something like:

• 223: JSpecify: value mapping method has a @NonNull return type, but a null source would be mapped to null. Define @ValueMapping(source = MappingConstants.NULL, target = ...) or implement a custom EnumMappingStrategy.
• 224: JSpecify: value mapping method has a @NonNull return type, but source = "%s" is mapped to target = "<NULL>".
• 225: Source parameter is @NonNull, so @ValueMapping(source = "<NULL>") can never be applied and is ignored.

When JSpecify is on the classpath (which can happen transitively through Guava, Spring 7, etc.), mappers that don't use JSpecify at all now get @NonNull on built-in helper parameters, forged subclass parameters and RETURN_DEFAULT methods, e.g.

protected Target1 source1ToTarget1(@NonNull Source1 source1)
private Calendar zonedDateTimeToCalendar(@NonNull ZonedDateTime dateTime)

I'm fine with this being the default, but we need to:

• mention it in the release notes
• update the docs for mapstruct.disableJSpecify (chapter-2-set-up.asciidoc and chapter-10-advanced-mapping-options.asciidoc), they currently say that disabling falls back to the pre JSpecify behaviour. That should mention the generated annotations, and it should stay true for the other changes (which is another reason to gate them on known nullability)
• add a test for it. The jspecify jar is excluded from the processor test classpath, so nothing covers a plain mapper with JSpecify available, and the same mapper with mapstruct.disableJSpecify=true