21

Given the following (heavily edited, pseudo-)code:

int count = 0;
thing.Stub(m => m.AddBlah()).WhenCalled(o => count++);
thing.Stub(m => m.RemoveBlah()).WhenCalled(o => count--);

DoStuff(thing);

Assert.AreEqual(1, count);

ReSharper provides a warning on count - "Access to modified closure". I understand why I'm getting this warning (the count variable is being modified in two different lambdas, and is likely to have undesirable semantics), but I don't understand ReSharper's advice: "Wrap local variable in array". If I let ReSharper do this, I get:

int count[] = { 0 };
thing.Stub(m => m.AddBlah()).WhenCalled(o => count[0]++);
thing.Stub(m => m.RemoveBlah()).WhenCalled(o => count[0]--);

DoStuff(thing);

Assert.AreEqual(1, count[0]);

And no warning.

Why is using an array safe?

citizenmatt
  • 18,085
  • 5
  • 55
  • 60

2 Answers2

13

I noticed this same thing in ReSharper myself, and was also left wondering why it doesn't warn when the value is wrapped in an array. The other answer here is unfortunately wrong, and seems to misunderstand how closures are implemented, so thought I would attempt explain (what I think is) the rationale behind this refactoring.

As you've seen, the result is the same whether array-wrapped or not, so the refactoring doesn't really "fix" anything and the same issues that can be encountered when accessing an ordinary modified closure exist after applying the change. However, after the change since the count array itself is not being modified (only its contents), the "Access to modified closure" warning is no longer relevant.

The change doesn't really make the problem any more obvious in (at least in my opinion), so it would seem that this suggestion is essentially telling ReSharper to ignore the issue, without having to resort to the rather messy // ReSharper disable AccessToModifiedClosure mechanism to suppress the error.

Iridium
  • 23,323
  • 6
  • 52
  • 74
  • It does seem plausible that this is their rationale because I can't think of any other reason for this code "fix". If it is indeed why they implemented this, it seems odd that they'd think wrapping all accesses to a variable in a one-element array would be less messy than simply disabling the warning with a comment only when it is used. – jam40jeff Dec 16 '17 at 18:36
1

This is because the 2 types are different. The int is a Value type and the array is a Reference type. This means that the int is on the stack and the array's pointer is on the stack.

When you update a Value type it updates that piece of stack memory. The Reference type on the other hand leaves that piece of stack memory alone and modifies what it points to.

Resharper doesn't complain about the array because the 2 different Lambda methods are creating a closure around the memory that points to where to update a value. Both Lambdas get the same address and don't change the original.

Jason
  • 3,736
  • 5
  • 33
  • 40
  • 1
    But surely the lambdas closing around an int variable are also both pointing to the same point in memory, so what's the difference? The code executes the same in both cases, so implementation wise, there's no difference. But I don't see why resharper treats them differently. – citizenmatt Jul 01 '11 at 07:36
  • @citizenmatt, think of the case when the method has returned, ReSharper does not know that the lambdas will only run while you are in the call to DoStuff(thing) – Ian Ringrose Jul 07 '11 at 12:33
  • This is not the correct reason, look at the other answer. There is no difference in capturing reference or value variables inside a lambda, which is why the behavior is the same either way. http://stackoverflow.com/questions/5438307/detailed-explanation-of-variable-capture-in-closures – Matt Klein Apr 27 '15 at 22:54