Readonly structs should warn when fields are implicitly copied for member invocation

[RikkiGibson]

The following doesn't produce a warning today, but eventually it should in a warning wave. A similar version where S1.s2 and S1 are non-readonly but S1.M1 is readonly using the readonly members feature (see dotnet/csharplang#1710) should produce the same warning.

Implicit copy for non-readonly method invocation on readonly struct field

public readonly struct S1
{
    public readonly S2 s2;
    public void M1()
    {
        // warn on implicit local copy
        s2.M2();

        // no warning on explicit copy
        var copy = s2;
        copy.M2();
    }
}

public struct S2
{
    public int i;
    public void M2()
    {
        i = 23;
    }
}

Implicit copy for calls to struct base members

In addition, we may want to start warning on invocations of base members on this which require an implicit copy. See dotnet/csharplang#1710 (comment)

public readonly struct S
{
    public readonly int i;
    public void M()
    {
        // warn
        this.ToString();
    }
}

Update: the exact scenario where base members of readonly structs are copied before invocation might require more investigation. SharpLab makes it appear that these receivers are simply passed by ref even though the base methods aren't readonly per-se.

Implicit copy for non-readonly invocation on readonly ref variable

public struct S
{
    public int i;

    public static void M1(in S s)
    {
        // warn on implicit local copy
        s.M2();

        // explicit copy, no warning
        var copy = s;
        copy.M2();
    }

    void M2()
    {
        i = 23;
    }
}

Add/remove event handlers on readonly value-typed variables

using System;

public struct S1
{
    public readonly S2 s2;

    public void M()
    {
        // These implicitly copy 's2' before calling
        // the add/remove methods because 's2' is 'readonly'.
        s2.E += Handler;
        s2.E -= Handler;
    }
    
    private void Handler(EventArgs args)
    {
        
    }
}

public struct S2
{
    public event Action<EventArgs> E;
}

[benaadams]

Related dotnet/roslyn#17310

[RikkiGibson]

Note that we've decided to give a warning out of the box when a non-readonly member is called from a readonly member and the receiver is this. Implicit copy warnings on fields will only be handled by this warning wave, regardless of whether the copy is occurring due to the field being readonly or the containing method being readonly.

This is in part due to the desire to have uniformity between readonly structs where the methods are also marked readonly, and readonly structs where the methods are not marked readonly.

[RikkiGibson]

@jaredpar @jmarolf should we put this in wave 6?

[jaredpar]

It should definitely be a candidate

[jmarolf]

I have no objections to this being in Wave 6. In general, if the compiler team thinks a warning is ok I am certainly not going to have a difference in opinion.

[RikkiGibson]

I would like to implement this warning in the compiler and also ship a code fix to explicitly copy the struct before accessing the member. I think we still want sign-off from runtime team in order to do this, so I'm transferring the issue over to there.

[RikkiGibson]

@jeffhandley @jmarolf could we get this in the triage queue?

[ghost]

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

---

The following doesn't produce a warning today, but eventually it should in a warning wave. A similar version where S1.s2 and S1 are non-readonly but S1.M1 is readonly using the readonly members feature (see dotnet/csharplang#1710) should produce the same warning.

Implicit copy for non-readonly method invocation on readonly struct field

public readonly struct S1
{
    public readonly S2 s2;
    public void M1()
    {
        // warn on implicit local copy
        s2.M2();

        // no warning on explicit copy
        var copy = s2;
        copy.M2();
    }
}

public struct S2
{
    public int i;
    public void M2()
    {
        i = 23;
    }
}

Implicit copy for calls to struct base members

In addition, we may want to start warning on invocations of base members on this which require an implicit copy. See dotnet/csharplang#1710 (comment)

public readonly struct S
{
    public readonly int i;
    public void M()
    {
        // warn
        this.ToString();
    }
}

Update: the exact scenario where base members of readonly structs are copied before invocation might require more investigation. SharpLab makes it appear that these receivers are simply passed by ref even though the base methods aren't readonly per-se.

