roslyn: Please stop adding opinionated style-based code analysis to the error list

I upgraded my build in 15.8, and Error List which was previous “clean” under all categories, had about ~500 of these

Severity	Code	Description	Project	File	Line	Suppression State
Message	IDE0046	'If' statement can be simplified	Microsoft.VisualStudio.Editors	E:\project-system2\src\Microsoft.VisualStudio.Editors\SettingsDesigner\SettingsValueSerializer.vb	90	Active
Message	IDE0046	'If' statement can be simplified	Microsoft.VisualStudio.Editors	E:\project-system2\src\Microsoft.VisualStudio.Editors\SettingsDesigner\SettingsValueSerializer.vb	145	Active
Message	IDE0046	'If' statement can be simplified	Microsoft.VisualStudio.Editors	E:\project-system2\src\Microsoft.VisualStudio.Editors\SettingsDesigner\TypeEditorHostControl.vb	513	Active
Message	IDE0046	'If' statement can be simplified	Microsoft.VisualStudio.Editors	E:\project-system2\src\Microsoft.VisualStudio.Editors\SettingsDesigner\TypeEditorHostControl.vb	551	Active
Message	IDE0046	'If' statement can be simplified	Microsoft.VisualStudio.Editors	E:\project-system2\src\Microsoft.VisualStudio.Editors\SettingsDesigner\TypeEditorHostControl.vb	618	Active
Message	IDE0046	'If' statement can be simplified	Microsoft.VisualStudio.Editors	E:\project-system2\src\Microsoft.VisualStudio.Editors\SettingsDesigner\TypeEditorHostControl.vb	787	Active
Message	IDE0046	'if' statement can be simplified	Microsoft.VisualStudio.ProjectSystem.Managed.TestServices	E:\project-system2\src\Microsoft.VisualStudio.ProjectSystem.Managed.TestServices\ProjectSystem\ProjectTreeParser\Tokenizer.cs	30	Active
Message	IDE0046	'if' statement can be simplified	Microsoft.VisualStudio.ProjectSystem.Managed.TestServices	E:\project-system2\src\Microsoft.VisualStudio.ProjectSystem.Managed.TestServices\ProjectSystem\ProjectTreeParser\Tokenizer.cs	123	Active
Message	IDE0046	'if' statement can be simplified	Microsoft.VisualStudio.ProjectSystem.Managed.TestServices	E:\project-system2\src\Microsoft.VisualStudio.ProjectSystem.Managed.TestServices\ProjectSystem\ProjectTreeParser\Tokenizer.cs	145	Active

I looked at the changes it wanted to make here’s one:

    If items.Length = 0 Then
        Return NativeMethods.S_OK
    End If
    Return VsUIShell2Service.SaveItemsViaDlg(CUInt(items.Length), items)

It wanted to change to:

Return If(items.Length = 0, NativeMethods.S_OK, VsUIShell2Service.SaveItemsViaDlg(CUInt(items.Length), items))

I do not believe that it makes the code better or clearer, while I understand that some people like this form, I do not. That’s what makes it a style choice. These style based should not appear in my Error List unless I’ve either got a setting that indicates what behavior I’d like (think var settings) or I’ll deliberated opt’d in them. Show them as a … in the editor or whatever, but my Error List are things that I need to address - I should haven’t keep turning off rules every time I upgrade my VS.

About this issue

  • Original URL
  • State: closed
  • Created 6 years ago
  • Reactions: 15
  • Comments: 22 (21 by maintainers)

Commits related to this issue

Most upvoted comments

Just found this - I also strongly dislike this refactor suggestion. It wants to turn multi-line if statements into ternaries. That’s not “simplifying” the code. It makes it harder to read and follow, and thus more error prone. IDEs should not do that. To illustrate the many-line case, if I have this:

public object TestMethod(bool condition)
{
    if (condition)
    {
        return new
        {
            Line1 = "true",
            Line2 = "true",
            Line3 = "true",
            Line4 = "true",
            Line5 = "true",
            Line6 = "true",
            Line7 = "true",
            Line8 = "true",
            Line9 = "true",
            Line10 = "true",
            Line11 = "true",
            Line12 = "true",
            Line13 = "true",
            Line14 = "true",
            Line15 = "true",
            Line16 = "true",
            Line17 = "true",
            Line18 = "true",
            Line19 = "true",
            Line20 = "true",
            Line21 = "true",
        };
    }
    else
    {
        return new
        {
            Line1 = "false",
            Line2 = "false",
            Line3 = "false",
            Line4 = "false",
            Line5 = "false",
            Line6 = "false",
            Line7 = "false",
            Line8 = "false",
            Line9 = "false",
            Line10 = "false",
            Line11 = "false",
            Line12 = "false",
            Line13 = "false",
            Line14 = "false",
            Line15 = "false",
            Line16 = "false",
            Line17 = "false",
            Line18 = "false",
            Line19 = "false",
            Line20 = "false",
            Line21 = "false",
        };
    }
}

It suggests turning it into:

public object TestMethod(bool condition)
{
    return condition
        ? (new
        {
            Line1 = "true",
            Line2 = "true",
            Line3 = "true",
            Line4 = "true",
            Line5 = "true",
            Line6 = "true",
            Line7 = "true",
            Line8 = "true",
            Line9 = "true",
            Line10 = "true",
            Line11 = "true",
            Line12 = "true",
            Line13 = "true",
            Line14 = "true",
            Line15 = "true",
            Line16 = "true",
            Line17 = "true",
            Line18 = "true",
            Line19 = "true",
            Line20 = "true",
            Line21 = "true",
        })
: (new
{
    Line1 = "false",
    Line2 = "false",
    Line3 = "false",
    Line4 = "false",
    Line5 = "false",
    Line6 = "false",
    Line7 = "false",
    Line8 = "false",
    Line9 = "false",
    Line10 = "false",
    Line11 = "false",
    Line12 = "false",
    Line13 = "false",
    Line14 = "false",
    Line15 = "false",
    Line16 = "false",
    Line17 = "false",
    Line18 = "false",
    Line19 = "false",
    Line20 = "false",
    Line21 = "false",
});

That’s just plain silly. And not simpler. It shouldn’t suggest it with many-line if statements regardless of when it should display at all.

Regardless of how (no strong feelings on the implementation): please don’t show this to people by default. I am actively disabling it in rulesets across all of our repos because it’s only an annoyance.

disable the analyzer by default

I would actually move it to Hidden severity by default so it is still operational, but isn’t proactive like it is now.

This works for me. This was an open question in the original PR: https://github.com/dotnet/roslyn/pull/26236#discussion_r183218203

However, it was never answered. I think the presumption was that if this was an issue for people, that we could trivially change hte default later if need be (as we seem to want to now).

I find most refactorings from if/else to ternary expressions reduce readability. I’m against any default that would encourage users to switch to ternary, and individual teams can increase the severity if they choose.

@CyrusNajmabadi the error list has a filter for “Messages” which can remove items at that severity.