Our website uses cookies to enhance your browsing experience.
Developing a static analyzer, as with any software project, involves trade-offs. Sometimes this means dropping perfectly valid analyzer warnings to make the tool better overall. In the article, we'll look at a real-life example of that kind of compromise.
For the context, we're actively developing a static code analyzer for JavaScript and TypeScript, which is currently in beta (and we'd love for you to join it).
But back to the point: I've been working on the V7001 diagnostic rule, which detects typos in the form of identical operands in binary expressions:
let baz = foo – foo // foo - bar
if (foo == foo) { // foo == bar
// ....
}It also focuses on more complex cases like nested expressions:
(a && b) || (c && d) || (a && b)And expressions that are not identical but equivalent:
(a * b) > (b * a)If you're curious about how our JavaScript diagnostic rules perform in practice, we've covered some findings from open-source projects in the separate article.
Right after I finished writing this diagnostic rule, I ran analysis on our project database and found this code snippet in pdf.js:
flagValues: Object.freeze({
applyOverPrint: 1,
applySoftProofSettings: 1 << 1,
applyWorkingColorSpaces: 1 << 2,
emitHalftones: 1 << 3,
emitPostScriptXObjects: 1 << 4,
emitFormsAsPSForms: 1 << 5,
maxJP2KRes: 1 << 6,
setPageSize: 1 << 7,
suppressBG: 1 << 8,
suppressCenter: 1 << 9,
suppressCJKFontSubst: 1 << 10,
suppressCropClip: 1 << 1,
suppressRotate: 1 << 12,
suppressTransfer: 1 << 13,
suppressUCR: 1 << 14,
useTrapAnnots: 1 << 15,
usePrintersMarks: 1 << 16,
}),The code defines a set of conversion flags used to control printing. Each flag represents a single option, and the flags can be freely combined using bitwise operations. The flags are grouped by categories: color correction, transformation suppression, fonts, and so on. However, there's something off in this code. See that?
The PVS-Studio warning: V7001 The operands of the '<<' operator in the '1 << 1' expression are equivalent. print_params.js 90
The warning points to the line between the 10th and 12th flags:
suppressCJKFontSubst: 1 << 10,
suppressCropClip: 1 << 1,
suppressRotate: 1 << 12,Perhaps, this code is a child of Ctrl+C and Ctrl+V, with the field name and flag number changed manually in each line. But in one place, 1 << 1 was never replaced with 1 << 11. One digit missing among a sea of ones, no wonder it could go unnoticed.
This code has just been added and isn't called anywhere yet, so it doesn't cause any issues. And yet, when the time comes, applySoftProofSettings and suppressCropClip will behave as a single flag. All in all, this is a good catch for the analyzer, it found a real error here.
However, if you run PVS-Studio for JavaScript or TypeScript today, you won't see this warning. Why? While we were developing this diagnostic rule, the analyzer output a huge number of similar warnings—but the vast majority of them looked like this:
export enum InputFlags {
None = 0,
SignalBased = 1 << 0,
HasDecoratorInputTransform = 1 << 1,
}The PVS-Studio warning: V7001 The operands of the '<<' operator in the '1 << 1' expression are equivalent. core.ts 49
const enum PRINTER_FLAGS {
EMPTY = 0,
PRESERVE_FORMAT = 1 << 0,
COMPACT = 1 << 1,
CONCISE = 1 << 2,
RETAIN_LINES = 1 << 3,
RETAIN_FUNCTION_PARENS = 1 << 4,
AUX_COMMENTS = 1 << 5,
}The PVS-Studio warning: V7001 The operands of the '<<' operator in the '1 << 1' expression are equivalent. index.ts 10
Here's pdf.js again.
const ON_CURVE_POINT = 1 << 0;
const X_SHORT_VECTOR = 1 << 1;
const Y_SHORT_VECTOR = 1 << 2;
const REPEAT_FLAG = 1 << 3;
const X_IS_SAME_OR_POSITIVE_X_SHORT_VECTOR = 1 << 4;
const Y_IS_SAME_OR_POSITIVE_Y_SHORT_VECTOR = 1 << 5;
const OVERLAP_SIMPLE = 1 << 6;The PVS-Studio warning: V7001 The operands of the '<<' operator in the '1 << 1' expression are equivalent. glyf.js 17
These are just 3 out of 42 similar warnings among the total number of the V7001 warnings (126) in the test database. As you can see, the analyzer flags 1 << 1 everywhere it appears Technically, it's not wrong: shifting a number by 1 rarely makes sense outside of very niche scenarios. The result of the 1 << 1 operation will always be 2. And the only thing the notation does is make the interpreter fold the constant during compilation into machine code.
But here we're dealing with bit flags, where readability and consistency matter more than abstract performance gains. That's the reason why developers usually arrange bit flags into a clean, sequential list with the flag numbers on each line—after all, the overhead from the extra operation is truly negligible.
Interestingly, the analyzer will also fire a false positive on 1 << 1 in the code snippet provided earlier—yet another strike against such a pattern.
applyOverPrint: 1,
applySoftProofSettings: 1 << 1,
applyWorkingColorSpaces: 1 << 2,If we add the case of 1 << 1 to the exceptions, exactly one third of all warnings in our test database disappear, all of which are guaranteed to be false, except for the single real case shown at the beginning. In other words, implementing an exception for 1 << 1 improves this rule by a third.
Our development team follows a deliberate approach to warning reliability: we'd rather limit the number of warnings than risk flooding users with noise. It's called an unsound strategy. Here's a quote from our article on taint analysis:
There are sound and unsound static analysis strategies. Usually, the analyzer operates with an unsound strategy and issues warnings only if it can prove an error exists. Under this strategy, the example above won't trigger a warning about the array index out of bounds.
The sound strategy follows the opposite principle: the analyzer issues a warning if it can't prove the absence of an error. The main problem with this strategy is the high number of false positives.
To keep the analyzer from generating hundreds or thousands of false positives, we have to delve into existing approaches and make heuristic exceptions for them. That's how the exception for 1 << 1 made its way into V7001, even though it meant sacrificing a valid warning.
We could try to design something brighter. For example, checking whether there are any other bit flags nearby. But what does nearby mean for the static code analyzer? The analyzer works with a syntax tree, and we could try searching for neighbors within the same enumeration:
But, first, it's easy to trick such heuristics if we:
In each of these cases, the analyzer will either fail to detect the actual error or start generating false positives again, returning to the very problem we were trying to solve.
Second, note that the above warnings appeared in three different contexts:
enum;And that's not counting exotic scenarios like bit flags stored in class fields. There are many such cases, especially in JavaScript/TypeScript, and they're very easy to overlook.
Third, even if we manage to cover all of them at the cost of significantly bloating the code for the rule, there is no guarantee that the heuristics won't deprecate with any future language update. After all, new syntax constructs can simply break it.
So yes, sometimes we have to make a tough call and sacrifice a good warning in favor of a better analyzer's reliability. Plus, this case gave us the idea to create a separate diagnostic rule for identifying errors in bit flag definition :)
I hope this gave you a peek into the day-to-day work of developing static analyzers and maybe shed some light on how such tools evolve, and on the compromises that come with improving them. If you've ever faced similar dilemmas in software development, feel free to share them in the comments—I'd love to hear your stories :)
To stay updated on new articles about code quality, you can subscribe to our monthly newsletter or follow my personal blog.
0
0
0
0