Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support From and To static factory methods #1117

Closed
TimothyMakkison opened this issue Feb 14, 2024 · 10 comments · Fixed by #1616
Closed

Support From and To static factory methods #1117

TimothyMakkison opened this issue Feb 14, 2024 · 10 comments · Fixed by #1616
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@TimothyMakkison
Copy link
Collaborator

Is your feature request related to a problem? Please describe.
Many objects provide static methods instead of constructors for semantic reasons. Unfortunately as Mapperly is unable to use them users have to manually provide wrapper methods. Based upon.

Solution

Mapperly should look for static methods that meet the following criteria:

  • static methods on the target type named From{SourceType.Name} accepting the source type as a single parameter and the target type as return type
  • static methods on the source type named To{TargetType.Name} accepting the source type as single parameter returning the target type

Implementation

  • This should be used as a fallback for if there are no compatible constructors. Should look for the target types From methods before looking for the source To methods.
  • Perhaps the sources To methods could include non static methods.
var target = new global::Mapperly_gRPC.HelloReply();
target.Message = response.Message;
target.ResponseTime = Timestamp.FromDateTimeOffset(response.ResponseTime);
return target;

See #1107.

@TimothyMakkison TimothyMakkison added the enhancement New feature or request label Feb 14, 2024
@latonz
Copy link
Contributor

latonz commented Feb 18, 2024

I'm not sure to what you are referring by if there are no compatible constructors (are you referring to the ones used by CtorMappingBuilder or by NewInstanceObjectMemberMappingBuilder). IMO this new mappings should try to be built directly after CtorMappingBuilder.

IMO the following signatures could be supported (all ordinary, non-async methods returning TTarget):

TSource.ToTTarget()
static TTarget.Create(TSource)
static TTarget.FromTSource(TSource)
static TSource.ToTTarget(TSource)

I'd split them across two MappingBuilders: ConvertInstanceMethodMappingBuilder and ConvertStaticMethodMappingBuilder.

Hints for a contribution despite the contributing documentation:

  • Create new entries in Riok.Mapperly.Abstractions.MappingConversionType
  • Create new *MappingBuilders, Riok.Mapperly.Descriptors.MappingBuilders.ParseMappingBuilder can be used as an idea how it could work
  • Register the new TryBuildMapping methods in Riok.Mapperly.Descriptors.MappingBuilders.MappingBuilder._builders

@latonz latonz added the good first issue Good for newcomers label Mar 19, 2024
@TonEnfer
Copy link
Contributor

Some system types contain To{Target} methods. For example decimal contains many methods to convert to other numeric types, such as ToByte, ToDouble, etc.
Currently such conversion is done via explicit casting, but adding a new ConvertStaticMethodMappingBuilder before ExplicitCastMappingBuilder will change the existing behavior. Is this ok or should the current behavior be preserved?

Also, in a naive implementation, ConvertInstanceMethodMappingBuilder will generate ToString calls for all X -> string mappings, which will also change the existing behavior, since the call to ConvertInstanceMethodMappingBuilder must be placed much higher than ToStringMappingBuilder. Is the mapping described supposed to be excluded from ConvertInstanceMethodMappingBuilder?

@latonz
Copy link
Contributor

latonz commented Nov 27, 2024

@TonEnfer thanks for your input, very valuable!
I'm not sure about the priority of this new mapping builder 🤔 We could probably also just replace ToStringMappingBuilder with the new ones... This shouldn't break existing code and as I look through the mapping builders in between the ToStringMappingBuilder and the proposed position after CtorMappingBuilder, I think there aren't any really relevant mapping builders. WDYT about replacing ToStringMappingBuilder with the new mapping builders?

@TonEnfer
Copy link
Contributor

It seems to me that ToStringMappingBuilder is a special case of the new ConvertInstanceMethodMappingBuilder, so replacing it with a new, more extensive, mapping builder would be logical.
I am not familiar enough with the project code to judge the consequences of such a decision, I cannot predict what it might lead to. If you think that this will not break compatibility with existing code, then I think this is the way to go.

