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.

Sign up for GitHub

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

Jump to bottom

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

Open
cston opened this issue Aug 10, 2022 · 3 comments · May be fixed by #76263
Open

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

cston opened this issue Aug 10, 2022 · 3 comments · May be fixed by #76263
Assignees
@jjonescz
Labels
Area-Compilers Bug Feature - Ref Fields
Milestone
17.13

Comments

@cston
Copy link
Member

cston commented Aug 10, 2022 •
edited
Loading 

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.
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Aug 10, 2022
@cston cston self-assigned this Aug 10, 2022
@jaredpar jaredpar added Bug and removed untriaged Issues and PRs which have not yet been triaged by a lead labels Aug 10, 2022
@jaredpar jaredpar added this to the 17.4 milestone Aug 10, 2022
@RikkiGibson
Copy link
Contributor

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:

roslyn/src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs

Lines 4131 to 4135 in 3a17b7b

if (UseUpdatedEscapeRules &&
call.Method.Parameters[0].EffectiveScope == DeclarationScope.ValueScoped)
{
continue;
}

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
Copy link
Member

jaredpar commented Oct 5, 2022 •
edited
Loading

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===

@jaredpar jaredpar closed this as completed Oct 5, 2022
@jaredpar jaredpar reopened this Oct 5, 2022
@jaredpar jaredpar modified the milestones: 17.4, 17.5 Oct 10, 2022
@jaredpar jaredpar modified the milestones: 17.5, 17.6 Jan 5, 2023
@jaredpar jaredpar assigned jaredpar and unassigned cston Jul 11, 2024
@jaredpar jaredpar modified the milestones: 17.6, 17.12 Jul 11, 2024
@jjonescz jjonescz mentioned this issue Nov 7, 2024
Open
@jcouv jcouv modified the milestones: 17.12, 17.13 Nov 12, 2024
@arunchndr arunchndr modified the milestones: 17.13, 17.13 P2 Nov 13, 2024
@jaredpar jaredpar modified the milestones: 17.13 P2, 17.13 Dec 2, 2024
@jjonescz
Copy link
Member

jjonescz commented Dec 4, 2024

This is no longer a bug

The bug still exists, see #75802 (comment).

I'm working on a fix.

@jjonescz jjonescz self-assigned this Dec 4, 2024
@jjonescz jjonescz linked a pull request Dec 4, 2024 that will close this issue
Draft
@jjonescz jjonescz mentioned this issue Dec 12, 2024
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jjonescz jjonescz

Labels
Area-Compilers Bug Feature - Ref Fields
Projects
None yet
Milestone
17.13
Development

Successfully merging a pull request may close this issue.

Check ref safety of arg mixing in interpolated strings jjonescz/roslyn
6 participants
@jaredpar @jjonescz @RikkiGibson @cston @jcouv @arunchndr