Implicit copy for non-readonly invocation on readonly ref variable

public struct S
{
    public int i;

    public static void M1(in S s)
    {
        // warn on implicit local copy
        s.M2();

        // explicit copy, no warning
        var copy = s;
        copy.M2();
    }

    void M2()
    {
        i = 23;
    }
}

Add/remove event handlers on readonly value-typed variables

using System;

public struct S1
{
    public readonly S2 s2;

    public void M()
    {
        // These implicitly copy 's2' before calling
        // the add/remove methods because 's2' is 'readonly'.
        s2.E += Handler;
        s2.E -= Handler;
    }
    
    private void Handler(EventArgs args)
    {
        
    }
}

public struct S2
{
    public event Action<EventArgs> E;
}

Author:	RikkiGibson
Assignees:	RikkiGibson
Labels:	area-System.Runtime, untriaged, feature request, Area-Compilers, New Feature - Warning Waves
Milestone:	-

[jaredpar]

A warning that fires on implicit struct copy is going to be very noisy, particularly in customer code. Have problems seeing this as a warning wave because it's not a correctness issue but a performance one. If it were a low volume rule then I could see it being added but this is almost certainly to be a high volume rule (based on past experiences).

This is another case though where it's also really hard to see analyzers getting this right. The number of cases where implicit copies happen are too great. To be 100% it needs to be done in the compiler. But there isn't a great mechanism right now for disabled by default but enable-able by user story that we could hook into.

[RikkiGibson]

I agree the warning should be implemented in the compiler.

I think lots of code bases will be "guilty" of this warning and not care at all about it, and perhaps that means it shouldn't be an enabled-by-default warning even in the latest wave. I feel like this has come up before with other warnings.

I am wondering if the right way would be to make the compiler produce a warning with 'IsSuppressed: true' set on it, and then add a way to "un-suppress" such warnings in the project file. cc @mavasani

[tannergooding]

What about having the compiler do the analysis and expose an API that says whether a copy is being made? That way the compiler doesn't need to surface any error but some new analyzer in dotnet/roslyn-analyzers could then utilize the API to surface the "issue" to the user?

It would be a different approach, but seems like something that might be useful information to have for a range of scenarios, depending on your usage/needs.

[jmarolf]

We do have a performance category for analyzers, and you can enable it with a single line in a project file.

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFramework>net6.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
	 <!-- Indicates that this library is performance sensitive and you want to be told about performance problems-->
	<AnalysisModePerformance>All</AnalysisModePerformance>
  </PropertyGroup>

</Project>

Setting aside the implementation details for a bit I would be fine with a customer experience where this is off by default but if the developer tells us "performance matters in this project" then we can be more aggressive and warn them about things like this.

[RikkiGibson]

What about having the compiler do the analysis and expose an API that says whether a copy is being made? That way the compiler doesn't need to surface any error but some new analyzer in dotnet/roslyn-analyzers could then utilize the API to surface the "issue" to the user?

It would be a different approach, but seems like something that might be useful information to have for a range of scenarios, depending on your usage/needs.

I would want to know of more scenarios besides the one in this issue. I feel like it would be good for the IOperation tree to have enough info to draw all these conclusions without "reimplementing" parts of the compiler so to speak. I do not know whether this is already the case. Ultimately any analyzer that wants to know about this is not going to be able to register one callback and handle all the scenarios correctly. There are loads of edge cases and places where methods are called on things implicitly.

[jmarolf]

Don't want to derail this but there is a general desire to track allocations in general. There is this analyzer: https://github.com/microsoft/RoslynClrHeapAllocationAnalyzer/tree/master/ClrHeapAllocationsAnalyzer but there are lots of things it will miss. Having some API to understand how copys/allocations are going to be done would be useful imho and was certainly one of John Hambys goals for IOperation way back when. Don't think we want to block this issue on that just yet. First, I want to establish what we think a reasonable default behavior for this should be.

