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.

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

Delegates with mutable parameters are treated as mutable #37

Open
mwelsh1118 opened this issue May 1, 2020 · 1 comment
Open

Delegates with mutable parameters are treated as mutable #37

mwelsh1118 opened this issue May 1, 2020 · 1 comment

Comments

@mwelsh1118
Copy link
Contributor

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

dbolin commented May 1, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants