Monday, January 30, 2017

Rechecking SharpDevelop: Any New Bugs?

PVS-Studio analyzer is continuously improving, and the C#-code analysis module is developing most actively: ninety new diagnostic rules were added in 2016. However, the best way to estimate the analyzer's efficiency is to look at the bugs it can catch. It's always interesting, as well as useful, to do recurring checks of large open-source projects at certain intervals and compare their results. Today I will talk about the results of the second analysis of SharpDevelop project.
Picture 1

Introduction

The previous article about the analysis results for SharpDevelop was written by Andrey Karpov in November 2015. We were only going through the testing stage of our new C# analyzer then and were preparing for its first release. However, with just the beta version on hand, Andrey successfully checked SharpDeveloper and found a few interesting bugs there. After that, SharpDevelop was "laid on the shelf" to be used with a number of other projects solely within our team for testing new diagnostics. Now the time has come to check SharpDevelop once again but with the more "brawny" version, PVS-Studio 6.12.
I downloaded the latest version of SharpDevelop's source code from GitHub. The project contains about one million lines of code in C#. At the end of the analysis, PVS-Studio output 809 warnings: 74 first-level, 508 second-level, and 227 third-level messages:
Picture 2
I will skip the Low-level warnings because there is a high rate of false positives among those. About 40% of the Medium- and High-level warnings (582 in total) were found to be genuine errors or highly suspicious constructs, which corresponds to 233 warnings. In other words, PVS-Studio found an average of 0.23 errors per 1000 lines of code. This rate indicates a very high quality of SharpDevelop project's code. Many of the other projects show much worse results.
The new check revealed some of the bugs found and described by Andrey in his previous article, but most of the errors are new. The most interesting ones are discussed below.

Analysis results

Canonical Copy-Paste bug
This error deserves its own standard in the International Bureau of Weights and Measures. It is also a vivid example of how useful static analysis is and how dangerous Copy-Paste can be.
PVS-Studio diagnostic message: V3102 Suspicious access to element of 'method.SequencePoints' object by a constant index inside a loop. CodeCoverageMethodTreeNode.cs 52
public override void ActivateItem()
{
  if (method != null && method.SequencePoints.Count > 0) {
    CodeCoverageSequencePoint firstSequencePoint =  
      method.SequencePoints[0];
    ....
    for (int i = 1; i < method.SequencePoints.Count; ++i) {
      CodeCoverageSequencePoint sequencePoint = 
        method.SequencePoints[0];  // <=
      ....
    }
    ....
  }
  ....
}
The zero-index element of the collection is accessed at each iteration of the for loop. I included the code fragment immediately following the condition of the if statement on purpose to show where the line used in the loop body was copied from. The programmer changed the variable name firstSequencePoint to sequencePoint but forgot to change the expression indexing into the elements. This is what the fixed version of the construct looks like:
public override void ActivateItem()
{
  if (method != null && method.SequencePoints.Count > 0) {
    CodeCoverageSequencePoint firstSequencePoint =  
      method.SequencePoints[0];
    ....
    for (int i = 1; i < method.SequencePoints.Count; ++i) {
      CodeCoverageSequencePoint sequencePoint = 
        method.SequencePoints[i];
      ....
    }
    ....
  }
  ....
}
"Find the 10 differences", or another Copy-Paste
PVS-Studio diagnostic message: V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless NamespaceTreeNode.cs 87
public int Compare(SharpTreeNode x, SharpTreeNode y)
{
  ....
  if (typeNameComparison == 0) {
    if (x.Text.ToString().Length < y.Text.ToString().Length)  // <=
      return -1;
    if (x.Text.ToString().Length < y.Text.ToString().Length)  // <=
      return 1;
  }  
  ....
}
Both if blocks use the same condition. I can't say for sure what exactly the correct version of the code should look like in this case; it has to be decided by the program author.
Late null check
PVS-Studio diagnostic message: V3095 The 'position' object was used before it was verified against null. Check lines: 204, 206. Task.cs 204
public void JumpToPosition()
{
  if (hasLocation && !position.IsDeleted)  // <=
    ....
  else if (position != null)
    ....
}
The position variable is used without testing it for null. The check is done in another condition, in the else block. This is what the fixed version could look like:
public void JumpToPosition()
{
  if (hasLocation && position != null && !position.IsDeleted)
    ....
  else if (position != null)
    ....
}
Skipped null check
PVS-Studio diagnostic message: V3125 The 'mainAssemblyList' object was used after it was verified against null. Check lines: 304, 291. ClassBrowserPad.cs 304
void UpdateActiveWorkspace()
{
  var mainAssemblyList = SD.ClassBrowser.MainAssemblyList;
  if ((mainAssemblyList != null) && (activeWorkspace != null)) {
    ....
  }
  ....
  mainAssemblyList.Assemblies.Clear();  // <=
  ....
}  
The mainAssemblyList variable is used without a prior null check, while such a check can be found in another statement in this fragment. The fixed code:
void UpdateActiveWorkspace()
{
  var mainAssemblyList = SD.ClassBrowser.MainAssemblyList;
  if ((mainAssemblyList != null) && (activeWorkspace != null)) {
    ....
  }
  ....
  if (mainAssemblyList != null) {
    mainAssemblyList.Assemblies.Clear();
  }  
  ....
}
Unexpected sorting result
PVS-Studio diagnostic message: V3078 Original sorting order will be lost after repetitive call to 'OrderBy' method. Use 'ThenBy' method to preserve the original sorting. CodeCoverageMethodElement.cs 124
void Init()
{
  ....
  this.SequencePoints.OrderBy(item => item.Line)
                     .OrderBy(item => item.Column);  // <=
}
This code will sort the SequencePoints collection only by the Column field, which doesn't seem to be the desired result. The problem with this code is that the second call to the OrderBy method will sort the collection without taking into account the results of the previous sort. To fix this issue, method ThenBy must be used instead of the second call to OrderBy:
void Init()
{
  ....
  this.SequencePoints.OrderBy(item => item.Line)
                     .ThenBy(item => item.Column);
}
Possible division by zero
PVS-Studio diagnostic message: V3064 Potential division by zero. Consider inspecting denominator 'workAmount'. XamlSymbolSearch.cs 60
public XamlSymbolSearch(IProject project, ISymbol entity)
{
  ....
  interestingFileNames = new List<FileName>();
  ....
  foreach (var item in ....)
    interestingFileNames.Add(item.FileName);
  ....
  workAmount = interestingFileNames.Count;
  workAmountInverse = 1.0 / workAmount;  // <=
}
If the interestingFileNames collection is found to be empty, a division by zero will occur. I can't suggest a ready solution for this code, but in any case, the authors need to improve the algorithm computing the value of the workAmountInverse variable when the value of the workAmount variable is zero.
Repeated assignment
PVS-Studio diagnostic message: V3008 The 'ignoreDialogIdSelectedInTextEditor' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 204, 201. WixDialogDesigner.cs 204
void OpenDesigner()
{
  try {
    ignoreDialogIdSelectedInTextEditor = true;  // <=
    WorkbenchWindow.ActiveViewContent = this;
  } finally {
    ignoreDialogIdSelectedInTextEditor = false;  // <=
  }
}
The ignoreDialogIdSelectedInTextEditor variable will be assigned with the value false regardless of the result of executing the try block. Let's take a closer look at the variable declarations to make sure there are no "pitfalls" there. This is the declaration of ignoreDialogIdSelectedInTextEditor:
bool ignoreDialogIdSelectedInTextEditor;
And here are the declarations of IWorkbenchWindow and ActiveViewContent:
public IWorkbenchWindow WorkbenchWindow {
  get { return workbenchWindow; }
}
IViewContent ActiveViewContent {
  get;
  set;
}
As you can see, there are no legitimate reasons for assigning another value to the ignoreDialogIdSelectedInTextEditor variable. Perhaps the correct version of this construct should use the catch keyword instead of finally:
void OpenDesigner()
{
  try {
    ignoreDialogIdSelectedInTextEditor = true;
    WorkbenchWindow.ActiveViewContent = this;
  } catch {
    ignoreDialogIdSelectedInTextEditor = false;
  }
}
Incorrect search of a substring
PVS-Studio diagnostic message: V3053 An excessive expression. Examine the substrings '/debug' and '/debugport'. NDebugger.cs 287
public bool IsKernelDebuggerEnabled {
  get {
    ....
    if (systemStartOptions.Contains("/debug") ||
     systemStartOptions.Contains("/crashdebug") ||
     systemStartOptions.Contains("/debugport") ||  // <=
     systemStartOptions.Contains("/baudrate")) {
      return true;
    }
    ....
  }
}
This code uses a serial search looking for the substring "/debug" or "/debugport" in the systemStartOptions string. The problem with this fragment is that the "/debug" string is itself a substring of "/debugport", so finding "/debug" makes further search of "/debugport" meaningless. It's not a bug, but it won't harm to optimize the code:
public bool IsKernelDebuggerEnabled {
  get {
    ....
    if (systemStartOptions.Contains("/debug") ||
     systemStartOptions.Contains("/crashdebug") ||
     systemStartOptions.Contains("/baudrate")) {
      return true;
    }
    ....
  }
}
Exception-handling error
PVS-Studio diagnostic message: V3052 The original exception object 'ex' was swallowed. Stack of original exception could be lost. ReferenceFolderNodeCommands.cs 130
DiscoveryClientProtocol DiscoverWebServices(....)
{
  try {
    ....
  } catch (WebException ex) {
    if (....) {
      ....
    } else {
      throw ex;  // <=
    }
  }
  ....
}
Executing the throw ex call will result in overwriting the stack of the original exception, as the intercepted exception will be generated anew. This is the fixed version:
DiscoveryClientProtocol DiscoverWebServices(....)
{
  try {
    ....
  } catch (WebException ex) {
    if (....) {
      ....
    } else {
      throw;
    }
  }
  ....
}
Using an uninitialized field in a class constructor
PVS-Studio diagnostic message: V3128 The 'contentPanel' field is used before it is initialized in constructor. SearchResultsPad.cs 66
Grid contentPanel;
public SearchResultsPad()
{
  ....
  defaultToolbarItems = ToolBarService
    .CreateToolBarItems(contentPanel, ....);  // <=
  ....
  contentPanel = new Grid {....};
  ....
}
The contentPanel field is passed as one of the arguments to the CreateToolBarItems method in the constructor of the SearchResultsPad class. However, this field is not initialized until it has been used. It's not necessarily an error, given that the possibility of the contentPanel variable with the value of null is taken into account in the body of the CreateToolBarItems method and further on in the stack. This code still looks very suspicious and needs to be examined by the authors.
As I have already said, this article discusses far not all the bugs found by PVS-Studio in this project but only those that seemed interesting to me. The project authors are welcome to contact us to get a temporary license key for a more thorough analysis of their code.

