Use pattern matching for null guards

[trejjam]

Use pattern matching for null guards

Description

Generate null guards using pattern matching:

if (source.MayBeNull is {} sourceMayBeNull)
{
    target.NonNullableValue = sourceMayBeNull;
}

Fixes #843

Checklist

• The existing code style is followed
• The commit message follows our guidelines
• Performed a self-review of my code
• Hard-to-understand areas of my code are commented
• The documentation is updated (as applicable)
• Unit tests are added/updated
• Integration tests are added/updated (as applicable, especially if feature/bug depends on roslyn or framework version in use)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (21fdc99) 91.49% compared to head (6bcbba0) 91.54%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #864      +/-   ##
==========================================
+ Coverage   91.49%   91.54%   +0.05%     
==========================================
  Files         215      215              
  Lines        7217     7263      +46     
  Branches      872      875       +3     
==========================================
+ Hits         6603     6649      +46     
  Misses        408      408              
  Partials      206      206              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[trejjam]

I will squash it to the required commit message after the review

[latonz]

Thank you for your efforts and this PR. I first looked at it some time ago but wasn't really happy with the design of the solution. In the meantime I've tried to come up with alternative designs, but haven't come up with a better solution. Tracking the member prefix in the context looks like a "hack" to me and may cause problems later on. I'll keep this PR open for now as I continue to think about alternative designs. Please feel free to comment any ideas you might have.

[latonz]

Wouldn't be nullConditionalSourcePath.Member.Name with the first char as lowercase almost as meaningful when reading the code but would simplify the handling here a lot?

[trejjam]

I can simplify the name, my goal was to prevent name duplicities. But they are handled in the UniqueNameBuilder

[latonz]

This is really hard to understand without any documentation. Did I got it correctly: it stores a source path which should be trimmed from any source base access which uses this context? I'm not really happy about this solution 🤔 Let me think about alternative ways on how to achieve this.

[trejjam]

I can work a bit on the documentation side.

This property should contain A, B when a full path is A.B?.C so it can replace A.B with the newly introduced guard variable.

[trejjam]

No worries, I understand it as a one-at-the-time. I was ok to wait until the static PR made it :)

[trejjam]

Hello @TimothyMakkison

I run a small benchmark:

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Jobs;

namespace Sandbox;

public record A(int Value);

public record D(int Value);

public record struct As(int Value);

public record struct Ds(int Value);

public record B(A? Value);

public record E(D? Value);

public record struct Bs(As? Value);

public record struct Es(Ds? Value);

[SimpleJob(RuntimeMoniker.Net70)]
[SimpleJob(RuntimeMoniker.Net80)]
[MemoryDiagnoser]
public class Bench
{
    private readonly B[] _source =
    {
        new(new A(1)),
        new(null),
    };

    private readonly Bs[] _sourceStruct =
    {
        new(new As(1)),
        new(null),
    };

    [Benchmark]
    public List<E> Map()
    {
        var result = new List<E>(_source.Length);
        foreach (var x in _source)
        {
            if (x.Value != null)
            {
                result.Add(new E(new D(x.Value.Value)));
            }
            else
            {
                result.Add(new E(null));
            }
        }

        return result;
    }

    [Benchmark]
    public List<E> Map_Guard()
    {
        var result = new List<E>(_source.Length);
        foreach (var x in _source)
        {
            if (x.Value is { } val)
            {
                result.Add(new E(new D(val.Value)));
            }
            else
            {
                result.Add(new E(null));
            }
        }

        return result;
    }

    [Benchmark]
    public List<Es> Map_Struct()
    {
        var result = new List<Es>(_sourceStruct.Length);
        foreach (var x in _sourceStruct)
        {
            if (x.Value != null)
            {
                result.Add(new Es(new Ds(x.Value.Value.Value)));
            }
            else
            {
                result.Add(new Es(null));
            }
        }

        return result;
    }

