Checking the C# Source Code of MSBuild with PVS-Studio | Hacker News Reader

Checking the C# Source Code of MSBuild with PVS-Studio

medium.com

66 pointsPVS-Studio15 comments

https://github.com/Microsoft/msbuild/issues/940 feels more like an advertisement than a genuine issue report. Wouldn't it be better to make two or three issues from the "worst" errors found, rather than pointing at a third party blog?

The problem is that there's a sliding tradeoff of effort they put into filing those bugs and how likely it is to {benefit them, get them fixed}.

Without any communication with the developers of MSBuild, every additional bug they filed might turn out to be a timesink for a good writeup only to get a "WONTFIX" equivalent, while having only submitted a bug report with a link to their breakdown (which they needed to write anyway for this to benefit them much) is the minimum required to be able to say "we warned them" if one of these bugs turns out to break something critically later.

So yes, they probably should have written a couple of the unambiguously wrong code bugs up and submitted them with a citation of their full writeup, but their goal, cynically, is to drive usage of their product, not necessarily to get the bugs fixed.

(e: plus, at that point, you could reasonably argue they should submit PRs with the trivial code fixes, and then you get into discussions of possibly having to sign a CLA and all sorts of mess that they probably don't want.)

Microsoft runs internally similar tools. The usual deal is that grandfathered issues (ie. issues found by new code analysis tools on old code base) are generally not fixed, and the warning is instead silenced (all tools have means to suppress specific warning for specific code place). This is done because the risk to fix a code analysis issue on old code base is often higher than the benefit.

New code on the other hand is usually required to pass all static analysis without introducing new warnings, and is often enforced by automated code review (bots that runs static analysis will mark the code review and it will not be possible to check it in).

From time to time some teams form to work on fixing static analysis code warnings on old code, but is not a high priority. Of course a public PR event like the OP may trigger some action, is going to be up to the individual team priorities, policy, schedule, workload etc.

This may sound like laziness or carelessness, but these are good engineers with plenty of experience that are capable of judging risk vs. benefits. If the code was like this for years and no actual bug was reported, is not always worth modifying it. Eric Lippert has a nice blog about How many Microsoft employees does it take to change a lightbulb? [0] and why some of these 'trivial' changes for a small project add up to a risky change at MS scale.

- [0] https://blogs.msdn.microsoft.com/ericlippert/2003/10/28/how-...

- StyleCop https://en.wikipedia.org/wiki/StyleCop

- FxCop https://en.wikipedia.org/wiki/FxCop

- Prefast https://msdn.microsoft.com/en-us/library/d3bbz7tz.aspx

- Code Analysis Check-In policies https://msdn.microsoft.com/en-us/library/ms182075.aspx

There are more tools like these and at the time I left there were still more popping up. I hated StyleCop, the Tabs vs. Spaces Sillicon Valley episode is very relevant for my relation with StyleCop...

It feels like a few of those could actually be prevented with different language features/by the compiler:

- `null` related errors: through algebraic data types like `Maybe` in Haskell or `Option` in Rust

- Unused arguments in string formatting methods: I don't get why the compiler doesn't see this as an error

I'd go further: that probably applies to all their checks. If you can formalize it enough to put it in an automated checker, why not put it in the language type system? Often these automated checkers end up having to keep track of a bunch of assertions about the input/output of the methods, sometimes even specified with annotations or magic comments - basically a second shadow type system. Just use in the types!

The appeal of those tools is that they allow you to retrofit more sophisticated safety features into existing lanugages, where a giant heap of existing code prohibits breaking changes.

I find it more effective to make the porting explicit - make a new language that has a really good FFI into the old language, and gradually migrate to that. (I tried to use the checkers framework for a few years, then instead started using Scala, which was much more effective)

Porting the C# compiler to Scala doesn't seem like it would constitute a real vote of confidence in the technology.

Porting it to F# would probably be more aligned to their business interests, and have much the same effect. Actually MS has been pretty good at cherry-picking parts of F# and including them in newer releases of C#, so simply "porting" it to "modern C#" would provide at least some of the benefits (though there's only so much that can be achieved without a willingness to outright deprecate features that now have a better replacement).

(You still need a linter to check for use of deprecated features, but that can be a very simple thing; what I'm saying doesn't make sense is sophisticated (and expensive) static analysis tools)

Static analyzers often also find issues that look suspicious and indicate that there's an error but don't have to really be an error. For instance here the check for variables that are always true. Another issues is that some analyses of static analyzers are too costly to do by the compiler.

When i see the tool in Github as a developer, i would just think.

Have you found a bug? No, so what are you submitting? Possible bugs.

Okay, this is closed. This isn't a bug, wontfix isn't even required.

Do you disable the compiler warnings just in case it reports something that isn't a bug? Probably not. The same applies here. Just because it's a (not very thinly veiled) advert, doesn't mean you should automatically dismiss it.

The miliseconds bit doesn't look like a bug to me. The truncation is on purpose.

Why would you specifically cast it to a float if you wanted an int?

Reply on news.ycombinator.com