[sonnemaf]

I use ErrorProne.NET.Structs for this. Version 0.4.0-beta1 works great. You can even set a size threshold using the error_prone.large_struct_threshold = 16 in the .editorconfig.

Maybe we should ask Sergey Teplyakov what the status is. cc @SergeyTeplyakov

[jaredpar]

I am wondering if the right way would be to make the compiler produce a warning with 'IsSuppressed: true' set on it, and then add a way to "un-suppress" such warnings in the project file.

This is my preferred approach. Today we can effectively produce hidden diagnostics that analyzers can hook and expose. That is very indirect and inefficient (loading a 3rd party assembly into the compiler to turn a diagnostic on). It seems like a better outcome in this cases is to have a warning that is effectively off by default that we can then turn on.

[mavasani]

make the compiler produce a warning with 'IsSuppressed: true' set on it, and then add a way to "un-suppress" such warnings in the project file.

Agree with the overall philosophy, but the suggested implementation seems a bit roundabout way to do it. Instead, a simpler implementation would be to adjust the descriptor created here for each compiler diagnostic to not force isEnabledByDefault: true for all error codes, but instead allow C# and VB implementations to pass in false for isEnabledByDefault for specific error codes. You would not require IsSuppressed: true to be set as the diagnostic itself is disabled by default, and one would need an entry in editorconfig/ruleset to enable this diagnostic ID.

[jmarolf]

Instead, a simpler implementation would be to adjust the descriptor created here for each compiler diagnostic to not force isEnabledByDefault: true for all error codes

ah, cool idea @mavasani! This would make everything light up with minimal changes. If the developer adds something like <AnalysisModePerformance>All</AnalysisModePerformance> to their project file the usual MSBuild goop will activate to pass the correct globalconfig file to the compiler. They could also manually add an entry in an editorconfig file.

[ghost]

Tagging subscribers to this area: @dotnet/area-meta
See info in area-owners.md if you want to be subscribed.

Issue Details

---

The following doesn't produce a warning today, but eventually it should in a warning wave. A similar version where S1.s2 and S1 are non-readonly but S1.M1 is readonly using the readonly members feature (see dotnet/csharplang#1710) should produce the same warning.

Implicit copy for non-readonly method invocation on readonly struct field

public readonly struct S1
{
    public readonly S2 s2;
    public void M1()
    {
        // warn on implicit local copy
        s2.M2();

        // no warning on explicit copy
        var copy = s2;
        copy.M2();
    }
}

public struct S2
{
    public int i;
    public void M2()
    {
        i = 23;
    }
}

Implicit copy for calls to struct base members

In addition, we may want to start warning on invocations of base members on this which require an implicit copy. See dotnet/csharplang#1710 (comment)

public readonly struct S
{
    public readonly int i;
    public void M()
    {
        // warn
        this.ToString();
    }
}

Update: the exact scenario where base members of readonly structs are copied before invocation might require more investigation. SharpLab makes it appear that these receivers are simply passed by ref even though the base methods aren't readonly per-se.

Implicit copy for non-readonly invocation on readonly ref variable

public struct S
{
    public int i;

    public static void M1(in S s)
    {
        // warn on implicit local copy
        s.M2();

        // explicit copy, no warning
        var copy = s;
        copy.M2();
    }

    void M2()
    {
        i = 23;
    }
}

Add/remove event handlers on readonly value-typed variables

using System;

public struct S1
{
    public readonly S2 s2;

    public void M()
    {
        // These implicitly copy 's2' before calling
        // the add/remove methods because 's2' is 'readonly'.
        s2.E += Handler;
        s2.E -= Handler;
    }
    
    private void Handler(EventArgs args)
    {
        
    }
}

public struct S2
{
    public event Action<EventArgs> E;
}

Author:	RikkiGibson
Assignees:	RikkiGibson
Labels:	area-Meta, untriaged, feature request, Area-Compilers, New Feature - Warning Waves
Milestone:	-