Suspicious fragments found by PVS-Studio Static Analyzer

[VasilievSerg]

Hello,
I checked .NET Core libraries using a static analyzer and found something interesting. I described the results of the check (fragments that seemed to me the most interesting) in the article: Checking the .NET Core Libraries Source Code by the PVS-Studio Static Analyzer. Hopefully, this article will help you make the code a bit better. :)

P.S. To consider the problem more thoroughly using the analyzer a key will be needed - this one can be used:

Name: .NET Core Libraries
Key: FM9G-50FJ-1K2G-SS07
License Type: Team
License Valid Thru: Sep 14, 2019

[ViktorHofer]

Thanks for running your analyzer over our code base and writing an article with explanations for many of the warnings. We will take a look at the warnings and errors over the next months.

[stephentoub]

Issues 1, 2, 3, 4, 6, 7, 10, 11, 15, 17, 21, 23, 24, 30, 31, 32, 33, 35, 38, 39, 42, 43, 48, 50, 51, 57, 58, 59, 60, 61, 62, 64
All of these are either false positives or fixed a long time ago. In any case, they’re not actionable now.

Issue 5
PR: #63369

Issue 8
False positive. sym will never be null (the null check as part of accessing the field is unnecessary, but we’re not churning this library for such things).

Issue 9
False positive. This is a stylistic choice.

Issue 12
PR: #63368

Issue 13
False positive. The code is purposefully invoking an instance method in order to throw a null ref.

Issue 14
By design, even if the design is questionable.

Issue 16
By design, this is all there for compat, and while it could be cleaned up, we’ve avoided churning this code.

Issue 18
PR: #63367

Issue 19
False positive. It’s virtual, an override can return false.

Issue 20
False positive. It’s virtual, an override can return false.

Issue 22
By design. The parameters are documented/annotated to only accept non-null, and nullable reference types will result in a compilation warning if null is passed. While we often validate and throw ArgumentNullException for such misuse, we don’t always and we don’t guarantee it.

Issue 25
PR: #63366

Issue 26
PR: #63364

Issue 27
PR: #63371

Issue 28
False positive. The public API was defined and shipped, and only later the parameter stopped being used. Removing the defunct parameters would be a breaking change.

Issue 29
By design, whether or not the design is questionable.

Issue 34
By design, whether or not the design is questionable.

Issue 36
False positive. The type name is used as the member name is null.

Issue 37
This does look incorrect, but it’s been like this forever, changing it now would likely be a large breaking change, and this library is on a path to being deprecated, anyway.

Issue 40
PR: #63363

Issue 41
This was previously discussed in dotnet/corefx#40342 and declined.

Issue 44
False positive. The implementation adds both constraints and attributes together; the code is written a bit wonky given that, of course.

Issue 45
PR: #63372

Issue 47
False positive. The logic outlined in the blog post is flawed. GetString will only return null if JsonTokenType is JsonTokenType.Null, but if JsonTokenType is Null, we won’t have called GetString and will instead have returned string.Empty.

Issue 49
By design. As a root document you’re guaranteed at BeforeRoot. The parameter is only there from the interface.

Issue 52
False positive. The logging code is defensive by nature; it ends up being stylistic to never assume non-null.

Issue 53, 54
This was discussed in an issue back in 2016 and the decision was to keep it as-is.

Issue 55
PR: #63362

Issue 56
PR : #63361

Issue 63
False positive. The value passed in for the second argument doesn’t matter because the first argument is infinite.