fix: only initialize unflattened nullable target members when needed (#…

…1408)
riok · Aug 9, 2024 · 496ff27 · 496ff27
1 parent df28332
commit 496ff27
Show file tree

Hide file tree

Showing 22 changed files with 451 additions and 345 deletions.
diff --git a/docs/docs/breaking-changes/4-0.md b/docs/docs/breaking-changes/4-0.md
@@ -16,6 +16,7 @@ description: How to upgrade to Mapperly v4.0 and a list of all its breaking chan
 - If the `ExplicitCast` conversion is disabled, disable the new `EnumUnderlyingType` conversion too.
 - Members of foreach mappings are now mapped, which may result in additional members being mapped or new diagnostics being reported.
 - To account for changed diagnostics adjust your `.editorconfig` as needed, see [updated diagnostics](#updated-diagnostics).
+- The ordering of the member assignments in mappings may change, if you rely on the order of members being mapped, you may need to diff and verify the generated source code.
 
 ## MapPropertyAttribute constructors
 

diff --git a/...Mapperly/Descriptors/MappingBodyBuilders/BuilderContext/MembersContainerBuilderContext.cs b/...Mapperly/Descriptors/MappingBodyBuilders/BuilderContext/MembersContainerBuilderContext.cs
@@ -15,6 +15,7 @@ public class MembersContainerBuilderContext<T>(MappingBuilderContext builderCont
     where T : IMemberAssignmentTypeMapping
 {
     private readonly Dictionary<MemberPath, MemberNullDelegateAssignmentMapping> _nullDelegateMappings = new();
+    private readonly HashSet<MemberPath> _initializedNullableTargetPaths = new();
 
     public void AddMemberAssignmentMapping(IMemberAssignmentMapping memberMapping) => AddMemberAssignmentMapping(Mapping, memberMapping);
 
@@ -59,18 +60,28 @@ private void AddMemberAssignmentMapping(IMemberAssignmentMappingContainer contai
         AddNullMemberInitializers(container, mapping.MemberInfo.TargetMember);
         container.AddMemberMapping(mapping);
         MappingAdded(mapping.MemberInfo);
+
+        // if the source value is a non-nullable value,
+        // the target should be non-null after this assignment and can be set as initialized.
+        if (!mapping.MemberInfo.IsSourceNullable && mapping.MemberInfo.TargetMember.MemberType.IsNullable())
+        {
+            _initializedNullableTargetPaths.Add(mapping.MemberInfo.TargetMember);
+        }
     }
 
     private void AddNullMemberInitializers(IMemberAssignmentMappingContainer container, MemberPath path)
     {
-        foreach (var nullableTrailPath in path.ObjectPathNullableSubPaths())
+        foreach (var nullablePathList in path.ObjectPathNullableSubPaths())
         {
-            var nullablePath = new NonEmptyMemberPath(path.RootType, nullableTrailPath);
+            var nullablePath = new NonEmptyMemberPath(path.RootType, nullablePathList);
             var type = nullablePath.Member.Type.NonNullable();
 
             if (!nullablePath.Member.CanSet)
                 continue;
 
+            if (!_initializedNullableTargetPaths.Add(nullablePath))
+                continue;
+
             if (!BuilderContext.InstanceConstructors.TryBuild(BuilderContext.Source, type, out var ctor))
             {
                 BuilderContext.ReportDiagnostic(DiagnosticDescriptors.NoParameterlessConstructorFound, type);
@@ -97,31 +108,28 @@ private MemberNullDelegateAssignmentMapping GetOrCreateNullDelegateMappingForPat
         if (_nullDelegateMappings.TryGetValue(nullConditionSourcePath, out var mapping))
             return mapping;
 
-        IMemberAssignmentMappingContainer parentMapping = Mapping;
+        var parentMapping = FindParentNonNullContainer(nullConditionSourcePath, out var needsNullSafeAccess);
+        var nullConditionSourcePathGetter = nullConditionSourcePath.BuildGetter(BuilderContext);
+        mapping = new MemberNullDelegateAssignmentMapping(nullConditionSourcePathGetter, parentMapping, needsNullSafeAccess);
+        _nullDelegateMappings[nullConditionSourcePath] = mapping;
+        parentMapping.AddMemberMappingContainer(mapping);
+        return mapping;
+    }
 
+    private IMemberAssignmentMappingContainer FindParentNonNullContainer(MemberPath nullConditionSourcePath, out bool needsNullSafeAccess)
+    {
         // try to reuse parent path mappings and wrap inside them
         // if the parentMapping is the first nullable path, no need to access the path in the condition in a null-safe way.
-        var needsNullSafeAccess = false;
-        foreach (var nullablePath in nullConditionSourcePath.ObjectPathNullableSubPaths().Reverse())
+        needsNullSafeAccess = false;
+        foreach (var nullablePathList in nullConditionSourcePath.ObjectPathNullableSubPaths().Reverse())
         {
-            if (
-                _nullDelegateMappings.TryGetValue(
-                    new NonEmptyMemberPath(nullConditionSourcePath.RootType, nullablePath),
-                    out var parentMappingHolder
-                )
-            )
-            {
-                parentMapping = parentMappingHolder;
-                break;
-            }
+            var nullablePath = new NonEmptyMemberPath(nullConditionSourcePath.RootType, nullablePathList);
+            if (_nullDelegateMappings.TryGetValue(nullablePath, out var parentMappingContainer))
+                return parentMappingContainer;
 
             needsNullSafeAccess = true;
         }
 
-        var nullConditionSourcePathGetter = nullConditionSourcePath.BuildGetter(BuilderContext);
-        mapping = new MemberNullDelegateAssignmentMapping(nullConditionSourcePathGetter, parentMapping, needsNullSafeAccess);
-        _nullDelegateMappings[nullConditionSourcePath] = mapping;
-        parentMapping.AddMemberMappingContainer(mapping);
-        return mapping;
+        return Mapping;
     }
 }
diff --git a/src/Riok.Mapperly/Descriptors/Mappings/MemberMappings/IAssignmentMappings.cs b/src/Riok.Mapperly/Descriptors/Mappings/MemberMappings/IAssignmentMappings.cs
@@ -0,0 +1,8 @@
+using Microsoft.CodeAnalysis.CSharp.Syntax;
+
+namespace Riok.Mapperly.Descriptors.Mappings.MemberMappings;
+
+public interface IAssignmentMappings
+{
+    IEnumerable<StatementSyntax> Build(TypeMappingBuildContext ctx, ExpressionSyntax targetAccess);
+}
diff --git a/src/Riok.Mapperly/Descriptors/Mappings/MemberMappings/IMemberAssignmentMapping.cs b/src/Riok.Mapperly/Descriptors/Mappings/MemberMappings/IMemberAssignmentMapping.cs
@@ -1,13 +1,9 @@
-using Microsoft.CodeAnalysis.CSharp.Syntax;
-
 namespace Riok.Mapperly.Descriptors.Mappings.MemberMappings;
 
 /// <summary>
 /// Represents a member assignment mapping or a container of member assignment mappings.
 /// </summary>
-public interface IMemberAssignmentMapping
+public interface IMemberAssignmentMapping : IAssignmentMappings
 {
     MemberMappingInfo MemberInfo { get; }
-
-    IEnumerable<StatementSyntax> Build(TypeMappingBuildContext ctx, ExpressionSyntax targetAccess);
 }
diff --git a/src/Riok.Mapperly/Descriptors/Mappings/MemberMappings/IMemberAssignmentMappingContainer.cs b/src/Riok.Mapperly/Descriptors/Mappings/MemberMappings/IMemberAssignmentMappingContainer.cs
@@ -1,11 +1,9 @@
-using Microsoft.CodeAnalysis.CSharp.Syntax;
-
 namespace Riok.Mapperly.Descriptors.Mappings.MemberMappings;
 
 /// <summary>
 /// Represents a container of several <see cref="IMemberAssignmentMapping"/>.
 /// </summary>
-public interface IMemberAssignmentMappingContainer
+public interface IMemberAssignmentMappingContainer : IAssignmentMappings
 {
     bool HasMemberMapping(IMemberAssignmentMapping mapping);
 
@@ -14,6 +12,4 @@ public interface IMemberAssignmentMappingContainer
     bool HasMemberMappingContainer(IMemberAssignmentMappingContainer container);
 
     void AddMemberMappingContainer(IMemberAssignmentMappingContainer container);
-
-    IEnumerable<StatementSyntax> Build(TypeMappingBuildContext ctx, ExpressionSyntax targetAccess);
 }
diff --git a/src/Riok.Mapperly/Descriptors/Mappings/MemberMappings/MemberAssignmentMappingContainer.cs b/src/Riok.Mapperly/Descriptors/Mappings/MemberMappings/MemberAssignmentMappingContainer.cs
@@ -9,31 +9,30 @@ public abstract class MemberAssignmentMappingContainer(IMemberAssignmentMappingC
 {
     private readonly HashSet<IMemberAssignmentMapping> _delegateMappings = new();
     private readonly HashSet<IMemberAssignmentMappingContainer> _childContainers = new();
+    private readonly List<IAssignmentMappings> _mappings = new();
 
-    public virtual IEnumerable<StatementSyntax> Build(TypeMappingBuildContext ctx, ExpressionSyntax targetAccess)
-    {
-        var childContainerStatements = _childContainers.SelectMany(x => x.Build(ctx, targetAccess));
-        var mappings = _delegateMappings.SelectMany(m => m.Build(ctx, targetAccess));
-        return childContainerStatements.Concat(mappings);
-    }
+    public virtual IEnumerable<StatementSyntax> Build(TypeMappingBuildContext ctx, ExpressionSyntax targetAccess) =>
+        _mappings.SelectMany(x => x.Build(ctx, targetAccess));
 
     public void AddMemberMappingContainer(IMemberAssignmentMappingContainer container)
     {
-        if (!HasMemberMappingContainer(container))
-        {
-            _childContainers.Add(container);
-        }
+        if (HasMemberMappingContainer(container))
+            return;
+
+        _childContainers.Add(container);
+        _mappings.Add(container);
     }
 
     public bool HasMemberMappingContainer(IMemberAssignmentMappingContainer container) =>
         _childContainers.Contains(container) || parent?.HasMemberMappingContainer(container) == true;
 
     public void AddMemberMapping(IMemberAssignmentMapping mapping)
     {
-        if (!HasMemberMapping(mapping))
-        {
-            _delegateMappings.Add(mapping);
-        }
+        if (HasMemberMapping(mapping))
+            return;
+
+        _delegateMappings.Add(mapping);
+        _mappings.Add(mapping);
     }
 
     public bool HasMemberMapping(IMemberAssignmentMapping mapping) =>

diff --git a/src/Riok.Mapperly/Descriptors/Mappings/MemberMappings/MemberMappingInfo.cs b/src/Riok.Mapperly/Descriptors/Mappings/MemberMappings/MemberMappingInfo.cs
@@ -18,6 +18,8 @@ public MemberMappingInfo(SourceMemberPath? sourceMember, NonEmptyMemberPath targ
     public MemberMappingInfo(NonEmptyMemberPath targetMember, MemberValueMappingConfiguration configuration)
         : this(null, targetMember, configuration) { }
 
+    public bool IsSourceNullable => SourceMember?.MemberPath.IsAnyNullable() ?? ValueConfiguration?.Value?.ConstantValue.IsNull ?? true;
+
     private string DebuggerDisplay =>
         $"{SourceMember?.MemberPath.FullName ?? ValueConfiguration?.DescribeValue()} => {TargetMember.FullName}";
 

diff --git a/src/Riok.Mapperly/Descriptors/Mappings/MemberMappings/SourceValue/MappedMemberSourceValue.cs b/src/Riok.Mapperly/Descriptors/Mappings/MemberMappings/SourceValue/MappedMemberSourceValue.cs
@@ -1,3 +1,4 @@
+using System.Diagnostics;
 using Microsoft.CodeAnalysis.CSharp.Syntax;
 using Riok.Mapperly.Symbols.Members;
 
@@ -7,24 +8,28 @@ namespace Riok.Mapperly.Descriptors.Mappings.MemberMappings.SourceValue;
 /// A mapped source member without any null handling.
 /// (e.g. <c>MapToD(source.A.B)</c> or <c>MapToD(source?.A?.B)</c>).
 /// </summary>
+[DebuggerDisplay("MappedMemberSourceValue({_sourceMember}: {_delegateMapping})")]
 public class MappedMemberSourceValue(
     INewInstanceMapping delegateMapping,
     MemberPathGetter sourceMember,
     bool nullConditionalAccess,
     bool addValuePropertyOnNullable
 ) : ISourceValue
 {
-    public bool RequiresSourceNullCheck => !nullConditionalAccess && sourceMember.MemberPath.IsAnyNullable();
+    private readonly MemberPathGetter _sourceMember = sourceMember;
+    private readonly INewInstanceMapping _delegateMapping = delegateMapping;
+
+    public bool RequiresSourceNullCheck => !nullConditionalAccess && _sourceMember.MemberPath.IsAnyNullable();
 
     public ExpressionSyntax Build(TypeMappingBuildContext ctx)
     {
         ctx = ctx.WithSource(
-            sourceMember.BuildAccess(
+            _sourceMember.BuildAccess(
                 ctx.Source,
                 addValuePropertyOnNullable: addValuePropertyOnNullable,
                 nullConditional: nullConditionalAccess
             )
         );
-        return delegateMapping.Build(ctx);
+        return _delegateMapping.Build(ctx);
     }
 }
diff --git a/...tegrationTests/_snapshots/CircularReferenceMapperTest.SnapshotGeneratedSource.verified.cs b/...tegrationTests/_snapshots/CircularReferenceMapperTest.SnapshotGeneratedSource.verified.cs
@@ -17,6 +17,7 @@ public static partial class CircularReferenceMapper
                 return existingTargetReference;
             var target = new global::Riok.Mapperly.IntegrationTests.Dto.CircularReferenceDto();
             refHandler.SetReference<global::Riok.Mapperly.IntegrationTests.Models.CircularReferenceObject, global::Riok.Mapperly.IntegrationTests.Dto.CircularReferenceDto>(source, target);
+            target.Value = source.Value;
             if (source.Parent != null)
             {
                 target.Parent = MapToCircularReferenceDto(source.Parent, refHandler);
@@ -25,7 +26,6 @@ public static partial class CircularReferenceMapper
             {
                 target.Parent = null;
             }
-            target.Value = source.Value;
             return target;
         }
     }

diff --git a/...rly.IntegrationTests/_snapshots/DeepCloningMapperTest.SnapshotGeneratedSource.verified.cs b/...rly.IntegrationTests/_snapshots/DeepCloningMapperTest.SnapshotGeneratedSource.verified.cs
@@ -1,4 +1,4 @@
-// <auto-generated />
+// <auto-generated />
 #nullable enable
 namespace Riok.Mapperly.IntegrationTests.Mapper
 {
@@ -20,6 +20,10 @@ public static partial class DeepCloningMapper
                 IntInitOnlyValue = src.IntInitOnlyValue,
                 RequiredValue = src.RequiredValue,
             };
+            target.IntValue = src.IntValue;
+            target.StringValue = src.StringValue;
+            target.RenamedStringValue = src.RenamedStringValue;
+            target.Flattening = Copy(src.Flattening);
             if (src.NullableFlattening != null)
             {
                 target.NullableFlattening = Copy(src.NullableFlattening);
@@ -28,6 +32,8 @@ public static partial class DeepCloningMapper
             {
                 target.NullableFlattening = null;
             }
+            target.UnflatteningIdValue = src.UnflatteningIdValue;
+            target.NullableUnflatteningIdValue = src.NullableUnflatteningIdValue;
             if (src.NestedNullable != null)
             {
                 target.NestedNullable = MapToTestObjectNested(src.NestedNullable);
@@ -52,6 +58,7 @@ public static partial class DeepCloningMapper
             {
                 target.NestedMember = null;
             }
+            target.StringNullableTargetNotNullable = src.StringNullableTargetNotNullable;
             if (src.TupleValue != null)
             {
                 target.TupleValue = MapToValueTupleOfStringAndString(src.TupleValue.Value);
@@ -84,21 +91,6 @@ public static partial class DeepCloningMapper
             {
                 target.NullableReadOnlyObjectCollection = null;
             }
-            if (src.SubObject != null)
-            {
-                target.SubObject = MapToInheritanceSubObject(src.SubObject);
-            }
-            else
-            {
-                target.SubObject = null;
-            }
-            target.IntValue = src.IntValue;
-            target.StringValue = src.StringValue;
-            target.RenamedStringValue = src.RenamedStringValue;
-            target.Flattening = Copy(src.Flattening);
-            target.UnflatteningIdValue = src.UnflatteningIdValue;
-            target.NullableUnflatteningIdValue = src.NullableUnflatteningIdValue;
-            target.StringNullableTargetNotNullable = src.StringNullableTargetNotNullable;
             target.MemoryValue = src.MemoryValue.Span.ToArray();
             target.StackValue = new global::System.Collections.Generic.Stack<string>(src.StackValue);
             target.QueueValue = new global::System.Collections.Generic.Queue<string>(src.QueueValue);
@@ -134,6 +126,14 @@ public static partial class DeepCloningMapper
             target.EnumRawValue = src.EnumRawValue;
             target.EnumStringValue = src.EnumStringValue;
             target.EnumReverseStringValue = src.EnumReverseStringValue;
+            if (src.SubObject != null)
+            {
+                target.SubObject = MapToInheritanceSubObject(src.SubObject);
+            }
+            else
+            {
+                target.SubObject = null;
+            }
             target.DateTimeValue = src.DateTimeValue;
             target.DateTimeValueTargetDateOnly = src.DateTimeValueTargetDateOnly;
             target.DateTimeValueTargetTimeOnly = src.DateTimeValueTargetTimeOnly;
@@ -154,6 +154,7 @@ public static partial class DeepCloningMapper
         private static global::Riok.Mapperly.IntegrationTests.Models.TestObjectNestedMember MapToTestObjectNestedMember(global::Riok.Mapperly.IntegrationTests.Models.TestObjectNestedMember source)
         {
             var target = new global::Riok.Mapperly.IntegrationTests.Models.TestObjectNestedMember();
+            target.NestedMemberId = source.NestedMemberId;
             if (source.NestedMemberObject != null)
             {
                 target.NestedMemberObject = MapToTestObjectNested(source.NestedMemberObject);
@@ -162,7 +163,6 @@ public static partial class DeepCloningMapper
             {
                 target.NestedMemberObject = null;
             }
-            target.NestedMemberId = source.NestedMemberId;
             return target;
         }