Conclusion

PVS-Studio did well again and revealed new interesting bugs during the second check of SharpDevelop project. It means the analyzer knows how to do its job and can help make the world a bit better.
Remember that you, too, can join us at any time by taking the opportunity of checking your own projects with the free version of PVS-Studio static analyzer.
You can download PVS-Studio at http://www.viva64.com/en/pvs-studio/

Please email us if you have any questions regarding the purchase of a commercial license. You can also contact us to ask for a temporary license for deeper exploration of PVS-Studio without the limitations of the demo version.

Friday, January 27, 2017

How to capture a variable in C# and not to shoot yourself in the foot

Back in 2005, with the release of C# 2.0 standard we got a possibility to pass a variable to the body of an anonymous delegate by capturing it from the current context. In 2008 the C# 3.0 brought us lambdas, user anonymous classes, LINQ requests and much more. Now it January, 2017 and the majority of C# developers are looking forward to the release of the C# 7.0 standard that should provide us a bunch of new useful features. However, there are still old features that need to be fixed. That's why there are plenty of ways to shoot yourself in the foot. Today we are going to speak about one of them, and it is related with quite an unobvious mechanism of variable capture in the body of anonymous functions in C#.
Picture 1

Introduction

As I have stated above, we are going to discuss peculiarities of the mechanism of variable capture in the body of anonymous functions in C#. I should warn in advance, that the article will contain a large number of technical details, but I hope that both experienced and beginner programmers will find my article interesting and simple to comprehend.
But enough talking. I'll give you simple example of the code, you should tell, what will be printed in the console.
So, here we go.
void Foo()
{
  var actions = new List<Action>();
  for (int i = 0; i < 10; i++)
  {
    actions.Add(() => Console.WriteLine(i));
  }

  foreach(var a in actions)
  {
    a();
  }
}
And now attention please, here is the answer. The console will print the number 10 ten times.
10
10
10
10
10
10
10
10
10
10
This article is for those who thought otherwise. Let's try to sort out, what are the reasons of such behavior.

