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
- Reduce the default severity of preferring conditional expressions Closes #27195 — committed to sharwell/roslyn by sharwell 6 years ago
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:It suggests turning it into:
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.
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.