
Checking the C# Source Code of MSBuild with PVS-Studio - PVS-Studio
https://medium.com/@sofiafateeva93/checking-the-source-code-of-msbuild-with-pvs-studio-36d71762d705
======
voltagex_
[https://github.com/Microsoft/msbuild/issues/940](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?

~~~
rusanu
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-...](https://blogs.msdn.microsoft.com/ericlippert/2003/10/28/how-
many-microsoft-employees-does-it-take-to-change-a-lightbulb/)

~~~
rusanu
\- StyleCop
[https://en.wikipedia.org/wiki/StyleCop](https://en.wikipedia.org/wiki/StyleCop)

\- FxCop
[https://en.wikipedia.org/wiki/FxCop](https://en.wikipedia.org/wiki/FxCop)

\- Prefast [https://msdn.microsoft.com/en-
us/library/d3bbz7tz.aspx](https://msdn.microsoft.com/en-
us/library/d3bbz7tz.aspx)

\- Code Analysis Check-In policies [https://msdn.microsoft.com/en-
us/library/ms182075.aspx](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...

------
legulere
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

~~~
lmm
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!

~~~
thomasz
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.

~~~
lmm
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)

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

~~~
lmm
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)

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

~~~
to3m
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.

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

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