Why does it happen so?

Upon the declaration of an anonymous function (it can be an anonymous delegate or lambda) inside your class, one more container class will be declared during the compilation, which contains fields for all the captured variables and a method, containing a body of the anonymous function. The disassembled structure of the program for the code fragment given above will be as follows:
Picture 3
In this case the Foo method in this fragment is declared inside the Program class. The compiler generated a container class c__DisplayClass1_0 for the lambda () => Console.WriteLine(i), and inside of the class-container it generated a field i, having a captured variable with the same name and the method b__0, containing the body of the lambda.
Let's consider the disassembled IL code of the b__0 method (lambda body) with my comments:
.method assembly hidebysig instance void '<Foo>b__0'() cil managed
{
  .maxstack  8
  // Puts the current class item (equivalent to 'this')
  // to the top of the stack.
  // It is necessary for the access to
  // the fields of the current class. 
  IL_0000:  ldarg.0 
  
  // Puts the value of the 'i' field to the top of the stack 
  // of the current class instance 
  IL_0001:  ldfld int32 
    TestSolution.Program/'<>c__DisplayClass1_0'::i
  
  // Calls a method to output the string to the console. 
  // Passes values from the stack as arguments.
  IL_0006:  call     void [mscorlib]System.Console::WriteLine(int32)
  
  // Exits the method.
  IL_000b:  ret
}
All correct, that's exactly what we do inside lambda, no magic. Let's go on.
As we know, the int type (the full name is Int32) is a structure, which means that it passed by value, not by reference.
The value of the i variable should be copied (according the logic) during the creation of the container class instance. And if you answered my question in the beginning of the article incorrectly, then most likely you expected that the container would be created right before the declaration of the lambda in the code.
In reality, the i variable won't be created after the compilation in the Foo method at all. Instead of it, an instance of the container class c__DisplayClass1_0 will get created, and its field will be initialized with 0 instead of the variable. Moreover, in all the fragments where we used a local variable i, there will be a field of a container class used.
The important point is that an instance of the container class is created before the loop, because its field will be used in the loop as an iterator.
As a result, we get one instance of the container class for all iterations of the for loop. Adding a new lambda to the actions list upon every iteration, we actually add the same reference to the instance of the container class created previously. As a result, when we traverse all the items of the actions list with the foreach loop, they all have the same instance of the container class. And we take into account that the for loop increments the value of an iterator after every iteration (even after the last one), then the value of the i field inside the container class after the exit from the loop gets equal to 10 after executing the for loop.
You can make sure of it by looking at the disassembled IL code of the Foo method (with my comments):
.method private hidebysig instance void  Foo() cil managed
{
  .maxstack  3
  
  // -========== DECLARATION OF LOCAL VARIABLES ==========-
  .locals init(
    // A list of 'actions'. 
    [0] class [mscorlib]System.Collections.Generic.List'1
      <class [mscorlib]System.Action> actions,
    
    // A container class for the lambda.
    [1] class TestSolution.Program/
      '<>c__DisplayClass1_0' 'CS$<>8__locals0',
    
    // A technical variable V_2 is necessary for temporary
    // storing the results of the addition operation.
    [2] int32 V_2,
    
    // Technical variable V_3 is necessary for storing  
    // the enumerator of the 'actions' list during
    // the iteration of the 'foreach' loop.
    [3] valuetype
      [mscorlib]System.Collections.Generic.List'1/Enumerator<class
      [mscorlib]System.Action> V_3)

    
// -================= INITIALIZATION =================-
  // An instance of the Actions list is created and assigned to the  
  // 'actions' variable. 
  IL_0000:  newobj     instance void class
[mscorlib]System.Collections.Generic.List'1<class
[mscorlib]System.Action>::.ctor()

  IL_0005:  stloc.0
  
  // An instance of the container class is created  
  // and assigned to a corresponding local variable
  IL_0006:  newobj     instance void
    TestSolution.Program/'<>c__DisplayClass1_0'::.ctor()
  IL_000b:  stloc.1
  
  // A reference of the container class is loaded to the stack. 
  IL_000c:  ldloc.1
  
  // Number 0 is loaded to the stack.
  IL_000d:  ldc.i4.0
  
  // 0 is assigned to the 'i' field of the previous 
  // object on the stack (an instance of a container class). 
  IL_000e:  stfld      int32
    TestSolution.Program/'<>c__DisplayClass1_0'::i
  
  
  
  // -================= THE FOR LOOP =================-
  // Jumps to the command IL_0037.
  IL_0013:  br.s       IL_0037
  
  // The references of the 'actions'
  // list and an instance of the container class
  // are loaded to the stack.
  IL_0015:  ldloc.0
  IL_0016:  ldloc.1
  
  // The reference to the 'Foo' method of the container class 
  // is loaded to the stack. 
  IL_0017:  ldftn      instance void
    TestSolution.Program/'<>c__DisplayClass1_0'::'<Foo>b__0'()
  
  // An instance of the 'Action' class is created and the reference 
  // to the 'Foo' method of the container class is passed into it.
  IL_001d:  newobj     instance void
    [mscorlib]System.Action::.ctor(object, native int)
  
  // The method 'Add' is called for the 'actions' list  
  // by adding an instance of the 'Action' class. 
  IL_0022:  callvirt   instance void class
    [mscorlib]System.Collections.Generic.List'1<class
    [mscorlib]System.Action>::Add(!0)
  
  // The value of the 'i' field of the instance of a container class  
  // is loaded to the stack. 
  IL_0027:  ldloc.1
  IL_0028:  ldfld      int32
    TestSolution.Program/'<>c__DisplayClass1_0'::i
  
  // The value of the 'i' field is assigned
  // to the technical variable 'V_2'. 
  IL_002d:  stloc.2
  
  // The reference to the instance of a container class and the value 
  // of a technical variable 'V_2' is loaded to the stack.
  IL_002e:  ldloc.1
  IL_002f:  ldloc.2
  
  // 1 is loaded to the stack. 
  IL_0030:  ldc.i4.1
  
  // It adds two first values on the stack
  // and assigns them to the third. 
  IL_0031:  add
  
  // The result of the addition is assigned to the 'i' field
  // (in fact, it is an increment)
  IL_0032:  stfld      int32
    TestSolution.Program/'<>c__DisplayClass1_0'::i
  
  // The value of the 'i' field of the container class instance  
  // is loaded to the stack.
  IL_0037:  ldloc.1
  IL_0038:  ldfld      int32
    TestSolution.Program/'<>c__DisplayClass1_0'::i
  
  // 10 is loaded to the stack. 
  IL_003d:  ldc.i4.s   10
  
  // If the value of the 'i' field is less than 10,  
  // it jumps to the command IL_0015.
  IL_003f:  blt.s      IL_0015
  
  
  // -================= THE FOREACH LOOP =================-
  //// The reference to the 'actions' list is loaded to the stack. 
  IL_0041:  ldloc.0
  
  // The technical variable V_3 is assigned with the result 
  // of the 'GetEnumerator' method of the 'actions' list.
  IL_0042:  callvirt   instance valuetype
    [mscorlib]System.Collections.Generic.List'1/Enumerator<!0> class
    [mscorlib]System.Collections.Generic.List'1<class
    [mscorlib]System.Action>::GetEnumerator()

  IL_0047:  stloc.3
  
  // The initialization of the try block
  // (the foreach loop is converted to  
  // the try-finally construct)
  .try
  {
    // Jumps to the command IL_0056.
    IL_0048:  br.s       IL_0056
    
    // Calls get_Current method of the V_3 variable. 
    // The result is written to the stack. 
    // (A reference to the Action object in the current iteration). 
    IL_004a:  ldloca.s   V_3 
    IL_004c:  call       instance !0 valuetype
      [mscorlib]System.Collections.Generic.List'1/Enumerator<class
      [mscorlib]System.Action>::get_Current()
    
    // Calls the Invoke method of the Action
    // object in the current iteration
    IL_0051:  callvirt   instance void
      [mscorlib]System.Action::Invoke()
    
    // Calls MoveNext method of the V_3 variable.  
    // The result is written to the stack.
    IL_0056:  ldloca.s   V_3
    IL_0058:  call       instance bool valuetype
      [mscorlib]System.Collections.Generic.List'1/Enumerator<class
      [mscorlib]System.Action>::MoveNext()
    
    // If the result of the MoveNext method is not null,  
    // then it jumps to the IL_004a command. 
    IL_005d:  brtrue.s   IL_004a
    
    // Finishes the try block execution and jumps to finally.
    IL_005f:  leave.s    IL_006f
  }  // end .try
  finally
  {
    // Calls the Dispose method of the V_3 variable.  
    IL_0061:  ldloca.s   V_3
    IL_0063:  constrained. Valuetype
      [mscorlib]System.Collections.Generic.List'1/Enumerator<class
      [mscorlib]System.Action>

    IL_0069:  callvirt   instance void
      [mscorlib]System.IDisposable::Dispose()
    
    // Finishes the execution of the finally block. 
    IL_006e:  endfinally
  }
  
  //  Finishes the execution of the current method.
  IL_006f:  ret
}

