fix: prefer coalesce operator over if-else to simplify generated source code (#1262)

Author: latonz

src/Riok.Mapperly/Descriptors/Mappings/NullDelegateMapping.cs

@@ -48,29 +48,50 @@ public override ExpressionSyntax Build(TypeMappingBuildContext ctx)
         if (!SourceType.IsNullable())
         {
             // if the target type is a nullable value type, there needs to be an additional cast in some cases
-            // (eg. in a linq expression, int => int?)
+            // (e.g. in a linq expression, int => int?)
             return TargetType.IsNullableValueType()
                 ? CastExpression(FullyQualifiedIdentifier(TargetType), _delegateMapping.Build(ctx))
                 : _delegateMapping.Build(ctx);
         }
 
         // source is nullable and the mapping method cannot handle nulls,
         // call mapping only if source is not null.
+        // with coalesce: Map(source ?? throw)
+        // or with if-else:
         // source == null ? <null-substitute> : Map(source)
         // or for nullable value types:
         // source == null ? <null-substitute> : Map(source.Value)
-        var sourceValue = SourceType.IsNullableValueType() ? MemberAccess(ctx.Source, NullableValueProperty) : ctx.Source;
+        var nullSubstitute = NullSubstitute(TargetType, ctx.Source, _nullFallbackValue);
+
+        // if throw is used instead of a default value
+        // and the delegate mapping is a synthetic or method mapping
+        // the coalesce operator can be used
+        // (the Map method isn't invoked in the null case since the exception is thrown,
+        // and it is ensured no parentheses are needed since it is directly used or is passed as argument).
+        // This simplifies the generated source code.
+        if (
+            _nullFallbackValue == NullFallbackValue.ThrowArgumentNullException
+            && (_delegateMapping.IsSynthetic || _delegateMapping is MethodMapping)
+        )
+        {
+            ctx = ctx.WithSource(Coalesce(ctx.Source, nullSubstitute));
+            return _delegateMapping.Build(ctx);
+        }
+
+        var nonNullableSourceValue = ctx.Source;
 
         // disable nullable waring if accessing array
-        if (sourceValue is ElementAccessExpressionSyntax)
+        if (nonNullableSourceValue is ElementAccessExpressionSyntax)
+        {
+            nonNullableSourceValue = PostfixUnaryExpression(SyntaxKind.SuppressNullableWarningExpression, nonNullableSourceValue);
+        }
+
+        // if it is a value type, access the value property
+        if (SourceType.IsNullableValueType())
         {
-            sourceValue = PostfixUnaryExpression(SyntaxKind.SuppressNullableWarningExpression, sourceValue);
+            nonNullableSourceValue = MemberAccess(nonNullableSourceValue, NullableValueProperty);
         }
 
-        return Conditional(
-            IsNull(ctx.Source),
-            NullSubstitute(TargetType, ctx.Source, _nullFallbackValue),
-            _delegateMapping.Build(ctx.WithSource(sourceValue))
-        );
+        return Conditional(IsNull(ctx.Source), nullSubstitute, _delegateMapping.Build(ctx.WithSource(nonNullableSourceValue)));
     }
 }

test/Riok.Mapperly.Tests/Mapping/DictionaryTest.cs

@@ -57,7 +57,7 @@ public void DictionaryToDictionaryNullableToNonNullable()
                 var target = new global::System.Collections.Generic.Dictionary<string, int>(source.Count);
                 foreach (var item in source)
                 {
-                    target[item.Key] = item.Value == null ? throw new System.ArgumentNullException(nameof(item.Value)) : item.Value.Value;
+                    target[item.Key] = item.Value ?? throw new System.ArgumentNullException(nameof(item.Value));
                 }
                 return target;
                 """

test/Riok.Mapperly.Tests/Mapping/EnumerableDeepCloningTest.cs

@@ -41,7 +41,7 @@ public void ArrayOfNullablePrimitiveTypesToNonNullableArrayDeepCloning()
                 var target = new int[source.Length];
                 for (var i = 0; i < source.Length; i++)
                 {
-                    target[i] = source[i] == null ? throw new System.NullReferenceException($"Sequence {nameof(source)}, contained a null value at index {i}.") : source[i].Value;
+                    target[i] = source[i] ?? throw new System.NullReferenceException($"Sequence {nameof(source)}, contained a null value at index {i}.");
                 }
                 return target;
                 """

