Skip to content

Commit

Permalink
fix: improve handling of read-only and immutable types
Browse files Browse the repository at this point in the history
* do not copy immutable types when deep cloning is enabled
* introduce new RMG083 for when trying to map to existing immutable types, replaces RMG009 if the member is read-only and the type is immutable
* RMG009 is not reported anymore if the read-only member on the target is matched automatically
  • Loading branch information
latonz committed Jul 23, 2024
1 parent b754503 commit f881652
Show file tree
Hide file tree
Showing 33 changed files with 370 additions and 216 deletions.
8 changes: 5 additions & 3 deletions docs/docs/breaking-changes/4-0.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ description: How to upgrade to Mapperly v4.0 and a list of all its breaking chan
- 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.
- Immutable collection types are not copied, even if `UseDeepCloning` is enabled.

## MapPropertyAttribute constructors

Expand Down Expand Up @@ -57,11 +58,12 @@ on how to set the default severity of a given diagnostic.

| ID | Change |
| -------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| `RMG009` | This is replaced by `RMG083` for immutable types. For read only members `RMG009` is used as in Mapperly v3. `RMG009` is not emitted anymore for automatically matched read-only members. |
| `RMG012` | Starting with v4.0 Mapperly enables strict mappings by default with a level of `warning`. The default severity of `RMG012: Source member was not found for target member` changes from `Info` to `Warning`. |
| `RMG017` | `RMG017: An init only member can have one configuration at max` and `RMG027: A constructor parameter can have one configuration at max` are merged into the new `RMG074: Multiple mappings are configured for the same target member` |
| `RMG027` | `RMG017: An init only member can have one configuration at max` and `RMG027: A constructor parameter can have one configuration at max` are merged into the new `RMG074: Multiple mappings are configured for the same target member` |
| `RMG020` | Starting with v4.0 Mapperly enables strict mappings by default with a level of `warning`. The default severity of `RMG020: Source member is not mapped to any target member` changes from `Info` to `Warning`. |
| `RMG026` | `RMG026: Cannot map from indexed member` is removed, starting with 4.0 indexed members are ignored |
| `RMG027` | `RMG017: An init only member can have one configuration at max` and `RMG027: A constructor parameter can have one configuration at max` are merged into the new `RMG074: Multiple mappings are configured for the same target member` |
| `RMG028` | `RMG028: Constructor parameter cannot handle target paths` is removed as this is now supported. |
| `RMG012` | Starting with v4.0 Mapperly enables strict mappings by default with a level of `warning`. The default severity of `RMG012: Source member was not found for target member` changes from `Info` to `Warning`. |
| `RMG020` | Starting with v4.0 Mapperly enables strict mappings by default with a level of `warning`. The default severity of `RMG020: Source member is not mapped to any target member` changes from `Info` to `Warning`. |
| `RMG037` | Starting with v4.0 Mapperly enables strict mappings by default with a level of `warning`. The default severity of `RMG037: An enum member could not be found on the source enum` changes from `Info` to `Warning`. |
| `RMG038` | Starting with v4.0 Mapperly enables strict mappings by default with a level of `warning`. The default severity of `RMG038: An enum member could not be found on the target enum` changes from `Info` to `Warning`. |
1 change: 1 addition & 0 deletions src/Riok.Mapperly/AnalyzerReleases.Shipped.md
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,7 @@ RMG037 | Mapper | Warning | An enum member could not be found on the source
RMG038 | Mapper | Warning | An enum member could not be found on the target enum
RMG081 | Mapper | Error | A mapping method with additional parameters cannot be a default mapping
RMG082 | Mapper | Warning | An additional mapping method parameter is not mapped
RMG083 | Mapper | Info | Cannot map to read only type

### Removed Rules

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ public static void BuildMappingBody(MappingBuilderContext ctx, IMemberAssignment
{
mappingCtx.SetTargetMemberMapped(initOnlyTargetMember);
}

// do not report "no member mapping" for existing target mappings
mappingCtx.MappingAdded();

mappingCtx.AddDiagnostics();
}

