5 ms·

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

66 points by PVS-Studio 10y ago

voltagex_ 10y ago
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?
- rincebrain 10y ago
  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.)
- rusanu 10y ago
  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-many-microsoft-employees-does-it-take-to-change-a-lightbulb/ https://blogs.msdn.microsoft.com/ericlippert/2003/10/28/how-...
  - rusanu 10y ago
    - 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...
NicoJuicy 10y ago
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 10y ago
  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 10y ago
The miliseconds bit doesn't look like a bug to me. The truncation is on purpose.
- d2p 10y ago
  Why would you specifically cast it to a float if you wanted an int?
legulere 10y ago
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 10y ago
  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 10y ago
    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 10y ago
      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 10y ago
        Porting the C# compiler to Scala doesn't seem like it would constitute a real vote of confidence in the technology.
        lmm 10y ago
        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)

66 points by PVS-Studio 10y ago

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?

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

NicoJuicy 10y ago

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 10y ago
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 10y ago

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

d2p 10y ago
Why would you specifically cast it to a float if you wanted an int?

legulere 10y ago

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 10y ago
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 10y ago
  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 10y ago
    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 10y ago
      Porting the C# compiler to Scala doesn't seem like it would constitute a real vote of confidence in the technology.
      - lmm 10y ago
        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)