test/Riok.Mapperly.Tests/Mapping/EnumerableTest.cs

@@ -19,7 +19,7 @@ public void NullableArrayToNonNullableArray()
         TestHelper
             .GenerateMapper(source)
             .Should()
-            .HaveSingleMethodBody("return source == null ? throw new System.ArgumentNullException(nameof(source)) : source;");
+            .HaveSingleMethodBody("return source ?? throw new System.ArgumentNullException(nameof(source));");
     }
 
     [Fact]
@@ -34,7 +34,7 @@ public void ArrayOfNullablePrimitiveTypesToNonNullableArray()
                 var target = new int[source.Length];
                 for (var i = 0; i < source.Length; i++)
                 {
-                    target[i] = source[i] == null ? throw new System.NullReferenceException($"Sequence {nameof(source)}, contained a null value at index {i}.") : source[i].Value;
+                    target[i] = source[i] ?? throw new System.NullReferenceException($"Sequence {nameof(source)}, contained a null value at index {i}.");
                 }
                 return target;
                 """
@@ -79,7 +79,7 @@ public void ArrayCustomClassNullableToArrayCustomClassNonNullable()
                 var target = new global::B[source.Length];
                 for (var i = 0; i < source.Length; i++)
                 {
-                    target[i] = source[i] == null ? throw new System.NullReferenceException($"Sequence {nameof(source)}, contained a null value at index {i}.") : source[i]!;
+                    target[i] = source[i] ?? throw new System.NullReferenceException($"Sequence {nameof(source)}, contained a null value at index {i}.");
                 }
                 return target;
                 """

test/Riok.Mapperly.Tests/Mapping/ObjectPropertyUseNamedMappingTest.cs

@@ -363,6 +363,25 @@ class B
         return TestHelper.VerifyGenerator(source);
     }
 
+    [Fact]
+    public Task UserMethodReturnsNullableShouldThrow()
+    {
+        var source = TestSourceBuilder.MapperWithBodyAndTypes(
+            """
+            [MapProperty("Name", "Value", Use = nameof(ToC))]
+            public partial B Map(A source);
+
+            [UserMapping(Default = false)]
+            public C? ToC(string name) => new C(name);
+            """,
+            "class A { public string? Name { get; set; } }",
+            "class B { public C Value { get; set; } }",
+            "record C(string Name);"
+        );
+
+        return TestHelper.VerifyGenerator(source);
+    }
+
     [Fact]
     public Task ShouldPassNullValueToNullableUserMappingMethod()
     {

test/Riok.Mapperly.Tests/TestHelper.cs

@@ -16,7 +16,7 @@ public static Task<VerifyResult> VerifyGenerator(string source, TestHelperOption
         var driver = Generate(source, options);
         var verify = Verify(driver);
 
-        if (args.Any())
+        if (args.Length != 0)
         {
             verify.UseParameters(args);
         }

test/Riok.Mapperly.Tests/_snapshots/ObjectPropertyNullableTest.NullableToNullablePropertyWithAnotherNullableToNonNullableMappingShouldDirectAssign#Mapper.g.verified.cs

@@ -25,7 +25,7 @@ public partial class Mapper
         var target = new global::System.Collections.Generic.List<int>(source.Count);
         foreach (var item in source)
         {
-            target.Add(item == null ? throw new System.ArgumentNullException(nameof(item)) : item.Value);
+            target.Add(item ?? throw new System.ArgumentNullException(nameof(item)));
         }
         return target;
     }

test/Riok.Mapperly.Tests/_snapshots/ObjectPropertyUseNamedMappingTest.UserMethodReturnsNullableShouldThrow#Mapper.g.verified.cs

@@ -0,0 +1,13 @@
+//HintName: Mapper.g.cs
+// <auto-generated />
+#nullable enable
+public partial class Mapper
+{
+    [global::System.CodeDom.Compiler.GeneratedCode("Riok.Mapperly", "0.0.1.0")]
+    public partial global::B Map(global::A source)
+    {
+        var target = new global::B();
+        target.Value = ToC(source.Name ?? throw new System.ArgumentNullException(nameof(source.Name))) ?? throw new System.NullReferenceException("ToC returned null");
+        return target;
+    }
+}