search

Pash-Project / Pash

An Open Source reimplementation of Windows PowerShell, for Mono.
https://groups.google.com/group/pash-project
BSD 3-Clause "New" or "Revised" License
517 stars 54 forks source link

Static analysis? #250

Open ygra opened 10 years ago

ygra commented 10 years ago

Coverity Scan is a free service by Coverity offering static analysis for open-source projects. Apparently we can integrate it with Travis as well.

I'm not sure if it's worth yet with large parts of the project still in flux and not working, but it might catch a few bugs we have that otherwise would wait around to be found later.

sburnicki commented 10 years ago

Sounds like we should give it a try and look how useful the results are for our work.

JayBazuzi commented 10 years ago

I have been thinking about all the analysis tools we might be able to use. FxCop, StyleCop, R# code analysis, and Coverity could all be useful.

I really like the idea of creating consistency in style and catching classes of bugs easily. Much better to catch those with tools than with vigilance.

I really don't like when these tools reject your commit for something trivial, which you must then manually fix. It feels like busywork.

Even worse is when there is a delay between when you write the issue in your editor and when you get the feedback from the tool. That is worst with CI, but anything slower than intellisense is disappointing.

Also, I tend to disagree with some of the defaults in these tools. (I'm very opinionated!) But this isn't a big concern, because I like consistency even more, and we can disable rules we don't like.

Let's explore further.

ygra commented 10 years ago

I registered at Coverity Scan, added Pash and uploaded a first build. There are surprisingly few real errors (but the last time I used static analysis it was in a large, legacy C++ codebase, so there probably are some differences to a small, relatively new C# codebase).

It seems like you first have to register with Coverity (the easiest is via Github) and then request access to see the results. I can then approve them. If someone of the more seasoned members or actual project maintainers would rather maintain the Coverity project, then that's okay too. I just thought I'd try it out.

As for the issues: Coverity reported 18, two of which are false positives. We have a few places where we dereference null in some cases. We have a few resource leaks. Apart from that it looks good. And nothing we need to fix right away, actually. You're far more likely to hit a ParseException or a NotImplementedException than die of resource leaks at this point. But most of the reported issues can be fixed easily.

JayBazuzi commented 9 years ago

+1

If you want to push this in to our Travis CI build, go for it.

StilgarISCA commented 9 years ago

Completely missed this thread. I just opened a #346 to turn on the FXCop code analysis for the Visual Studio side of things. See the Google Group thread. When you enable all of the rules in VS you get way more than 18 violations. A lot of them are related to globalization issues, but there are quite a few concerning ones: methods that should be static, classes with incorrect exposure, that sort of thing.