Lessons from Building Static Analysis Tools at Google
cacm.acm.org
cacm.acm.org
This means that you can static analysis checks besides your library. For example, you have unit testing library that expects to be used in a certain way that cannot be completely described using type system -- it expects that test methods marked by some test attribute has to be public. Such library can include analyzer that would check usages of types from the library and report warnings. User of the library don't have to do a thing to register such analyzer, it will just work.
1. The false positives sometimes lead to ugly workarounds needed to quiet the message.
2. There's no standard, meaning different SA tools produce very different results, often contradictory.
A better approach is to improve the language. For example, the `(a < b < c)` is tripped by many static analyzers. In D, the grammar was changed so `a < b < c` is not valid. And the C/C++ committees should really deprecate `l` as a valid integer literal suffix :-)
I usually write a python program that doesn't complain about style violations, it just fixes them, like autopep8, but with much more stuff, like correctly capitalizing function names, variables, making strings consistent, making or taking away long-quotes, putting conditions in ifs, even renames shadowed variables (built it in last week after what feels like the 100th time I hit a bug due to shadowing) ... Makes people wonder how I can possibly program so fast under such onerous rules. On the downside: I work here now for 6+ years, and still don't know (or care) about most of the style guide, and frequently get confused when people ask questions "because you have written the most code, you must know, right ? Capitalize functions or not ? ... Ehm, well ...". Also, people ignore these things, so I must run it on any file I edit and submit a "style fixes" cl before doing anything else. But since git became an option, not much of a problem.
That's the trouble with style guides: 95% of what they do COULD have been implemented in editors if we demanded that style guide authors weren't "architects" and policy-makers, but programmers that really know what they're doing. Very little they do requires a policy, it can just be described as a parse-tree-transform. Instead, they're just people who feel they can just push their opinion on a large group of people, just because it makes them feel a bit better. That they can't actually program a fix, but instead just create another problem, doesn't seem to bother any of them, ever.
And we actually reward people for this. I don't get that. These people CREATE a problem, then somehow negotiate this style with a department, slowing everyone down for no good reason other than that they don't know how to do it automatically, and they usually actually get rewarded for providing a bad solution. Like they "introduce standards". Even just providing a style-guide-checker with a consistent interface (so IDE integration would be easy) is too much to ask at every place I've ever worked.
No, they substitute 10% of the all labor in this company for the job a simple perl script COULD do.
OpenSSL was trying to use uninitialized memory to seed the PRNG in one of the calls. The fix silenced the warnings by removing (almost) all seeding of the PRNG, instead of just the uninitialized memory seeding.
To his credit, the patch author did attempt to ask the OpenSSL developers if the patch was doing things correctly. However, the developers apparently didn't watch their own mailing list, and this didn't come out until after the problems were made public and the developers were laughing about what a n00b the Debian contributor was.
And it is wishful thinking to think you will not have regressions in either correctness or performance. Often both.
Edit: to be explicit, I'm not saying it is worse, either. Just has trade offs.
I have never seen contradictory parts of 2, so I don't know what you are talking about. If it is what I think: I would reject one of the two tools just so that I had a single easy to apply set of rules that have no false positives.
Static analysis is very valuable, but only when you can treat it like the hand of some god that will throw lightening at any developer who writes a found error. I'm currently looking at 1 suppression per 2 million lines of code. I think there is more value in more checks, but unless they can be fully automated without telling everybody how to suppress false positives I won't consider them.
You can tune an analysis to be considerably more conservative and reduce false positives. The analysis becomes more like a semantically aware formatter. Doing most things related to pointers will lead to FPs, but you can still get value even if you are terrified of FPs.
So no linters either?
I know that there are lots of checks that would be useful that we are missing. However they are not worth looking at because every false positive destroys some trust, and soon they are not trusted even when they are right.
It sounds to me like the issue isn't that false positives exist (you aren't ever going to fully get rid of them), it's that the SA tools need easy "escape hatches" for those false positives.
Even something as simple as an editor plugin to let you click on a specific "error" or "warning" and click "silence" to add a magic comment or something to explicitly "allow" the line/function/file.
It reduces the amount of extra work from a false positive to next to nothing, it still can and will catch issues before they hit production, and it can make iffy-code more explicit (The added magic comment essentially saying "yes, i did mean to do that").
2. Those magic comments are put in the source code, meaning they uglify the code in github, etc.
Of course, one could create a separate database for the annotations, but that adds another layer of complexity for the users to prevent its use.
They are as portable as the code is, yes they might only work with a single language requiring another "format" for other linters in other languages, but they will stay with the code wherever it goes.
And they aren't "uglifying" the code any more than type annotations or variable names. They are an additional hint to the user and the system about what a specific line or block of code is doing. They are just an annotation that the compiler ignores (for the most part).
That said, the analyses (a) are specific to our team (b) require about 2 hours (each) to compute on a 6-core box (c) are only run every couple of months (d) are run and applied by the same person. (e) have a false positive rate of around 1%, so require manual review (f) often make assumptions that are only valid for how we use C++ and various libraries (without some "well just don't do that" assumptions you can't do any useful analysis across a C++ codebase :-)
However, our project is relatively small, so we are normally done in ~15 minutes. We run it right before upstreaming patches, which means that the tests run once or twice a day.
Which is to say there are some checks that are always right and google doesn't allow violations. The 10% seems to refer to checks that catch some significant bugs - things bad enough that they are worth looking at, but the check isn't perfect.
I am a bit unclear about this statement. Does it really mean that Google uses a single, monolithic program to do everything from advertisement payment tracking to the GMail user interface? Or does it mean they don't have the infrastructure to do analysis of many individual programs? (Surely, that can't be true!?)
"Attempt 2. Filing bugs. The BugBot team then began to manually triage new issues found by each nightly FindBugs run, filing bug reports for the most important ones. In May 2009, hundreds of Google engineers participated in a companywide "Fixit" week, focusing on addressing FindBugs warnings.3 They reviewed a total of 3,954 such warnings (42% of 9,473 total), but only 16% (640) were actually fixed, despite the fact that 44% of reviewed issues (1,746) resulted in a bug report being filed. Although the Fixit validated that many issues found by FindBugs were actual bugs, a significant fraction were not important enough to fix in practice. Manually triaging issues and filing bug reports is not sustainable at a large scale.
"Attempt 3. Code review integration. The BugBot team then implemented a system in which FindBugs automatically ran when a proposed change was sent for review, posting results as comments on the code-review thread, something the code-review team was already doing for style/formatting issues. Google developers could suppress false positives and apply FindBugs' confidence in the result to filter comments. The tooling further attempted to show only new FindBugs warnings but sometimes miscategorized issues as new. Such integration was discontinued when the code-review tool was replaced in 2011 for two main reasons: the presence of effective false positives caused developers to lose confidence in the tool, and developer customization resulted in an inconsistent view of analysis results."
Once upon a time, I had a co-worker on a C++ project commit a change disabling -Wall with a comment that can loosely be paraphrased as, "No one has time for that."
It's good to see Google has the same issues.
How would someone integrate static analysis to the workflow if there's no code review?
My experience with bug dashboards is the same of Google. It is outside the developers workflow.
In addition to tslint (the "old way") we now have http://tsetse.info which follows the Error Prone model of baking checks into the TypeScript compiler and failing the compilation the same way as the type checker (for the error case).
Still have some work to do to make warnings appear in code review like we talk about in the paper...
Somewhat interestingly, Clippy is an analogous tool for Rust and has 181 contributors now, more than Error Prone. https://github.com/rust-lang-nursery/rust-clippy