Generator crashes if [CreateSyncVersion] is applied to overloads

[atifaziz]

Suppose the following (taken from tests):

public partial class Stuff
{
    [CreateSyncVersion]
    static async Task<int> HalfCheckSumAsync(Memory<byte> buffer, Stream stream, CancellationToken ct)
        => (await ChecksumReadAsync(buffer, stream, ct).ConfigureAwait(false)) / 2;

    [CreateSyncVersion]
    static async Task<int> ChecksumReadAsync(Memory<byte> buffer, Stream stream, CancellationToken ct)
    {
        int bytesRead = await stream.ReadAsync(buffer, ct).ConfigureAwait(true);
        return Checksum(buffer.Span.Slice(0, bytesRead));
    }
    static int Checksum(Span<byte> buffer) => 0;
}

Suppose further that the developer wants to offer another overload of HalfCheckSumAsync without the ct parameter:

public partial class Stuff
{
    [CreateSyncVersion]
    static Task<int> HalfCheckSumAsync(Memory<byte> buffer, Stream stream) =>
        HalfCheckSumAsync(buffer, stream, CancellationToken.None);

    [CreateSyncVersion]
    static async Task<int> HalfCheckSumAsync(Memory<byte> buffer, Stream stream, CancellationToken ct)
        => (await ChecksumReadAsync(buffer, stream, ct).ConfigureAwait(false)) / 2;

    [CreateSyncVersion]
    static async Task<int> ChecksumReadAsync(Memory<byte> buffer, Stream stream, CancellationToken ct)
    {
        int bytesRead = await stream.ReadAsync(buffer, ct).ConfigureAwait(true);
        return Checksum(buffer.Span.Slice(0, bytesRead));
    }
    static int Checksum(Span<byte> buffer) => 0;
}

Upon compilation, the following warning is emitted:

CSC : warning CS8785: Generator 'SyncMethodSourceGenerator' failed to generate source. It will not contribute to the output and compilation errors may occur as a result. Exception was of type 'ArgumentException' with message 'The hintName '.Stuff.HalfCheckSumAsync.g.cs' of the added source file must be unique within a generator. (Parameter 'hintName')'

You could say that the overload doesn't need [CreateSyncVersion] because it just delegates the implementation, but the generator should emit a diagnostic rather than crashing.

I also wonder what are you thoughts on overloads, whether those should be supported by-design or not.

[virzak]

I agree, there shouldn't be a crash and this case should return proper diagnostics message.

I'd say that overloads should be supported as long as they don't converge to same parameter list as in the example above.

[virzak]

da4385d fixes the overload crash. Diagnostics is still on the todo list, but this should allow anyone to move forward.

[atifaziz]

When I look at the unit test and snapshots in da4385d, then something is off. First, the two generated files, ending in ReadAsMemoryAsync.g.verified.cs and ReadAsMemoryAsync_2.g.verified.cs, look identical, which means only one of the overloads got picked up. Second, the test doesn't really exercise how one would really write such overloads where code is duplicated into each. In general, most overloads of method will simply defer/delegate to the most specific or parameterzied one, like so:

[CreateSyncVersion]
Task ReadAsMemoryAsync(Stream stream, byte[] sampleBytes)
    => ReadAsMemoryAsync(stream, sampleBytes, CancellationToken.None);

[CreateSyncVersion]
async Task ReadAsMemoryAsync(Stream stream, byte[] sampleBytes, CancellationToken ct)
    => await stream.ReadAsync(sampleBytes.AsMemory(0, 123), ct).ConfigureAwait(false);

Note that the first overload doesn't need any async-await.

[virzak]

look identical, which means only one of the overloads got picked up.

Both of them got picked up, but they got reduced to the same thing. This would cause compilation issue, so this test case will be spitting out a warning or error in the future.

Second, the test doesn't really exercise how one would really write such overloads where code is duplicated into each. In general, most overloads of method will simply defer/delegate to the most specific or parameterzied one, like so:

[CreateSyncVersion]
Task ReadAsMemoryAsync(Stream stream, byte[] sampleBytes)
    => ReadAsMemoryAsync(stream, sampleBytes, CancellationToken.None);

[CreateSyncVersion]
async Task ReadAsMemoryAsync(Stream stream, byte[] sampleBytes, CancellationToken ct)
    => await stream.ReadAsync(sampleBytes.AsMemory(0, 123), ct).ConfigureAwait(false);

Note that the first overload doesn't need any async-await.

Yeah, will definitely provide better diagnostics for that exact scenario. The crash was affecting all overloads, even with completely different parameters, so I wanted to get that out of the way first.

[atifaziz]

Understood. Happy then if you want to close this issue as done since the generator doesn't crash anymore.

[virzak]

I'll keep it open just so that I don't need to create a new issue and fix it in the next few days.

Also wanted to confirm that this issue isn't a showstopper for you if you decided to integrate this library into moreLINQ.

[atifaziz]

Also wanted to confirm that this issue isn't a showstopper for you if you decided to integrate this library into moreLINQ.

No blocker at the moment as I am still very early in the evaluation phase, but one big plus has been all your prompt support! I might continue with a bigger spike when I have time next and would be happy to report issues if you're interested?

[virzak]

Very interested, thanks!