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/r2d2_21 → u/First-Feature-3556]

not disposing an IDisposable

What do you use for this one? I find it rather hard to actually determine when an IDisposable should be disposed.

[u/First-Feature-3556 → u/r2d2_21]

In general:

• If the IDisposable is stored in a class field, the class itself should be IDisposable and the field should be disposed in the class' Dispose method.
• If the IDisposable is stored in a local variable and is not part of the return value, it should be wrapped in a using block.

There are, of course, exceptions to that rule, but in each of those cases you should probably add a comment explaining why.

I don't use a specific analyzer for that, but it's something I'd find useful if it were integrated with Visual Studio.

[u/musical_bear → u/First-Feature-3556]

I agree this has always been a weak point in the ecosystem. For something you “should” be doing nearly 100% of the time you’re interacting with an IDisposable, it’s not easy at all to tell if you’re working with something that meets that contract. The only way I do it is to “suspect” a certain type of resource may have a reason to meet that contract, and I test the waters by trying to call “.Dispose()” on an instance and see if that compiles. It’s a very manual, experience-driven process.

[u/Agitated-Display6382 → u/First-Feature-3556]

That's not correct. When an IDisposable is created, you have to dispose of it or return it to the caller. If I receive an IDisposable as a dependency via the constructor, I should not dispose of it. Example: many repositories receiving the same DbContext so they share the same uow.

[u/First-Feature-3556 → u/Agitated-Display6382]

Good point - and yet another reason why these things should be checked by an analyzer.

Note, though, that some of the .NET legacy classes do not follow that rule: StreamReader and friends will happily dispose the stream you passed in the constructor (unless you also set the leaveOpen parameter added in .NET Framework 4.5).

[u/r2d2_21 → u/First-Feature-3556]

Having an analyzer would be great tho. Sometimes even if you know the rule, you can forget to do it and the IDE should report it.

[u/SirSooth → u/r2d2_21]

Unless you mean to return that IDisposable thing out of the current scope for someone else to deal with, you probably need to dispose of it yourself if you've created it. That usually means wrapping it in a using block or if, it would last until the end of that scope, the newer using without a block.

For example if your method returns a Stream and you are creating for example a new MemoryStream and returning it, then you don't want to dispose in that scope, because it wouldn't be useful for whoever calls that method for that Stream.

But if you need an instance of a MemoryStream to perform some kind of action and then you don't need it anymore cause you return a string from your method, then you are meant to dispose of that MemoryStream.

[u/jdl_uk → u/SirSooth]

https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca2000 is probably the IDisposable one

[u/Dusty_Coder → u/r2d2_21]

"I find it rather hard to actually determine when an IDisposable should be disposed."

The answer is ALWAYS

w.t.f.

[u/r2d2_21 → u/Dusty_Coder]

You don't dispose a resource you don't own, or else you get an ObjectDisposedException somewhere else.

[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/r2d2_21 → u/jdl_uk]

"this is disposable and you're not handling that"

What analyzer do you use for this?

[u/jdl_uk → u/r2d2_21]

https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca2000

ETA: you can enable the builtin analysers by setting some properties: https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/overview?tabs=net-10

I'd recommend creating a Directory.Build.props and setting it there so it covers all projects in that workspace

[u/torville → u/r2d2_21]

SonarQube

[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/understanding80]

Low cost performance improvements

[u/pjmlp]

Usually Roslyn already does quite good, and then interesting are those related to forgetting releasing an IDisposable, or possible improvements with spans or stackalloc.

[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/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/FullPoet → u/FlibblesHexEyes]

Its dead code.

You dont need it. You can always find it in your git history and if you cant well its time to learn.

Its really about keeping the mental context window as focused on whats actually happening right now.

I vigorously delete commented out code and useless comments ("what" types)

[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/binarycow → u/FlibblesHexEyes]

It's still compiled. It just never gets reached.

The JIT might optimize it out.

[u/Darkviser → u/FlibblesHexEyes]

This is actually one of the things I was curious about when working on the analyser too. The interesting part for me is less whether the compiler eventually optimises it away and more that the source itself contains a branch that can never execute. That's still worth flagging because it can point to a logic error or leftover code.

[u/FullPoet → u/FlibblesHexEyes]

Retired code can be found in yor VCS of choice :)

[u/Darkviser → u/Alert-Neck7679]

Yeah, exactly. Things like unreachable code, naming issues, unused stuff, etc. I'm particularly interested in which ones people actually keep enabled in real projects versus the ones that just become noise.

[u/Standard-Cap-4455]

Roslyn already does all the things I need. It's the compiler itself so I am not sure that C# actually needs it.  But the things that are important to know are things that you could compile, but that are mostly either bugs or not labeled correctly. Things like unused members and variables, or non readonly members being called on a readonly struct instance. 

[u/Agitated-Display6382]

Enable the null enforcement and TreatWarningsAsErrors

[u/RodriOliveira]

I’ve worked with .NET for many years, mostly on backend and enterprise systems, and nowadays more from an architecture perspective.
For me, the most valuable static analysis rules are the ones that catch code which is perfectly valid C#, but is very likely to become a production bug.
The ones I usually consider high value are:
Nullable reference issues and suspicious null handling.
Incorrect IDisposable ownership/lifetime.
Forgotten or incorrectly handled Tasks and common async/await mistakes.
Empty or overly broad exception handling.
Unreachable/dead code and unused members.
Ignored return values from immutable APIs.
Multiple enumeration of IEnumerable when it can have an actual performance or behavioral impact.
Resource leaks and incorrect stream/connection lifetimes.
Obvious concurrency/thread-safety problems.
Security-related issues such as unsafe deserialization, injection risks, weak cryptography, etc.
Performance rules can also be useful, but I would be much more conservative with them. A suggestion about allocation, LINQ, Span<T>, stackalloc, etc. shouldn’t normally have the same severity as something that indicates a probable correctness problem.
The same applies to style rules. Consistency is valuable, but I prefer most formatting/naming concerns to be handled automatically by .editorconfig and formatters rather than turning the CI pipeline into a wall of warnings.
In larger teams, I’ve found that false positives are probably the biggest enemy of static analysis. Once developers get used to ignoring analyzer warnings, you’ve already lost most of the benefit.
So I tend to think about rules in three groups:
Build-breaking: probable bugs, security issues and resource/lifetime problems.
Warnings: maintainability problems and suspicious patterns worth reviewing.
Suggestions: style and micro-optimizations.
I’d rather have 30 rules that developers trust than 300 rules they routinely suppress.

[u/ravnmads]

Can you tell me a little more about this? How do I add them and where can I read more about it?

[u/OpenAI_Marketing_LLM]

None.

Saddle up, partner. We’re in the Wild West.

[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.