dbolin / Apex.Analyzers

Roslyn powered analyzers for C# to support convention defined architecture
MIT License
15 stars 2 forks source link

Delegates with mutable parameters are treated as mutable #37

Open mwelsh1118 opened 4 years ago

mwelsh1118 commented 4 years ago

We now have a way to mark arbitrary types immutable via configuration. This allows us to configure System.Action`1, for example, as immutable without impacting other consumers of the analyzers. However, we're still experiencing a fair amount of pain due to the generic type argument checking.

For example this is a violation:

[Immutable]
class Test
{
    public Action<NotImmutable> Action { get; }
}

Even though Action`1 is treated as immutable, its generic type argument (NotImmutable) is not, and so it produces an IMM004.

I know that delegates are treated as mutable as a conscious design choice, but might this be reconsidered? Another corner case (like state-capturing lambdas) that is not covered would be:

[Immutable]
class Test
{
    private readonly object _random;

    public Test(object random)
    {
        _random = random;
    }
    public int Value => ((System.Random)_random).Next(0, 10);
}

I'm not arguing that this should be caught -- simply that there will always be ways to subvert the rules if someone is truly determined to do so.

Alternately, could we add configuration to control the treatment of delegates? The Microsoft analyzers now support options defined in an .editorconfig. Unfortunately their implementation is internal, so cannot be called directly. But the code is open-source and could be incorporated.

dbolin commented 4 years ago

Yeah, the analyzer isn't meant to try to stop anyone from getting around what it is doing - that's part of the reason the onFaith flag exists for immutable type definitions, since there are some things that could be part of an immutable type hierarchy that need to go around these rules for performance or other reasons.

Anyway, the reason the analyzer has to verify the generic type arguments of fields that are an immutable generic type is because something like ImmutableList is only really immutable if T is immutable.

There's more to think about here, but things like this are why the format of the whitelist might need to be structured so we could have an option to check type parameters or not.