Roslyn: Report a diagnostic for assignment to this with readonly fields

[sharwell]

This is related to #30240 and #30262.

A diagnostic should be reported if an assignment to this appears outside the constructor of a struct when the type contains one or more fields marked readonly.

[KristianJakubik]

Hi, I would like to work on this, if it is possible.

[sharwell]

@KristianJakubik that would be awesome! Since this seems more related to code quality than code style, it would likely follow the new pattern used by @mavasani for the dead code analyzer (a simpler pattern that doesn't need updates to the Code Style UI or .editorconfig). This analyzer is also much less complex than the dead code analyzer.

The code fix would remove readonly from all instance fields of the type. I'm getting pulled onto some other tasks in parallel, so if I miss a question from you feel free to ask @mavasani and/or @jcouv. 😄

[KristianJakubik]

Hi @mavasani ,
would you mind providing some link to implementation of yours, based on which I could get inspired and could help me better solve this issue.
@sharwell advised me, that you solved similar problems already.
Thanks.

[mavasani]

@KristianJakubik Implementation of dead code analyzer is here. It looks for references to member symbols, classifies them as read/write, and then reports diagnostics.

You probably want a similar symbol start/end analyzer for SymbolKind.NamedType, then register following nested actions on SymbolStartAnalysisContext:

1. Symbol action for SymbolKind.Field to compute if all fields defined in the named type are readonly or not.
2. Operation action for OperationKind.SimpleAssignment to check if the underlying IAssignmentOperation.Target is an IInstanceReferenceOperation with the required instance reference kind.
3. SymbolEnd action to report diagnostics on operations identified in 2. if you did not find any non-readonly field reported in 1.

Let me know if you have questions!

[KristianJakubik]

Hi @jcouv
I would like to ask:
What IDEDiagnosticIds should I use for my diagnostic.

Is the analyzer's name MakeStructFieldsWritable sufficient for you and is it keeping up with Roslyn standards.

[CyrusNajmabadi]

What IDEDiagnosticIds should I use for my diagnostic.

You can add a new diagnostic ID to that type, picking an unused name/value.

Is the analyzer's name MakeStructFieldsWritable sufficient for you and is it keeping up with Roslyn standards.

That would be fine with me. It would really depend on how it finally ended up working out in practice. If it really did nothing more than that, then the name would fit fine.

[KristianJakubik]

@CyrusNajmabadi
Hope nobody is doing something in parallel and we won't get into the conflict.
Just for the record, I will go with IDE0058.
Thanks

[CyrusNajmabadi]

Hope nobody is doing something in parallel and we won't get into the conflict

There's lots happening in parallel. Conflicts are easily taken care of :) No one happens to be working on a 'make fields writable' feature, so there's no concern there.

[KristianJakubik]

@CyrusNajmabadi
Hi,
I would like to know, if this solution should be implemented for VB.NET as well.
I've only implemented for C# yet, because this is interim solution for readonly struct feature and it is not supported in VB, or at least I was not able to find any info.

I would like to ask, if analyzer should cover the casein which readonly properties are used instead of readonly fields:

struct S
    {
        public S(int value) => this.Value = value;

        public int Value { get; }

        public void Mutate()
        {
            this = new S(Value + 1);
        }
    }

    static class C
    {
        static S v;

        static void Main()
        {
            v.Mutate();
            Console.WriteLine(v.Value);
        }
    }

[mavasani]

Fixed with https://github.com/dotnet/roslyn/pull/32063