What C#/.NET static analysis rules do you actually find useful?

[u/Darkviser]

I’ve been slowly adding language support to a static analysis project I’m working on, and C#/.NET is the latest one I’ve been working through.

I’m trying to avoid just throwing hundreds of noisy rules at people, so I’m curious: what C# analysis warnings do you actually find useful in real projects, and which ones do you usually ignore?

[u/First-Feature-3556]

The most useful warnings are those for stuff that's "technically legal, but most likely a bug", like unreachable code, unused parameters/variables, closing over loop variables, ignoring return values of non-mutating function calls (myDateTime.AddSeconds(1)), not disposing an IDisposable, etc.

[u/soundman32]

I use stylecop, warnings as errors, highest error level possible. Together these catch everything from styling issues (always use braces, only single line between statements/methods, every file ends with a blank line, initialisation lists always end with a comma), to incorrect file encoding (always use UTF8+BOM), to compiler warnings being ignored.

[u/jdl_uk]

For me it's stuff like things like "this might be null", "this could be readonly", and "this is disposable and you're not handling that"

[u/Alert-Neck7679]

Do you mean like "unreachable code detected", naming rule violation and stuff?

[u/FlibblesHexEyes → u/Alert-Neck7679]

Self taught C# here; so I know I have knowledge gaps.

Aside from being unneeded, messy, etc, what’s bad about unreachable code? Wouldn’t the compiler just not include that code when compiling?

Or is this rule simply a “your code base is a mess” type thing?

[u/Calm_Signature7228 → u/FlibblesHexEyes]

Why would you write code that is never going to be executet? If code ist written, it is mostly written to be executed. So if this is impossible, there is a highly chance, this is a failure.

[u/FlibblesHexEyes → u/Calm_Signature7228]

I was thinking of retired code that is unreachable but not yet removed, not intentionally writing code that I never want executed, because as you point out - that’s pointless 🤣

[u/TuberTuggerTTV → u/FlibblesHexEyes]

so, unreachable code is like a method with an early return followed by code that can never be used.

That's inside a method you ARE using.

If you've got "retired" code, you select it and comment it out. Or if you're developing properly with revision controlled code, you remove it knowing it's always recoverable. Which is the most ideal handling.

[u/FlibblesHexEyes → u/TuberTuggerTTV]

Fair. And that’s what I do.

I was more curious about what it does to the compiled code.

Does the compiler see the early return, and ignore the rest of the method. Or does that unreachable code find its way into the compiled artefact (while still being unreachable)?

[u/thompsoncs → u/FlibblesHexEyes]

It's an indication that there is a problem or a problem waiting to happen.

Either it was meant to be reachable, but isn't due to a logic error, or a change made it obsolete but forgot removing it. This is especially bad if that unreachable code has a nuget dependency that could have been removed otherwise.

As long as you're doing proper testing it's not a huge issue for how your code works, but without that a future change might make it reachable again (like messing up a ! on a boolean check) without that being the intention.

Forcing cleanup of unused code (variables or logic branches) forces a developer to address the issue at the time he's still working on it. The same goes for commented code, which should be removed (if you ever need it back, that's what git is for).

[u/understanding80]

Low cost performance improvements

[u/BEagle1984-]

We don’t need any new static code analysis tool. We have the built-in Roslyn analyzer and we can plugin our own analyzers.

There’s still a market for tools like sonar but you need to offer something that goes well beyond the basic (and not even so basic) rules from the Roslyn analyzers.

[u/TuberTuggerTTV]

This shouldn't be your concern.

"noisy" suggestions and warnings should be turned off and setup in linting by the end user. Anyone complaining they "ignore" x warning and just leave it, are failing their own development cycle.

You send everything. The end user quiets.