Also, I can't predict the consequences of calling static To{TTarget} methods instead of explicit casting, which would be the case with ConvertStaticMethodMappingBuilder. It might reduce the performance of the generated code.
The very simple benchmarks I wrote show the following results:

Results

BenchmarkDotNet v0.14.0, Windows 11 (10.0.26100.2314)
AMD Ryzen 9 7950X, 1 CPU, 32 logical and 16 physical cores
.NET SDK 9.0.100
[Host] : .NET 9.0.0 (9.0.24.52809), X64 RyuJIT AVX-512F+CD+BW+DQ+VL+VBMI
.NET 8.0 : .NET 8.0.11 (8.0.1124.51707), X64 RyuJIT AVX-512F+CD+BW+DQ+VL+VBMI
.NET 9.0 : .NET 9.0.0 (9.0.24.52809), X64 RyuJIT AVX-512F+CD+BW+DQ+VL+VBMI
.NET Framework 4.8 : .NET Framework 4.8.1 (4.8.9282.0), X64 RyuJIT VectorSize=256

Method Job Runtime _count Mean Error StdDev Ratio RatioSD
ExplicitCastToByte .NET 8.0 .NET 8.0 1000 2.416 us 0.0311 us 0.0260 us 1.00 0.01
ToByte .NET 8.0 .NET 8.0 1000 2.342 us 0.0416 us 0.0389 us 0.97 0.02
ExplicitCastToByte .NET 9.0 .NET 9.0 1000 2.301 us 0.0441 us 0.0391 us 1.00 0.02
ToByte .NET 9.0 .NET 9.0 1000 2.394 us 0.0468 us 0.0808 us 1.04 0.04
ExplicitCastToByte .NET Framework 4.8 .NET Framework 4.8 1000 2.690 us 0.0217 us 0.0181 us 1.00 0.01
ToByte .NET Framework 4.8 .NET Framework 4.8 1000 2.646 us 0.0492 us 0.0505 us 0.98 0.02
Method Job Runtime _count Mean Error StdDev Median Ratio RatioSD
ExplicitCastToDouble .NET 8.0 .NET 8.0 1000 1.250 us 0.0245 us 0.0472 us 1.243 us 1.00 0.05
ToDouble .NET 8.0 .NET 8.0 1000 1.236 us 0.0247 us 0.0664 us 1.216 us 0.99 0.06
ExplicitCastToDouble .NET 9.0 .NET 9.0 1000 1.226 us 0.0244 us 0.0463 us 1.215 us 1.00 0.05
ToDouble .NET 9.0 .NET 9.0 1000 1.227 us 0.0245 us 0.0381 us 1.218 us 1.00 0.05
ExplicitCastToDouble .NET Framework 4.8 .NET Framework 4.8 1000 4.657 us 0.0896 us 0.1133 us 4.642 us 1.00 0.03
ToDouble .NET Framework 4.8 .NET Framework 4.8 1000 4.445 us 0.0859 us 0.1312 us 4.429 us 0.95 0.04
Code
public class ExplicitCastVsToByte
{
    [Params(1000)]
    public int _count;

    private decimal[] _data;

    [GlobalSetup]
    public void Setup()
    {
        _data = new decimal[_count];

        for (var i = 0; i < _count; i++)
        {
            _data[i] = new Random().Next() % 255;
        }
    }

    [Benchmark(Baseline = true)]
    public byte[] ExplicitCast()
    {
        var result = new byte[_data.Length];

        for (var i = 0; i < _data.Length; i++)
        {
            result[i] = (byte)_data[i];
        }

        return result;
    }

    [Benchmark]
    public byte[] ToByte()
    {
        var result = new byte[_data.Length];

        for (var i = 0; i < _data.Length; i++)
        {
            result[i] = decimal.ToByte(_data[i]);
        }

        return result;
    }
}

public class ExplicitCastVsToDouble
{
    [Params(1000)]
    public int _count;

