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

Improve SA1131 to also consider methods as constants #3677

Closed
bjornhellander opened this issue Jul 13, 2023 · 4 comments · Fixed by #3710
Closed

Improve SA1131 to also consider methods as constants #3677

bjornhellander opened this issue Jul 13, 2023 · 4 comments · Fixed by #3710

Comments

@bjornhellander
Copy link
Contributor

bjornhellander commented Jul 13, 2023

The following code triggers SA1131 (Use readable conditions) :

// Totally contrived example code :-) Only here to illustrate the current behaviour.
class TestClass
{
    private readonly System.Action myAction;

    public TestClass(bool stuff)
    {
        myAction = stuff ? DoSomething : DoSomethingElse;
    }

    public bool MyCondition => myAction == DoSomething; // SA1131 here!

    private void DoSomething()
    {
    }

    private void DoSomethingElse()
    {
    }
}

The code fix changes the condition like this:
public bool MyCondition => DoSomething == myAction;

DoSomething is in effect a constant in that condition, so I think that SA1131 should not be triggered.
Unusual case, but might be worth considering if the change is easy?

Tested with 1.2.0-beta.507.

@sharwell
Copy link
Member

Are you sure that example produces SA1131?

@bjornhellander
Copy link
Contributor Author

But what the... No, it doesn't. But it did. I think. :-)
Let me try to de-simplify this again. I'll be back.

@bjornhellander
Copy link
Contributor Author

bjornhellander commented Jul 25, 2023

Ah, it triggers if the field is static readonly. The example needs some changes, but this triggers for example:

class TestClass
{
    private static readonly System.Action myAction;

    static TestClass()
    {
        var someMoreElaborateCondition = true;
        myAction = someMoreElaborateCondition ? DoSomething : DoSomethingElse;
    }

    public bool MyCondition => myAction == DoSomething; // SA1131 here!

    private static void DoSomething()
    {
    }

    private static void DoSomethingElse()
    {
    }
}

@sharwell
Copy link
Member

It seems fine to make this change.

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

Successfully merging a pull request may close this issue.

2 participants