Expand Down Expand Up @@ -49,6 +53,17 @@ public static bool ValidateMappingSpecification(
// the target member path is readonly or not accessible
if (!targetMemberPath.Member.CanSet)
{
// If the mapping is matched automatically without any configuration
// mark both members as mapped,
// as the "CannotMapToReadOnlyMember" diagnostic should be informative.
// Also, an additional diagnostic may not even be expected here:
// for example, if the source and target contain the same computed read-only member.
if (memberInfo.IsAutoMatch && sourceMemberPath?.Type == SourceMemberType.Member)
{
ctx.SetMembersMapped(memberInfo);
return false;
}

ctx.BuilderContext.ReportDiagnostic(
DiagnosticDescriptors.CannotMapToReadOnlyMember,
memberInfo.DescribeSource(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,16 +132,16 @@ INewInstanceMapping valueMapping
if (!ctx.CollectionInfos.Target.ImplementedTypes.HasFlag(CollectionType.IDictionary))
return null;

if (BuildKeyValueMapping(ctx) is not var (keyMapping, valueMapping))
return null;

// if target is an immutable dictionary then don't create a foreach loop
if (ctx.CollectionInfos.Target.ImplementedTypes.HasFlag(CollectionType.IImmutableDictionary))
{
ctx.ReportDiagnostic(DiagnosticDescriptors.CannotMapToReadOnlyMember);
return null;
ctx.ReportDiagnostic(DiagnosticDescriptors.CannotMapToReadOnlyType, ctx.Target);
return new NoOpMapping(ctx.Source, ctx.Target);
}

if (BuildKeyValueMapping(ctx) is not var (keyMapping, valueMapping))
return null;

return new ForEachSetDictionaryExistingTargetMapping(
ctx.CollectionInfos,
keyMapping,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,12 @@ public static class EnumerableMappingBuilder
if (!ctx.CollectionInfos.Source.ImplementsIEnumerable || !ctx.CollectionInfos.Target.ImplementsIEnumerable)
return null;

if (ctx.CollectionInfos.Target.IsImmutableCollectionType || ctx.CollectionInfos.Target.IsArray)
{
ctx.ReportDiagnostic(DiagnosticDescriptors.CannotMapToReadOnlyType, ctx.Target);
return new NoOpMapping(ctx.Source, ctx.Target);
}

var elementMapping = ctx.FindOrBuildMapping(ctx.CollectionInfos.Source.EnumeratedType, ctx.CollectionInfos.Target.EnumeratedType);
if (elementMapping == null)
return null;
Expand All @@ -84,11 +90,6 @@ public static class EnumerableMappingBuilder
return new ForEachAddEnumerableExistingTargetMapping(ctx.CollectionInfos, elementMapping, addMethodName);
}

if (ctx.CollectionInfos.Target.IsImmutableCollectionType)
{
ctx.ReportDiagnostic(DiagnosticDescriptors.CannotMapToReadOnlyMember);
}

return null;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,8 @@ public static class MemoryMappingBuilder
// can only map to Enumerable. Existing types Span, Memory and array are all immutable
if (!target.ImplementsIEnumerable)
{
ctx.ReportDiagnostic(DiagnosticDescriptors.CannotMapToReadOnlyMember, ctx.Target);
return null;
ctx.ReportDiagnostic(DiagnosticDescriptors.CannotMapToReadOnlyType, ctx.Target);
return new NoOpMapping(ctx.Source, ctx.Target);
}

if (ctx.FindOrBuildMapping(source.EnumeratedType, target.EnumeratedType) is not { } elementMapping)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@ public static class NewInstanceObjectMemberMappingBuilder
if (ctx.Target.SpecialType != SpecialType.None || ctx.Source.SpecialType != SpecialType.None)
return null;

if (ctx.Target.IsImmutable())
return null;

if (ctx.Source.IsEnum() || ctx.Target.IsEnum())
return null;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,8 @@ when elementMapping.IsSynthetic && !ctx.Configuration.Mapper.UseDeepCloning

if (target.IsSpan)
{
ctx.ReportDiagnostic(DiagnosticDescriptors.CannotMapToReadOnlyMember, ctx.Target);
return null;
ctx.ReportDiagnostic(DiagnosticDescriptors.CannotMapToReadOnlyType, ctx.Target);
return new NoOpMapping(ctx.Source, ctx.Target);
}

if (source.IsMemory || target.IsMemory)
Expand All @@ -88,7 +88,8 @@ when elementMapping.IsSynthetic && !ctx.Configuration.Mapper.UseDeepCloning
// we diagnostic when it is an existing target mapping
if (ctx.CollectionInfos.Target.IsImmutableCollectionType)
{
ctx.ReportDiagnostic(DiagnosticDescriptors.CannotMapToReadOnlyMember);
ctx.ReportDiagnostic(DiagnosticDescriptors.CannotMapToReadOnlyType, ctx.Target);
return new NoOpMapping(ctx.Source, ctx.Target);
}

return null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,12 @@ public MemberMappingInfo(NonEmptyMemberPath targetMember, MemberValueMappingConf
private string DebuggerDisplay =>
$"{SourceMember?.MemberPath.FullName ?? ValueConfiguration?.DescribeValue()} => {TargetMember.FullName}";

/// <summary>
/// Returns <c>true</c> if the <see cref="SourceMember"/> and <see cref="TargetMember"/>
/// were matched automatically by Mapperly without any additional user configuration.
/// </summary>
public bool IsAutoMatch => Configuration == null && ValueConfiguration == null;

public TypeMappingKey ToTypeMappingKey()
{
if (SourceMember == null)
Expand Down
12 changes: 12 additions & 0 deletions src/Riok.Mapperly/Descriptors/Mappings/NoOpMapping.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Riok.Mapperly.Descriptors.Mappings.ExistingTarget;

namespace Riok.Mapperly.Descriptors.Mappings;

public class NoOpMapping(ITypeSymbol sourceType, ITypeSymbol targetType) : MethodMapping(sourceType, targetType), IExistingTargetMapping
{
public override IEnumerable<StatementSyntax> BuildBody(TypeMappingBuildContext ctx) => [];

public IEnumerable<StatementSyntax> Build(TypeMappingBuildContext ctx, ExpressionSyntax target) => [];
}
10 changes: 10 additions & 0 deletions src/Riok.Mapperly/Diagnostics/DiagnosticDescriptors.cs
Original file line number Diff line number Diff line change
Expand Up @@ -767,6 +767,16 @@ public static class DiagnosticDescriptors
true
);

public static readonly DiagnosticDescriptor CannotMapToReadOnlyType =
new(
"RMG083",
"Cannot map to read only type",
"Cannot map to read-only type {0}",
DiagnosticCategories.Mapper,
DiagnosticSeverity.Info,
true
);

private static string BuildHelpUri(string id)
{
#if ENV_NEXT
Expand Down
27 changes: 23 additions & 4 deletions src/Riok.Mapperly/Helpers/SymbolExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,23 @@ internal static class SymbolExtensions
SymbolDisplayMiscellaneousOptions.IncludeNullableReferenceTypeModifier
);

private static readonly ImmutableHashSet<string> _wellKnownImmutableNamespaces = ImmutableHashSet.Create(
"System.Collections.Immutable"
);

private static readonly ImmutableHashSet<string> _wellKnownImmutableTypes = ImmutableHashSet.Create(
typeof(Uri).FullName!,
typeof(Version).FullName!
"System.Uri",
"System.Version",
"System.DateTime",
"System.DateTimeOffset",
"System.DateOnly",
"System.TimeOnly",
"System.TimeSpan",
"System.DBNull",
"System.Void",
"System.ReadOnlyMemory<T>",
"System.ReadOnlySpan<T>",
"System.Linq.Lookup<TKey, TElement>"
);

internal static Location? GetSyntaxLocation(this ISymbol symbol) =>
Expand All @@ -30,8 +44,13 @@ internal static bool IsImmutable(this ISymbol symbol)

return namedSymbol.IsUnmanagedType
|| namedSymbol.SpecialType == SpecialType.System_String
|| IsDelegate(symbol)
|| _wellKnownImmutableTypes.Contains(namedSymbol.ToDisplayString());
|| (!namedSymbol.IsGenericType && _wellKnownImmutableTypes.Contains(namedSymbol.ToDisplayString()))
|| (namedSymbol.IsGenericType && _wellKnownImmutableTypes.Contains(namedSymbol.ConstructedFrom.ToDisplayString()))
|| (
namedSymbol.ContainingNamespace != null
&& _wellKnownImmutableNamespaces.Contains(namedSymbol.ContainingNamespace.ToDisplayString())
)
|| IsDelegate(symbol);
}

internal static bool IsDelegate(this ISymbol symbol)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,9 +111,9 @@
3
],
ImmutableStackValue: [
1,
3,
2,
3
1
],
ImmutableSortedSetValue: [
1,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,9 +111,9 @@
3
],
ImmutableStackValue: [
1,
3,
2,
3
1
],
ImmutableSortedSetValue: [
1,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,13 +94,13 @@ public static partial class DeepCloningMapper
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);
target.ImmutableArrayValue = global::System.Collections.Immutable.ImmutableArray.ToImmutableArray(src.ImmutableArrayValue);
target.ImmutableListValue = global::System.Collections.Immutable.ImmutableList.ToImmutableList(src.ImmutableListValue);
target.ImmutableQueueValue = global::System.Collections.Immutable.ImmutableQueue.CreateRange(src.ImmutableQueueValue);
target.ImmutableStackValue = global::System.Collections.Immutable.ImmutableStack.CreateRange(src.ImmutableStackValue);
target.ImmutableSortedSetValue = global::System.Collections.Immutable.ImmutableSortedSet.ToImmutableSortedSet(src.ImmutableSortedSetValue);
target.ImmutableDictionaryValue = global::System.Collections.Immutable.ImmutableDictionary.ToImmutableDictionary(src.ImmutableDictionaryValue);
target.ImmutableSortedDictionaryValue = global::System.Collections.Immutable.ImmutableSortedDictionary.ToImmutableSortedDictionary(src.ImmutableSortedDictionaryValue);
target.ImmutableArrayValue = src.ImmutableArrayValue;
target.ImmutableListValue = src.ImmutableListValue;
target.ImmutableQueueValue = src.ImmutableQueueValue;
target.ImmutableStackValue = src.ImmutableStackValue;
target.ImmutableSortedSetValue = src.ImmutableSortedSetValue;
target.ImmutableDictionaryValue = src.ImmutableDictionaryValue;
target.ImmutableSortedDictionaryValue = src.ImmutableSortedDictionaryValue;
foreach (var item in src.ExistingISet)
{
target.ExistingISet.Add(item);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,13 +94,13 @@ public static partial class DeepCloningMapper
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);
target.ImmutableArrayValue = global::System.Collections.Immutable.ImmutableArray.ToImmutableArray(src.ImmutableArrayValue);
target.ImmutableListValue = global::System.Collections.Immutable.ImmutableList.ToImmutableList(src.ImmutableListValue);
target.ImmutableQueueValue = global::System.Collections.Immutable.ImmutableQueue.CreateRange(src.ImmutableQueueValue);
target.ImmutableStackValue = global::System.Collections.Immutable.ImmutableStack.CreateRange(src.ImmutableStackValue);
target.ImmutableSortedSetValue = global::System.Collections.Immutable.ImmutableSortedSet.ToImmutableSortedSet(src.ImmutableSortedSetValue);
target.ImmutableDictionaryValue = global::System.Collections.Immutable.ImmutableDictionary.ToImmutableDictionary(src.ImmutableDictionaryValue);
target.ImmutableSortedDictionaryValue = global::System.Collections.Immutable.ImmutableSortedDictionary.ToImmutableSortedDictionary(src.ImmutableSortedDictionaryValue);
target.ImmutableArrayValue = src.ImmutableArrayValue;
target.ImmutableListValue = src.ImmutableListValue;
target.ImmutableQueueValue = src.ImmutableQueueValue;
target.ImmutableStackValue = src.ImmutableStackValue;
target.ImmutableSortedSetValue = src.ImmutableSortedSetValue;
target.ImmutableDictionaryValue = src.ImmutableDictionaryValue;
target.ImmutableSortedDictionaryValue = src.ImmutableSortedDictionaryValue;
foreach (var item in src.ExistingISet)
{
target.ExistingISet.Add(item);
Expand Down
8 changes: 7 additions & 1 deletion test/Riok.Mapperly.Tests/Mapping/DictionaryCustomTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,13 @@ public void ReadOnlyDictionaryToCustomTypeReadOnlyReadOnlyDictionaryShouldIgnore
TestHelper
.GenerateMapper(source, TestHelperOptions.AllowDiagnostics)
.Should()
.HaveDiagnostic(DiagnosticDescriptors.CannotMapToReadOnlyMember)
.HaveDiagnostics(
DiagnosticDescriptors.SourceMemberNotMapped,
"The member Keys on the mapping source type System.Collections.Generic.IReadOnlyDictionary<string, string> is not mapped to any member on the mapping target type A",
"The member Values on the mapping source type System.Collections.Generic.IReadOnlyDictionary<string, string> is not mapped to any member on the mapping target type A",
"The member Count on the mapping source type System.Collections.Generic.IReadOnlyDictionary<string, string> is not mapped to any member on the mapping target type A"
)
.HaveAssertedAllDiagnostics()
.HaveMapMethodBody(
"""
var target = new global::A();
Expand Down
13 changes: 9 additions & 4 deletions test/Riok.Mapperly.Tests/Mapping/DictionaryImmutableTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,11 @@ public void EnumerableToReadOnlyImmutableDictionaryShouldDiagnostic()
TestHelper
.GenerateMapper(source, TestHelperOptions.AllowDiagnostics)
.Should()
.HaveDiagnostic(DiagnosticDescriptors.CannotMapToReadOnlyMember)
.HaveDiagnostic(DiagnosticDescriptors.SourceMemberNotMapped)
.HaveDiagnostics(
DiagnosticDescriptors.CannotMapToReadOnlyType,
"Cannot map to read-only type System.Collections.Immutable.ImmutableDictionary<string, string>",
"Cannot map to read-only type System.Collections.Immutable.ImmutableDictionary<string, string>"
)
.HaveAssertedAllDiagnostics();
}

Expand All @@ -114,8 +117,10 @@ public void EnumerableToReadOnlyImmutableSortedDictionaryShouldDiagnostic()
TestHelper
.GenerateMapper(source, TestHelperOptions.AllowDiagnostics)
.Should()
.HaveDiagnostic(DiagnosticDescriptors.CannotMapToReadOnlyMember)
.HaveDiagnostic(DiagnosticDescriptors.SourceMemberNotMapped)
.HaveDiagnostic(
DiagnosticDescriptors.CannotMapToReadOnlyType,
"Cannot map to read-only type System.Collections.Immutable.ImmutableSortedDictionary<string, string>"
)
.HaveAssertedAllDiagnostics();
}
}
Loading

0 comments on commit f881652

Please sign in to comment.