I once caused a serious, halt-the-enterprise production bug by "fixing" a problem found by FindBugs. This was Java code, something along the lines of:
Boolean b = new Boolean(true);
The static analyzer correctly identified this as an unnecessary new object creation (style guides and good sense recommend you simply use Boolean.TRUE). I "fixed" it, and went on my way.
Little did I realize that this variable was actually a lock, and there was a synchronized(b) block later (and much deeper) in the code, which I effectively eliminated by removing the new.
In my defense I feel that the real bug here was that of documentation- had the variable been named something like "lock" I'd have understood immediately what was going on. But that doesn't make you feel much better when your team's been up all night fixing your bug!
Moral of the story: your codebase (especially if it's an older one) might actually be depending on its "bugs" for proper behavior. Think (and test) hard before applying suggested changes from static analysis.
The choice of a Boolean as a monitor object is a little odd. My understanding is that the usual convention is to create such objects via "new Object()", which is a little more obvious- the only reason you would ever call the base Object constructor is to produce something that can be used as a monitor.
It was a couple of years ago and I no longer have access to the codebase, but the business logic of that piece of code called for a boolean.
The problem was they they also (ab)used that field for the lock. It either should have been a separate field (of type Object, as you suggested) or use the existing Boolean, but call it "lock" or "monitor" or somesuch.
I agree. If you know you're writing something that is un-idiomatic or you think its intended purpose will be a surprise to most readers, put a comment in explaining why.
// we need a heap object so we can synchronize on it later
Boolean b = new Boolean(true);
This points to a deeper problem of static analysis, though - any analysis package without the ability to annotate code is _doomed_. The false positives will be so annoying that people will give up on it.
And for the people who work on SA systems - please give me a way to annotate that is not exclusively via comments. Especially once people use multiple SA packages, that is rather annoying :)
Sure, it wasn't great code, but this also sounds like a flaw in the static analyzer. It should have been able to tell that the variable was being synchronized on, and recognized that using Boolean.TRUE would have been an unsafe change.
Sure I could have found all of the references easily. That's one of the reasons I still prefer Java over a dynamic language- my IDE can tell me instantly (option key + F7) where a particular object is used in the entire codebase. I just wasn't careful enough, because hey, what could possibly go wrong- it's a stupid boolean, right?
Little did I realize that this variable was actually a lock, and there was a synchronized(b) block later (and much deeper) in the code, which I effectively eliminated by removing the new.
In my defense I feel that the real bug here was that of documentation- had the variable been named something like "lock" I'd have understood immediately what was going on. But that doesn't make you feel much better when your team's been up all night fixing your bug!
Moral of the story: your codebase (especially if it's an older one) might actually be depending on its "bugs" for proper behavior. Think (and test) hard before applying suggested changes from static analysis.