Skip to content

Commit

Permalink
allow multiple MapProperty attributes for the same target member (#1560)
Browse files Browse the repository at this point in the history
  • Loading branch information
latonz authored Oct 25, 2024
1 parent eb58e9c commit 5ab24a9
Show file tree
Hide file tree
Showing 13 changed files with 142 additions and 81 deletions.
5 changes: 5 additions & 0 deletions src/Riok.Mapperly/Configuration/IMemberPathConfiguration.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@ public interface IMemberPathConfiguration
/// </summary>
string FullName { get; }

/// <summary>
/// The member names of the path / each segment, e.g. [A,B,C]
/// </summary>
IEnumerable<string> MemberNames { get; }

/// <summary>
/// The number of path segments in this path.
/// </summary>
Expand Down
1 change: 1 addition & 0 deletions src/Riok.Mapperly/Configuration/StringMemberPath.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ public StringMemberPath(IEnumerable<string> path)
public string RootName => Path[0];
public string FullName => string.Join(MemberPathConstants.MemberAccessSeparatorString, Path);
public int PathCount => Path.Count;
public IEnumerable<string> MemberNames => Path;

public override string ToString() => FullName;

Expand Down
3 changes: 2 additions & 1 deletion src/Riok.Mapperly/Configuration/SymbolMemberPath.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,9 @@ public record SymbolMemberPath(ImmutableEquatableArray<ISymbol> Path) : IMemberP
private string? _fullName;

public string RootName => Path[0].Name;
public string FullName => _fullName ??= string.Join(MemberPathConstants.MemberAccessSeparatorString, Path.Select(x => x.Name));
public string FullName => _fullName ??= string.Join(MemberPathConstants.MemberAccessSeparatorString, MemberNames);
public int PathCount => Path.Count;
public IEnumerable<string> MemberNames => Path.Select(x => x.Name);

public override string ToString() => FullName;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ public interface IMembersBuilderContext<out T>
void SetTargetMemberMapped(IMappableMember targetMember);

void ConsumeMemberConfigs(MemberMappingInfo members);
bool HasDuplicatedMemberConfig(NonEmptyMemberPath targetMemberPath);

void TryAddSourceMemberAlias(string alias, IMappableMember member);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ public void ConsumeMemberConfigs(MemberMappingInfo members)
}
}

public bool HasDuplicatedMemberConfig(NonEmptyMemberPath targetMemberPath) => _state.HasDuplicatedMemberConfig(targetMemberPath);

public void MappingAdded() => _state.MappingAdded();

protected void MappingAdded(MemberMappingInfo info, bool ignoreTargetCasing = false) => _state.MappingAdded(info, ignoreTargetCasing);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ internal class MembersMappingState(
Dictionary<string, IMappableMember> targetMembers,
Dictionary<string, List<MemberValueMappingConfiguration>> memberValueConfigsByRootTargetName,
Dictionary<string, List<MemberMappingConfiguration>> memberConfigsByRootTargetName,
Dictionary<string, List<IMemberPathConfiguration>> configuredTargetMembersByRootName,
HashSet<string> ignoredSourceMemberNames
)
{
Expand Down Expand Up @@ -105,6 +106,14 @@ public void IgnoreMembers(string name)
}
}

public bool HasDuplicatedMemberConfig(NonEmptyMemberPath targetMemberPath)
{
if (!configuredTargetMembersByRootName.TryGetValue(targetMemberPath.RootName, out var configs))
return false;

return configs.Where(targetMemberPath.Equals).Skip(1).Any();
}

public void SetTargetMemberMapped(IMappableMember targetMember) => SetTargetMemberMapped(targetMember.Name);

