fix: Insert a cast to IDictionary when an explicit setter is present

[TimothyMakkison]

Resolves #286

• Looks for an explicit setter, casting the target dictionary if one is found. - not sure if GetExplicitIndexer should be in DictionaryMappingBuilder or ForEachSetDictionaryExistingTargetMapping
• All casts use the global type
• Added tests

[codecov-commenter]

Codecov Report

Merging #341 (41c45a0) into main (dce4b42) will increase coverage by 0.32%.
The diff coverage is 92.45%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main     #341      +/-   ##
==========================================
+ Coverage   91.71%   92.03%   +0.32%     
==========================================
  Files         109      122      +13     
  Lines        3379     3894     +515     
  Branches      438      531      +93     
==========================================
+ Hits         3099     3584     +485     
- Misses        189      209      +20     
- Partials       91      101      +10     

Impacted Files	Coverage Δ
src/Riok.Mapperly.Abstractions/MapEnumAttribute.cs	100.00% <ø> (ø)
...berMappings/MemberNullDelegateAssignmentMapping.cs	57.14% <0.00%> (ø)
.../Mappings/NewInstanceObjectFactoryMemberMapping.cs	100.00% <ø> (ø)
...Mapperly/Descriptors/UserMethodMappingExtractor.cs	98.38% <50.00%> (ø)
...riptors/MappingBuilders/QueryableMappingBuilder.cs	68.42% <68.42%> (ø)
...Descriptors/Mappings/EnumFromStringParseMapping.cs	73.68% <73.68%> (ø)
...ptors/Mappings/MemberMappings/NullMemberMapping.cs	76.27% <76.27%> (ø)
src/Riok.Mapperly/Symbols/FieldMember.cs	78.57% <78.57%> (ø)
src/Riok.Mapperly/Symbols/PropertyMember.cs	78.57% <78.57%> (ø)
...lders/NewInstanceObjectMemberMappingBodyBuilder.cs	80.17% <80.17%> (ø)
... and 60 more

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[latonz]

Thanks for another great contribution!
The idea is to use a MappingBuilder to construct a Mapping object. The Mapping then should only transform the given information into a syntax. The MappingBuilder should provide all the required information for the Mapping to do only the transformation.

• This does not fix Enumerable / Dictionary mappings do not support explicit interface implementations #286 for enumerables, does it?
• This does not fix the case when the source implements IDictionary<K, V>, IReadOnlyDictionary<K, V> or IEnumerable<KeyValuePair<K, V>> explicitly, does it?

[TimothyMakkison]

The idea is to use a MappingBuilder to construct a Mapping object. The Mapping then should only transform the given information into a syntax. The MappingBuilder should provide all the required information for the Mapping to do only the transformation.

Gotcha, I'll keep the invocation GetExplicitIndexer out of ForEachDictionary.

• This does not fix Enumerable / Dictionary mappings do not support explicit interface implementations #286 for enumerables, does it?

Where is this a problem for Enumerables? Arrays aren't affected, I don't think the IList<T> indexers are used, foreach will get explicit enumerators and we check for Add.

• This does not fix the case when the source implements IDictionary<K, V>, IReadOnlyDictionary<K, V> or IEnumerable<KeyValuePair<K, V>> explicitly, does it?

Looking at the tests the only thing the source is used for is Enumerating. Do you have an example?

[latonz]

Ah yap your totally right, foreach does work even if the IEnumerable is implemented explicitly. I had in my mind that the enumerator couldn't be created when IEnumerable is implemented explicitly but thats not the case.
So then the only improvement which could be done there would be to support add calls when the interface is implemented explicitly. The dictionary implementation is alright 👍

[TimothyMakkison]

So then the only improvement which could be done there would be to support add calls when the interface is implemented explicitly.

A lot of .Net collections "implement" ICollection, not implementing Add and making it explicit. ie the Immutable collections, Frozen collections, Queue<T>, Stack<T>, LinkedList<T>, HashSet<T>, SortedSet<T>, etc. A lot of these types are unlikely to be used with Mapperly/some won't reach this section of code #312, but they will still break if casted.

I created HasImplicitMethod to prevent Add being called on these types, although in retrospect it would have been simpler to check if Target was a List<T>. SortedSet<T>, HashSet<T> etc and use Add accordingly. If anything implementing ICollection<T> is explicitly cast, we would have to create a large black list filter.

tldr I would be more inclined to delete HasImplicitMethod and instead call Add on specific types.

[latonz]

Thank you for the input, this makes totally sense!

However, I don't think removing HasImplicitMethod would be any better as then a custom ICollection<T> implementation could never be used as a mapping target. This may also be related to #342 (comment). Don't most of these collections which don't throw on add have ctor with a single enumerable parameter?

[TimothyMakkison]

However, I don't think removing HasImplicitMethod would be any better as then a custom ICollection<T> implementation could never be used as a mapping target. This may also be related to #342 (comment). Don't most of these collections which don't throw on add have ctor with a single enumerable parameter?

Good point, #324 would prevent a lot of these cases. Unfortunately the casting problem would still affect TryBuildExistingTargetMapping as the constructor version cannot be used when the target collection already exists.

[latonz]

I rebased the branch via GH and only then saw that there are two commits. Could you squash those two commits together into one feature commit? 😊

[TimothyMakkison]

Thanks for your many reviews 😄 I'll rebase and squish later

[latonz]

@TimothyMakkison this looks ready to merge to me, are you okay with merging?

[TimothyMakkison]

Thanks for your help @latonz, you can merge this request!

[github-actions]

🎉 This PR is included in version 2.8.0-next.2 🎉

The release is available on:

• GitHub release
• v2.8.0-next.2

Your semantic-release bot 📦🚀