How do you fix an obvious bug?

Consider the following code (from Eclipse 3.2, org.eclipse.jdk.internal.core.NamedMember):

for (int i = 0; i < length; i++) {
  String typeArgument = typeArguments[i];
  typeArgument.replace('/', '.');
  buffer.append(Signature.toString(typeArgument));
  if (i < length-1)
    buffer.append(',');
  }

FindBugs points out that the return value of replace('/', '.') is being ignored and thus the call has no effect. It is very tempting to just "fix" the code to get

for (int i = 0; i < length; i++) {
  String typeArgument = typeArguments[i];
  typeArgument = typeArgument.replace('/', '.');
  buffer.append(Signature.toString(typeArgument));
  if (i < length-1)
    buffer.append(',');
  }

But does this actually fix the code? We probably don't have any test cases where the type argument contains a '/', or else we would have already have fixed the code. Or perhaps the buffer should contain slashes rather than dots, and the fact that the return value from replace is ignored is what makes the code correct. Or perhaps '.' would be the correct answer, but other code in the system depends upon the error in this code.

Static analysis tools, such as FindBugs, don't actually know what your code is supposed to do. Instead, they typically just find inconsistencies in your code. Invoking a String replace method and ignoring the return value is weird or inconsistent, but we don't actually know that using the return value is more correct.

So no easy answers. On recommendation is that when you see an obvious bug, maybe you shouldn't just bang out a quick fix. Instead, figure out why tests aren't showing a problem with this code, try to document the intended behavior and write a test case showing the existing code gives incorrect behavior.

Then, go make the obvious fix.


Bill Pugh