Cysharp / MemoryPack

Zero encoding extreme performance binary serializer for C# and Unity.
MIT License
3.29k stars 193 forks source link

CS9192 Argument 1 should be passed with 'ref' or 'in' keyword - In generated code for struct #202

Closed RandallFlagg closed 7 months ago

RandallFlagg commented 11 months ago

Hi Is it possible to fix the generated code so in the output code the CS9192 warning won't show up?

This is the file that it happens in:

C:\MyProj\MemoryPack.Generator\MemoryPack.Generator.MemoryPackGenerator\MyProj.StructFile.MemoryPackFormatter.g.cs

image

vcampoy commented 8 months ago

I have a similar issue, when I find the solution, I'll post it here.

rayfung commented 8 months ago

The following code can reproduce the warning in .NET 8:

[MemoryPackable]
public partial class Foo
{
    public List<Bar> Bars1; // Warning CS9192
    public List<Bar> Bars2 { get; set; } // Warning CS9193
}

[MemoryPackable]
public partial class Bar
{
    public int Id { get; set; }
}

Look at the generated code, it seems that we should add ref before the parameter of Unsafe.AsRef (eg: Unsafe.AsRef(ref value.@Bars1)) ?

[global::MemoryPack.Internal.Preserve]
static void IMemoryPackable<Foo>.Serialize<TBufferWriter>(ref MemoryPackWriter<TBufferWriter> writer, scoped ref Foo? value) 
{
    if (value == null)
    {
        writer.WriteNullObjectHeader();
        goto END;
    }

    writer.WriteObjectHeader(2);
    global::MemoryPack.Formatters.ListFormatter.SerializePackable(ref writer,
        ref System.Runtime.CompilerServices.Unsafe.AsRef(value.@Bars1));
    global::MemoryPack.Formatters.ListFormatter.SerializePackable(ref writer,
        ref System.Runtime.CompilerServices.Unsafe.AsRef(value.@Bars2));

END:
    return;
}
maboroshinokiseki commented 8 months ago

I think it's not too hard to fix, just add two more checks to the generator, one for checking if it's a property, and the other for checking if it's a class type. Or maybe we can just remove the ref, since it seems like that SerializePackable does not modify the variable itself.

rayfung commented 8 months ago

The following code can reproduce the warning in .NET 8:

[MemoryPackable]
public partial class Foo
{
    public List<Bar> Bars1; // Warning CS9192
    public List<Bar> Bars2 { get; set; } // Warning CS9193
}

[MemoryPackable]
public partial class Bar
{
    public int Id { get; set; }
}

Look at the generated code, it seems that we should add ref before the parameter of Unsafe.AsRef (eg: Unsafe.AsRef(ref value.@Bars1)) ?

[global::MemoryPack.Internal.Preserve]
static void IMemoryPackable<Foo>.Serialize<TBufferWriter>(ref MemoryPackWriter<TBufferWriter> writer, scoped ref Foo? value) 
{
    if (value == null)
    {
        writer.WriteNullObjectHeader();
        goto END;
    }

    writer.WriteObjectHeader(2);
    global::MemoryPack.Formatters.ListFormatter.SerializePackable(ref writer,
        ref System.Runtime.CompilerServices.Unsafe.AsRef(value.@Bars1));
    global::MemoryPack.Formatters.ListFormatter.SerializePackable(ref writer,
        ref System.Runtime.CompilerServices.Unsafe.AsRef(value.@Bars2));

END:
    return;
}

I found the above code compiles without warnings in .NET 7 because the signature of Unsafe.AsRef is: public static ref T AsRef<T> (scoped in T source); while in .NET 8 it is: public static ref T AsRef<T> (scoped ref T source);

So there is a breaking change in .NET 8.

rayfung commented 8 months ago

There is no need for ref in ListFormatter.SerializePackable while serialization will not (and should not) change the list reference.

I suggest changing void SerializePackable<T, TBufferWriter>(ref MemoryPackWriter<TBufferWriter> writer, scoped ref List<T?>? value) to void SerializePackable<T, TBufferWriter>(ref MemoryPackWriter<TBufferWriter> writer, List<T?>? value)

hadashiA commented 7 months ago

Hi. Thanks for the suggestions. I fixed in #232.