Conclusion

The guys from Microsoft say that this is a feature, not a bug and that this behavior was made intentionally, aiming to increase the performance of the programs. You will find more information by this link. In reality it results in bugs and confusion of novice developers.
An interesting fact is that the foreach loop had the same behavior before the C# 5.0 standard. The Microsoft was bombarded with complaints about nonintuitive behavior in the bug-tracker, but with the release of the C# 5.0 standard this behavior was changed by declaring the iterator variable inside every loop iteration, not before it on the compilation stage, but for all other constructions similar behavior remained without any changes. More information can be found by the link in the Breaking Changes section.
You may ask how to avoid such an error? Actually the answer is very simple. You need to keep track of where and what variables you capture. Just remember that the container class will be created in that place where you have declared your variable that you will capture. If the capture occurs in the body of the loop, and the variable is declared outside it, then it's necessary to reassign it inside the body of the loop to a new local variable. The correct version of the example given in the beginning can be as follows:
void Foo()
{
  var actions = new List<Action>();
  for (int i = 0; i < 10; i++)
  {
    var index = i; // <=
    actions.Add(() => Console.WriteLine(index));
  }

  foreach(var a in actions)
  {
    a();
  }
}
If you execute this code, the console will show the numbers from 0 to 9, as expected:
0
1
2
3
4
5
6
7
8
9
Looking at the IL code of the for loop from this example, we'll see that an instance of the container class will be created upon every iteration of the loop. Thus, the actions list will contain references to various instances with correct values of the iterators.
// -================= THE FOR LOOP =================-
// Jumps to the command IL_002d.
IL_0008:  br.s       IL_002d

