riok / mapperly

A .NET source generator for generating object mappings. No runtime reflection.
https://mapperly.riok.app
Apache License 2.0
2.9k stars 146 forks source link

Allow the usage of external methods with `MapProperty.Use`, `MapPropertyFromSource.Use` and `MapValue.Use` #1298

Open Kibonik opened 5 months ago

Kibonik commented 5 months ago

Describe the bug

UseStaticMapper doesnt work with MapPropertyFromSource

Code

    public class SomeTest
    {
        public int Id { get; set; }
        public string? Number { get; set; }
        public string? Prefix { get; set; }
    }
    public class SomeTestDto
    {
        public int Id { get; set; }
        public string? NumberFull { get; set; }

    }
    [Mapper]
    [UseStaticMapper(typeof(TestMapper))]
    public static partial class SomeMapper
    {
        [MapPropertyFromSource(nameof(SomeTestDto.NumberFull), Use = nameof(MapProducer))] //or Use = nameof(TestMapper.MapProducer)
        public static partial SomeTestDto MapMe(SomeTest s);
    }

    public static class TestMapper
    {
        public static string? MapProducer(SomeTest s)
                   => s.Prefix + s.Number;
    }

Generated code

            public static partial global::TimberErp.Shared.Models.LoadMovesDtoMapper.SomeTestDto MapMe(global::TimberErp.Shared.Models.LoadMovesDtoMapper.SomeTest s)
            {
                var target = new global::TimberErp.Shared.Models.LoadMovesDtoMapper.SomeTestDto();
                target.Id = s.Id;
                target.NumberFull = s.ToString();
                return target;
            }

Error

Severity    Code    Description Project File    Line    Suppression State
Error   CS0103  The name 'MapProducer' does not exist in the current context    
Error   RMG061  The referenced mapping named MapProducer was not found  
latonz commented 5 months ago

This is not supported and isn't expected to work. Right now Use does only allow methods of the same class and its type hierarchy. This needs some concept on how to address these external methods (probably use a "full-name" concept similar to the one we use for properties (see docs)?)...

IanKemp commented 1 month ago

100% support the @nameof / "fullnameof" special-casing as a way to achieve this feature, especially since the C# language implementers can't be bothered to implement it. I would consider this a "small feature" as outlined in your contribution docs, @latonz would you agree and is this something you would be happy to entertain a PR to implement?

latonz commented 1 month ago

@IanKemp Go for it! 😊 I'm looking forward to review your PR. Note: the implementation should work for all possible variations (e.g. a static method on a static type, an instance method on a field of the mapper class, ...).

IanKemp commented 1 month ago

@latonz Would appreciate a pointer on the architectural approach before I go ahead and jump into implementation.

From my initial investigation it seems that the simplest way to implement this would be to change MemberMappingConfiguration.Use from being typed as string? to StringMemberPath? and then special-case handling of the latter type in AttributeDataAccessor.BuildArgumentValue (which currently is only handling the non-nullable version). I'd probably also have to pass the targetType to CreateMemberPath such that the latter can differentiate between "short full nameof" (current behaviour, used for MemberMappingConfiguration.Source and Target) and "actual full nameof" (new behaviour for Use). This approach is not great because if there should ever be a desire to change the nullability of any of the three aforementioned properties, then their behaviour will also implicitly change.

Then I looked at making StringMemberPath an abstract record class as opposed to the record struct it currently is; having two derived types, ShortFullNameOfStringMemberPath for Source and Target and FullNameOfStringMemberPath for Use; and then special-casing BuildArgumentValue and CreateMemberPath on these discrete derived types. The problem there is that changing struct to class has semantics around allocation and therefore performance, and of course source generators (and indeed anything Roslyn-related) is performance-sensitive, so this seems like a bit of an invasive change for the desired feature.

Thoughts?

latonz commented 1 month ago

@IanKemp instead of adding nullable value type handling to all of the arms in AttributeDataAccessor.BuildArgumentValue, we could simply use Nullable.GetUnderlyingType and overwrite the targetType if the underlying type is non-null.

The second problem with the .Skip(1): i really dislike this Skip(1) and I planned already to rework this to get the fullnameof feature to work also with namespaced/nested type references. I think with the help of SemanticModel.GetOperation we could directly get the full path to the target member. I think if we could use such an approach, there would be no need for a string representation of the member path, instead a symbol member path could be used. If that doesn't work out, I'd probably simply create a completely separate FullStringMemberPath for now (and document the differences in a xml comment).

latonz commented 1 month ago

@IanKemp I pushed my experiments here https://github.com/riok/mapperly/pull/1518, see especially Riok.Mapperly.Configuration.SymbolMemberPath which could be what you need.