public void SetTargetMemberMapped(string targetName, bool ignoreCase = false)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,23 @@ internal static class MembersMappingStateBuilder
public static MembersMappingState Build(MappingBuilderContext ctx, IMapping mapping)
{
// build configurations
var configuredTargetMembers = new HashSet<IMemberPathConfiguration>();
var memberValueConfigsByRootTargetName = BuildMemberValueConfigurations(ctx, mapping, configuredTargetMembers);
var memberConfigsByRootTargetName = BuildMemberConfigurations(ctx, mapping, configuredTargetMembers);
// duplicated configurations are filtered inside the MapValue configurations
// if a MapProperty configuration references a target member which is already configured
// via MapValue it is also filtered & reported
var valueMappingConfiguredTargetPaths = new HashSet<string>();
var configuredTargetMembersByRootName = new ListDictionary<string, IMemberPathConfiguration>();
var memberValueConfigsByRootTargetName = BuildMemberValueConfigurations(
ctx,
mapping,
valueMappingConfiguredTargetPaths,
configuredTargetMembersByRootName
);
var memberConfigsByRootTargetName = BuildMemberConfigurations(
ctx,
mapping,
valueMappingConfiguredTargetPaths,
configuredTargetMembersByRootName
);

// build all members
var unmappedSourceMemberNames = GetSourceMemberNames(ctx, mapping);
Expand Down Expand Up @@ -47,6 +61,7 @@ public static MembersMappingState Build(MappingBuilderContext ctx, IMapping mapp
targetMembers,
memberValueConfigsByRootTargetName,
memberConfigsByRootTargetName,
configuredTargetMembersByRootName.AsDictionary(),
ignoredSourceMemberNames
);
}
Expand Down Expand Up @@ -76,55 +91,58 @@ private static Dictionary<string, IMappableMember> GetTargetMembers(MappingBuild
private static Dictionary<string, List<MemberValueMappingConfiguration>> BuildMemberValueConfigurations(
MappingBuilderContext ctx,
IMapping mapping,
HashSet<IMemberPathConfiguration> configuredTargetMembers
HashSet<string> configuredTargetPaths,
ListDictionary<string, IMemberPathConfiguration> configuredTargetMembersByRootName
)
{
return GetUniqueTargetConfigurations(ctx, mapping, configuredTargetMembers, ctx.Configuration.Members.ValueMappings, x => x.Target)
.GroupBy(x => x.Target.RootName)
.ToDictionary(x => x.Key, x => x.ToList());
var byTargetRootName = new ListDictionary<string, MemberValueMappingConfiguration>(ctx.Configuration.Members.ValueMappings.Count);
foreach (var config in ctx.Configuration.Members.ValueMappings)
{
configuredTargetMembersByRootName.Add(config.Target.RootName, config.Target);
if (configuredTargetPaths.Add(config.Target.FullName))
{
byTargetRootName.Add(config.Target.RootName, config);
continue;
}

ctx.ReportDiagnostic(
DiagnosticDescriptors.MultipleConfigurationsForTargetMember,
mapping.TargetType.ToDisplayString(),
config.Target.FullName
);
}

return byTargetRootName.AsDictionary();
}

private static Dictionary<string, List<MemberMappingConfiguration>> BuildMemberConfigurations(
MappingBuilderContext ctx,
IMapping mapping,
HashSet<IMemberPathConfiguration> configuredTargetMembers
HashSet<string> valueConfiguredTargetPaths,
ListDictionary<string, IMemberPathConfiguration> configuredTargetMembersByRootName
)
{
// order by target path count as objects with less path depth should be mapped first
// to prevent NREs in the generated code
return GetUniqueTargetConfigurations(
ctx,
mapping,
configuredTargetMembers,
ctx.Configuration.Members.ExplicitMappings,
x => x.Target
)
.GroupBy(x => x.Target.RootName)
.ToDictionary(x => x.Key, x => x.OrderBy(cfg => cfg.Target.PathCount).ToList());
}

private static IEnumerable<T> GetUniqueTargetConfigurations<T>(
MappingBuilderContext ctx,
IMapping mapping,
HashSet<IMemberPathConfiguration> configuredTargetMembers,
IEnumerable<T> configs,
Func<T, IMemberPathConfiguration> targetPathSelector
)
{
foreach (var config in configs)
var result = new ListDictionary<string, MemberMappingConfiguration>();
foreach (var config in ctx.Configuration.Members.ExplicitMappings.OrderBy(cfg => cfg.Target.PathCount))
{
var targetPath = targetPathSelector(config);
if (configuredTargetMembers.Add(targetPath))
// if MapValue is already configured for this target member
// no additional MapProperty is allowed => diagnostic duplicate config.
if (valueConfiguredTargetPaths.Contains(config.Target.FullName))
{
yield return config;
ctx.ReportDiagnostic(
DiagnosticDescriptors.MultipleConfigurationsForTargetMember,
mapping.TargetType.ToDisplayString(),
config.Target.FullName
);
continue;
}

ctx.ReportDiagnostic(
DiagnosticDescriptors.MultipleConfigurationsForTargetMember,
mapping.TargetType.ToDisplayString(),
targetPath.FullName
);
configuredTargetMembersByRootName.Add(config.Target.RootName, config.Target);
result.Add(config.Target.RootName, config);
}

return result.AsDictionary();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -107,11 +107,22 @@ public static bool ValidateMappingSpecification(
var noInitOnlyPath = allowInitOnlyMember ? targetMemberPath.ObjectPath : targetMemberPath.Path;
if (noInitOnlyPath.Any(p => p.IsInitOnly))
{
ctx.BuilderContext.ReportDiagnostic(
DiagnosticDescriptors.CannotMapToInitOnlyMemberPath,
memberInfo.DescribeSource(),
targetMemberPath.ToDisplayString(includeMemberType: false)
);
if (ctx.HasDuplicatedMemberConfig(targetMemberPath))
{
ctx.BuilderContext.ReportDiagnostic(
DiagnosticDescriptors.MultipleConfigurationsForTargetMember,
ctx.Mapping.TargetType.ToDisplayString(),
targetMemberPath.ToDisplayString(includeMemberType: false, includeRootType: false)
);
}
else
{
ctx.BuilderContext.ReportDiagnostic(
DiagnosticDescriptors.CannotMapToInitOnlyMemberPath,
memberInfo.DescribeSource(),
targetMemberPath.ToDisplayString(includeMemberType: false)
);
}
return false;
}

Expand Down
16 changes: 14 additions & 2 deletions src/Riok.Mapperly/Helpers/ListDictionary.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,20 @@ namespace Riok.Mapperly.Helpers;
public class ListDictionary<TKey, TValue>
where TKey : notnull
{
private readonly IReadOnlyList<TValue> _empty = [];
private readonly Dictionary<TKey, List<TValue>> _data = new();
private static readonly IReadOnlyList<TValue> _empty = [];
private readonly Dictionary<TKey, List<TValue>> _data;

public ListDictionary()
{
_data = new Dictionary<TKey, List<TValue>>();
}

public ListDictionary(int capacity)
{
_data = new Dictionary<TKey, List<TValue>>(capacity);
}

public Dictionary<TKey, List<TValue>> AsDictionary() => _data;

public IReadOnlyList<TValue> GetOrEmpty(TKey key) => _data.GetValueOrDefault(key) ?? _empty;

Expand Down
15 changes: 15 additions & 0 deletions src/Riok.Mapperly/Symbols/Members/MemberPath.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using System.Diagnostics;
using Microsoft.CodeAnalysis;
using Riok.Mapperly.Configuration;
using Riok.Mapperly.Descriptors;
using Riok.Mapperly.Helpers;

Expand Down Expand Up @@ -79,6 +80,20 @@ public static MemberPath Create(ITypeSymbol rootType, IReadOnlyList<IMappableMem
return new NonEmptyMemberPath(rootType, path);
}

public bool Equals(IMemberPathConfiguration path)
{
if (path.PathCount != Path.Count)
return false;

foreach (var (pathSegment1, pathSegment2) in Path.Zip(path.MemberNames))
{
if (!string.Equals(pathSegment1.Name, pathSegment2, StringComparison.Ordinal))
return false;
}

return true;
}

public override bool Equals(object? obj)
{
if (ReferenceEquals(null, obj))
Expand Down
5 changes: 5 additions & 0 deletions src/Riok.Mapperly/Symbols/Members/NonEmptyMemberPath.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,11 @@ public NonEmptyMemberPath(ITypeSymbol rootType, IReadOnlyList<IMappableMember> p
/// </summary>
public override IMappableMember Member => Path[^1];

/// <summary>
/// Gets the name of the first path segment.
/// </summary>
public string RootName => Path[0].Name;

/// <summary>
/// Gets the type of the <see cref="Member"/>. If any part of the path is nullable, this type will be nullable too.
/// </summary>
Expand Down
30 changes: 18 additions & 12 deletions test/Riok.Mapperly.Tests/Mapping/ObjectPropertyTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -177,30 +177,36 @@ public void WithManualMappedProperty()
}

[Fact]
public void WithManualMappedPropertyDuplicated()
public void WithManualMappedPropertyDuplicatedAndNullFilter()
{
var source = TestSourceBuilder.MapperWithBodyAndTypes(
"""
[MapProperty(nameof(A.StringValue), nameof(B.StringValue2)]
[MapProperty(nameof(A.StringValue), nameof(B.StringValue2)]
[MapProperty(nameof(A.StringValue1), nameof(B.StringValue)]
[MapProperty(nameof(A.StringValue2), nameof(B.StringValue)]
partial B Map(A source);
""",
"class A { public string StringValue { get; set; } }",
"class B { public string StringValue2 { get; set; } }"
TestSourceBuilderOptions.Default with
{
AllowNullPropertyAssignment = false,
},
"class A { public string? StringValue1 { get; set; } public string? StringValue2 { get; set; } }",
"class B { public string StringValue { get; set; } }"
);

TestHelper
.GenerateMapper(source, TestHelperOptions.AllowDiagnostics)
.GenerateMapper(source)
.Should()
.HaveDiagnostic(
DiagnosticDescriptors.MultipleConfigurationsForTargetMember,
"Multiple mappings are configured for the same target member B.StringValue2"
)
.HaveAssertedAllDiagnostics()
.HaveSingleMethodBody(
"""
var target = new global::B();
target.StringValue2 = source.StringValue;
if (source.StringValue1 != null)
{
target.StringValue = source.StringValue1;
}
if (source.StringValue2 != null)
{
target.StringValue = source.StringValue2;
}
return target;
"""
);
Expand Down
25 changes: 0 additions & 25 deletions test/Riok.Mapperly.Tests/Mapping/ValueTupleTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -624,31 +624,6 @@ public void QueryableTupleToIQueryableValueTuple()
);
}

[Fact]
public void TupleToTupleWithManyMapPropertyShouldDiagnostic()
{
var source = TestSourceBuilder.MapperWithBodyAndTypes(
"""
[MapProperty("B", "A")]
[MapProperty("Item1", "A")]
partial (int A, int) Map((int, string B) source);
"""
);

TestHelper
.GenerateMapper(source, TestHelperOptions.AllowDiagnostics)
.Should()
.HaveDiagnostic(
DiagnosticDescriptors.SourceMemberNotMapped,
"The member Item1 on the mapping source type (int, string B) is not mapped to any member on the mapping target type (int A, int)"
)
.HaveDiagnostic(
DiagnosticDescriptors.MultipleConfigurationsForTargetMember,
"Multiple mappings are configured for the same target member (int A, int).A"
)
.HaveAssertedAllDiagnostics();
}

[Fact]
public void TupleToTupleWithMapPropertyWithImplicitNameShouldDiagnostic()
{
Expand Down

1 comment on commit 5ab24a9

@github-actions
Copy link

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'SourceGeneratorBenchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 3.

Benchmark suite Current: 5ab24a9 Previous: e9abaf4 Ratio
Riok.Mapperly.Benchmarks.SourceGeneratorBenchmarks.Compile 2096327.052455357 ns (± 153863.55644484275) 120741.05671574519 ns (± 1678.1529391966976) 17.36
Riok.Mapperly.Benchmarks.SourceGeneratorBenchmarks.LargeCompile 45928717.43030303 ns (± 273266.98278387904) 1277208.939174107 ns (± 7926.104374412328) 35.96

This comment was automatically generated by workflow using github-action-benchmark.

Please sign in to comment.