    private decimal[] _data;

    [GlobalSetup]
    public void Setup()
    {
        _data = new decimal[_count];

        for (var i = 0; i < _count; i++)
        {
            _data[i] = new Random().Next() % 255;
        }
    }

    [Benchmark(Baseline = true)]
    public double[] ExplicitCast()
    {
        var result = new double[_data.Length];

        for (var i = 0; i < _data.Length; i++)
        {
            result[i] = (double)_data[i];
        }

        return result;
    }

    [Benchmark]
    public double[] ToDouble()
    {
        var result = new double[_data.Length];

        for (var i = 0; i < _data.Length; i++)
        {
            result[i] = decimal.ToDouble(_data[i]);
        }

        return result;
    }
}

I don't have enough experience writing this kind of code to be sure that these benchmarks even test what they are supposed to. But if their results are to be believed, then the performance of calling static methods is almost the same as with explicit casting.

The other side of the issue is the possible behavioral inconsistencies that would be introduced by such a code change. What do you think about this?

@latonz
Copy link
Contributor

latonz commented Nov 27, 2024

If we place the ConvertStaticMethodMappingBuilder immediately after the ConvertInstanceMethodMappingBuilder (which replaces the ToStringMappingBuilder), the static method call will have a lower priority than the casts. This change should therefore only affect existing code if a method is defined that would be matched by the new mapping builders, instead of the previously generated instance + member mapping. However, I believe this impact will be minimal, as To*/From* methods are quite specific and unlikely to interfere.

@TonEnfer
Copy link
Contributor

When I was looking at the code that might be related to the implementation of this new functionality, I had the following question.

Currently, ParseMappingBuilder, if it cannot find a method with a suitable signature, returns null.

This situation is reproduced, for example, in the following case: string -> B, B contains static B? Parse(string x).
In this case, Mapperly generates a diagnostic and does not produce any code.

It seemed to me that it would be more logical to call this method with ?? <fallback throw> for nullable ref types and, perhaps, with .GetValueOrDefault() for nullable struct types (Nullable<T>). This would save developers from having to write their own conversion method for such mappings.

For ParseMappingBuilder it is not critical, because I am not sure that such implementation is possible and occurs in real code (or is it possible?), but for the new ConvertStaticMethodMappingBuilder it seems real.
Do you think that new builders should keep the existing behavior similar to ParseMappingBuilder?

@latonz
Copy link
Contributor

latonz commented Nov 27, 2024

Currently, Mapperly aims to handle nullable reference types (NRTs) flexibly, especially within the NullableMappingBuilder. It also "upgrades" all types with NRTs disabled to nullable-annotated types. The idea behind this is to centralize all nullable-related handling. While this approach was intended to simplify mapping, it has resulted in numerous bugs (e.g. #1293, #1611). We recognize the need for improvement, but addressing these issues requires considerable effort, and we currently lack the resources to make the necessary changes.
For now, I suggest implementing these new mapping builders in a similar manner to the existing ones, as this appears to be an edge case. To address the nullable handling situation, we should carefully evaluate how we want to improve it while keeping the approach as simple as possible.

@TonEnfer
Copy link
Contributor

Thanks for all the clarifications.

I plan to work on this feature, but I'm not sure I'll have enough time to do it well enough to share it with the community.
At least your clarifications will be useful to someone else if they want to do it and I don't have time to finish the work by then.

@latonz
Copy link
Contributor

latonz commented Nov 27, 2024

Thank you for your willingness to contribute to Mapperly! I've assigned the issue to you 😊. If you find you don’t have the time to work on it, feel free to unassign yourself. Don’t hesitate to reach out if you have any questions!

@TonEnfer
Copy link
Contributor

Hi @latonz
I have prepared a draft pull request, but I am not sure if I have done everything necessary to request your review.

TonEnfer added a commit to TonEnfer/mapperly that referenced this issue Dec 1, 2024
TonEnfer added a commit to TonEnfer/mapperly that referenced this issue Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants