dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.05k stars 4.03k forks source link

Escape analysis ignores local passed by reference to interpolated string handler `AppendFormatted()` method #63306

Open cston opened 2 years ago

cston commented 2 years ago
using System;
using System.Runtime.CompilerServices;

[InterpolatedStringHandler]
ref struct CustomHandler
{
    private ref readonly int _i;
    public CustomHandler(int literalLength, int formattedCount) { }
    public void AppendFormatted(in int i) { _i = ref i; }
}

class Program
{
    static CustomHandler F1()
    {
        int i = 1;
        return $"{i}"; // error?
    }

    static CustomHandler F2()
    {
        CustomHandler h = $"{2}"; // error?
        return h;
    }
}

Expected: Escape analysis should report errors for the conversions from interpolated string to CustomHandler since a reference to the local is potentially returned to the calling method.

Actual: No errors.

RikkiGibson commented 2 years ago

In this bit, it seems like we weren't precisely correct in our assumptions on how the calls within interpolated string conversions could be formed. An unscoped in-parameter could come in here which we don't handle properly: https://github.com/dotnet/roslyn/blob/3a17b7bca601565f3629ac62e964e02312183eb7/src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs#L4131-L4135

I think we could avoid entirely making assumptions about the shape of the call here, and instead just pass it through the appropriate general purpose helper for calls (GetInvocationArgumentsForEscape or GetInvocationArgumentsForEscapeWithUpdatedRules in #62886). Another layer may be synthesizing the call within some limited set of constraints, but it doesn't matter to the escape analysis layer.

jaredpar commented 2 years ago

This is no longer a bug because the in here is RTSE of return only so it can't be captured. A very related scenario is if a ref struct is passed by value that is tracked by our current code. There is no test for that scenario though. This bug will now track updating the existign test for in and adding a new test for passing a ref struct by value

https://sharplab.io/#v2:EYLgtghglgdgNAFxFANnAJiA1AHwAIBMAjALABQeADAAR5EB0ASgK4wJRgCm9AwgPZgADqk4AnAMpiAblADGnAM4BucuQDaASTZjBfFBASd04hKNgBzABIQY6FGIC65UZwBm1BaeayE1Hs08Ba1t7UXIAb3JqaOpBMykDTmpxQRsAHlgEAD5qAH1XKE4UdBUyGNoAZj8AhCCbOzEACkzqFChDUQgUABlOGHMEAAs4ahbXPlFIBEN0flYEAEpqcOoAXyiYvCq8ABZqAEFBQT70ADEJqZnGlPTMnIVUmCWV/MLi6gBeD0elNfJ1sjkQi0IgAdgiG2idAAbNVAmBgg1RNRTkRGgtIctMeUbjAMmx7o9Ph4EBBZABrLooPiyUZsNQ7AgOUrlcp4UHUAAkACJwg8bKtub8APTC6hiUQTAD8mIBqyAA===