Wednesday, May 11, 2016

Extract Method introduces a bug in this corner case

I rely on automated Extract Method to do the right thing, regardless of test coverage. This is a key part of attacking legacy code that lacks tests. But if the Extract Method introduces a subtle bug, then I can't rely on it.

Here's the code:

As it is, the test passes. If you extract the indicated block, then the test fails. Extract Method should add a `ref`  to the parameter on the new method.

This repros with VS2013, VS2015, and ReSharper 8, 9, and 10.


Unknown said...

Wow... that's subtle.

I'm assuming that the failure is that value is 0 when the `Assert` on line 14 executes. Is that correct?

I'm also guessing that this is the simplest way to reproduce this issue. Can you talk more about the scenario that lead you to discover this edge case?

I wouldn't have expected that to result in a failure. What do you think the solution is? One thing I can think of is to have the refactoring engine examine whether or not the variable has been passed by reference in the current scope, which would have to include the variable getting used by a closure.

Jay Bazuzi said...

I made it even simpler by removing the Increment() method.

Jay Bazuzi said...

Yep, you assume correctly.

A coworker discovered this some time ago, in real code, and then we whittled it down to this simple example.

My understanding is that VS2005-VS2013 couldn't detect this case, but that VS2015 (Roslyn) does detect it, and that in prerelease builds it inserted the 'ref', but they got user feedback that it noisy, so they deliberately turned it off.