    [Benchmark]
    public List<Es> Map_Struct_Guard()
    {
        var result = new List<Es>(_sourceStruct.Length);
        foreach (var x in _sourceStruct)
        {
            if (x.Value is { } val)
            {
                result.Add(new Es(new Ds(val.Value)));
            }
            else
            {
                result.Add(new Es(null));
            }
        }

        return result;
    }
}

with the following result:

BenchmarkDotNet v0.13.10, Windows 11 (10.0.22621.2715/22H2/2022Update/SunValley2)
12th Gen Intel Core i7-1260P, 1 CPU, 16 logical and 12 physical cores
.NET SDK 8.0.100
  [Host]   : .NET 8.0.0 (8.0.23.53103), X64 RyuJIT AVX2
  .NET 7.0 : .NET 7.0.14 (7.0.1423.51910), X64 RyuJIT AVX2
  .NET 8.0 : .NET 8.0.0 (8.0.23.53103), X64 RyuJIT AVX2


| Method           | Job      | Runtime  | Mean     | Error    | StdDev   | Median   | Gen0   | Allocated |
|----------------- |--------- |--------- |---------:|---------:|---------:|---------:|-------:|----------:|
| Map              | .NET 7.0 | .NET 7.0 | 26.95 ns | 0.593 ns | 0.990 ns | 26.75 ns | 0.0153 |     144 B |
| Map_Guard        | .NET 7.0 | .NET 7.0 | 26.59 ns | 0.764 ns | 2.078 ns | 26.35 ns | 0.0153 |     144 B |
| Map_Struct       | .NET 7.0 | .NET 7.0 | 19.52 ns | 1.598 ns | 4.660 ns | 17.45 ns | 0.0076 |      72 B |
| Map_Struct_Guard | .NET 7.0 | .NET 7.0 | 19.32 ns | 0.549 ns | 1.603 ns | 19.01 ns | 0.0076 |      72 B |
| Map              | .NET 8.0 | .NET 8.0 | 21.74 ns | 0.739 ns | 2.048 ns | 20.79 ns | 0.0153 |     144 B |
| Map_Guard        | .NET 8.0 | .NET 8.0 | 29.29 ns | 1.817 ns | 5.212 ns | 27.74 ns | 0.0153 |     144 B |
| Map_Struct       | .NET 8.0 | .NET 8.0 | 11.12 ns | 0.427 ns | 1.218 ns | 10.74 ns | 0.0076 |      72 B |
| Map_Struct_Guard | .NET 8.0 | .NET 8.0 | 13.05 ns | 0.805 ns | 2.310 ns | 12.51 ns | 0.0076 |      72 B |

It seems that on .NET7 the guard variable version was a little bit faster. NET8 is faster in general and the naive implementation is also faster. So the PR may be not useful...

[TimothyMakkison]

Hey @trejjam, thanks for writing the benchmarks! For what it's worth the number vary a lot when performing microbenchmarks, how many times have you run this? The change between runs can be quit significant and hard to track.

The drop in time for Map & Map_Struct seems suspiciously large. I wonder if the relevant change is mentioned in Stpehen Toub's .NET 8 optimization blog.

I'll try running your benchmarks later, I'll try looking at IL code but I can't promise anything 😄

[trejjam]

I have run this benchmark 3times (first without struct and memory info) and the numbers were always in this way. I did not dig into the reason why NET8 is so much faster. I accept it as a usual performance improvement :D 6->7->8 I will let you know if I find something in Stephen's blog post.

[TimothyMakkison]

I have run this benchmark 3times (first without struct and memory info) and the numbers were always in this way.

Thanks, my numbers were all over the place 😅

I accept it as a usual performance improvement :D 6->7->8 I will let you know if I find something in Stephen's blog post

Suffering from success 😁. I only mentioned his blog because that's where I usually read about improvements. Please don't read the whole thing on my behalf.