// Creates an instance of a container class
// and loads the reference to the stack.
IL_000a:  newobj     instance void
  TestSolution.Program/'<>c__DisplayClass1_0'::.ctor()

IL_000f:  stloc.2
IL_0010:  ldloc.2

// Assigns the 'index' field in the container class  
// with a value 'i'. 
IL_0011:  ldloc.1
IL_0012:  stfld      int32
  TestSolution.Program/'<>c__DisplayClass1_0'::index

// Creates an instance of the 'Action' class with a reference to  
// the method of a container class and add it to the 'actions' list.
IL_0017:  ldloc.0
IL_0018:  ldloc.2
IL_0019:  ldftn      instance void
  TestSolution.Program/'<>c__DisplayClass1_0'::'<Foo>b__0'()

IL_001f:  newobj     instance void
  [mscorlib]System.Action::.ctor(object, native int)

IL_0024:  callvirt   instance void class
  [mscorlib]System.Collections.Generic.List'1<class
  [mscorlib]System.Action>::Add(!0)
 
// Performs the increment to the 'i' variable
IL_0029:  ldloc.1
IL_002a:  ldc.i4.1
IL_002b:  add
IL_002c:  stloc.1

// Loads the value of the 'i' variable to the stack
// This time it is not in the container class 
IL_002d:  ldloc.1

// Compares the value of the variable 'i' with 10.
// If 'i < 10', then jumps to the command IL_000a.
IL_002e:  ldc.i4.s   10
IL_0030:  blt.s      IL_000a
Finally, let me remind you that we are all human beings and we all make errors, that's why it would be illogical, and as a rule long and resource-intensive to hope only for the human factor when searching for bugs and typos. So, it's always a good idea to use technical solutions to detect errors in the code. The machine doesn't get tired and does the work much quicker.
Quite recently, we as a team of PVS-Studio static code analyzer developers have created a diagnostic rule that is aimed at detecting incorrect capture of the variables and anonymous functions inside the loops. In my turn I suggest checking your code with our analyzer and see if it can detect bugs in your code.

At this point, I'm finishing my article, I wish you clean code bugless programs.