Roslyn: Incorrect IDE0044 when NRTs are enabled, the class is generic, and the field is only set by a non-this reference

[jnm2]

Visual Studio 16.3.1 and 16.4p1

IDE0044 should not be reported.

class GenericClass<T>
{
    // IDE0044 Make field readonly
    private int field;

    public static void Method(GenericClass<T> instance)
    {
        instance.field++;
    }
}

Similarly:

class GenericClass<T>
{
    // IDE0044 Make field readonly
    private int field;

    public struct Helper
    {
        private readonly GenericClass<T> instance;

        public Helper(GenericClass<T> instance)
        {
            this.instance = instance;
        }

        public void Method()
        {
            instance.field++;
        }
    }
}

Csproj:

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

  <PropertyGroup>
    <TargetFramework>netcoreapp3.0</TargetFramework>
    <LangVersion>latest</LangVersion>
    <Nullable>enable</Nullable>
  </PropertyGroup>

</Project>

[CyrusNajmabadi]

Neither cases repro for me on 16.4p1:

[jnm2]

I'm reproing on 16.4p1. Did you press Ctrl+. on the line? The default visibility wouldn't show any visual.

To help me quickly see when it was present when I was trimming down the repro, I used this editorconfig:

root = true

[*]
dotnet_style_readonly_field = true:error

[CyrusNajmabadi]

Crazy! And yes, i've got this feature turned on. I'm also on:

[jnm2]

So sorry, I forgot that I had added <Nullable>enable</Nullable> to the project file. Updated at the top.

[CyrusNajmabadi]

Yup. I repro now. Fascinating!

[CyrusNajmabadi]

Tagging @mavasani . Note the necessity to update your project file. This didn't repro for me in a 4.7.2 project...

[jnm2]

It repros for me on net472 (where I initially discovered it) as well as netcoreapp3.0.
E.g.

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

  <PropertyGroup>
    <TargetFramework>net472</TargetFramework>
    <LangVersion>latest</LangVersion>
    <Nullable>enable</Nullable>
  </PropertyGroup>

</Project>

[mavasani]

Tagging @333fred - seems to be another analyzer affected by https://github.com/dotnet/roslyn/issues/38195

[mavasani]

Also tagging @chsienki who is currently investigating #38195

[CyrusNajmabadi]

Note: if code has to be updated anyways to get the right nullable-behavior, then i'm far more inclined to say that our choices to try to expose nullable through these extra properties seems less useful. After all, if you still need to update to support nullable properly then we could have exposed nullable not through properties at all. In both cases people need to update to have their code work in the presence of nullable :)

[jnm2]

This is marked 'help wanted' but it looks like it's linked to https://github.com/dotnet/roslyn/issues/38195. Is there something a community member could do to move this along?

[mavasani]

Fixing #38195 should fix this issue. @chsienki @333fred do we know which release that fix is targeted for? If that is a some way out, then we can possibly put some workaround in the analyzer.

[chsienki]

It was originally planned for 16.4, but it got put on hold because of all the ISymbol/Symbol separation work going on in the compiler. Should be a fairly easy fix once that work is finished though.

[jnm2]

Still says up-for-grabs. Does that mean that the larger work is too far away?

[jasonmalinowski]

That larger work is now landed. @chsienki what's the next step here then?

[chsienki]

Was actually planning on starting #38195 on Monday :) aiming for 16.5 but we'll see on timelines if it makes it in.

[chsienki]

Fixed via #41123