Automatically update local variable debug info (#676)

* Automatically update local variable debug info

When manipulating the local variables collection on a method body, automatically update the debug info for all affected local variables.

When removing a local variable, also remove the debug info for that local variable from the scopes.

When removing or inserting update the indeces of local variable debug infos which are afected by the action. Note the local variable debug info either holds just a backpointer to the local variable in which case it doesn't store the index at all (and thus nothing to do), or it stores the index explicitly in which case it needs to be updated.

Added tests for both insert and remove cases.

* Add internal properties on `VariableOffset` to make it easier to manipulate it.

Reworked the `UpdateVariableIndeces` to only loop over variables once and also to handle removal of resolved variable debug infos.

* Simplify the code a little and add comments

* PR feedback

Mono.Cecil.Cil/CodeReader.cs

@@ -132,7 +132,7 @@ void ReadFatMethod ()
 		public VariableDefinitionCollection ReadVariables (MetadataToken local_var_token)
 		{
 			var position = reader.position;
-			var variables = reader.ReadVariables (local_var_token);
+			var variables = reader.ReadVariables (local_var_token, method);
 			reader.position = position;
 
 			return variables;

Mono.Cecil.Cil/MethodBody.cs

@@ -81,7 +81,7 @@ public bool HasVariables {
 		public Collection<VariableDefinition> Variables {
 			get {
 				if (variables == null)
-					Interlocked.CompareExchange (ref variables, new VariableDefinitionCollection (), null);
+					Interlocked.CompareExchange (ref variables, new VariableDefinitionCollection (this.method), null);
 
 				return variables;
 			}
@@ -134,13 +134,17 @@ public ILProcessor GetILProcessor ()
 
 	sealed class VariableDefinitionCollection : Collection<VariableDefinition> {
 
-		internal VariableDefinitionCollection ()
+		readonly MethodDefinition method;
+
+		internal VariableDefinitionCollection (MethodDefinition method)
 		{
+			this.method = method;
 		}
 
-		internal VariableDefinitionCollection (int capacity)
+		internal VariableDefinitionCollection (MethodDefinition method, int capacity)
 			: base (capacity)
 		{
+			this.method = method;
 		}
 
 		protected override void OnAdd (VariableDefinition item, int index)
@@ -151,9 +155,7 @@ protected override void OnAdd (VariableDefinition item, int index)
 		protected override void OnInsert (VariableDefinition item, int index)
 		{
 			item.index = index;
-
-			for (int i = index; i < size; i++)
-				items [i].index = i + 1;
+			UpdateVariableIndices (index, 1);
 		}
 
 		protected override void OnSet (VariableDefinition item, int index)
@@ -163,10 +165,47 @@ protected override void OnSet (VariableDefinition item, int index)
 
 		protected override void OnRemove (VariableDefinition item, int index)
 		{
+			UpdateVariableIndices (index + 1, -1, item);
 			item.index = -1;
+		}
 
-			for (int i = index + 1; i < size; i++)
-				items [i].index = i - 1;
+		void UpdateVariableIndices (int startIndex, int offset, VariableDefinition variableToRemove = null)
+		{
+			for (int i = startIndex; i < size; i++)
+				items [i].index = i + offset;
+
+			var debug_info = method == null ? null : method.debug_info;
+			if (debug_info == null || debug_info.Scope == null)
+				return;
+
+			foreach (var scope in debug_info.GetScopes ()) {
+				if (!scope.HasVariables)
+					continue;
+
+				var variables = scope.Variables;
+				int variableDebugInfoIndexToRemove = -1;
+				for (int i = 0; i < variables.Count; i++) {
+					var variable = variables [i];
+
+					// If a variable is being removed detect if it has debug info counterpart, if so remove that as well.
+					// Note that the debug info can be either resolved (has direct reference to the VariableDefinition)
+					// or unresolved (has only the number index of the variable) - this needs to handle both cases.
+					if (variableToRemove != null &&
+						((variable.index.IsResolved && variable.index.ResolvedVariable == variableToRemove) ||
+							(!variable.index.IsResolved && variable.Index == variableToRemove.Index))) {
+						variableDebugInfoIndexToRemove = i;
+						continue;
+					}
+
+					// For unresolved debug info updates indeces to keep them pointing to the same variable.
+					if (!variable.index.IsResolved && variable.Index >= startIndex) {
+						variable.index = new VariableIndex (variable.Index + offset);
+					}
+				}
+
+				if (variableDebugInfoIndexToRemove >= 0)
+					variables.RemoveAt (variableDebugInfoIndexToRemove);
+			}
 		}
 	}
 

Mono.Cecil.Cil/Symbols.cs

@@ -248,6 +248,10 @@ public int Index {
 			}
 		}
 
+		internal bool IsResolved => variable != null;
+
+		internal VariableDefinition ResolvedVariable => variable;
+
 		public VariableIndex (VariableDefinition variable)
 		{
 			if (variable == null)

Mono.Cecil/AssemblyReader.cs

@@ -2141,7 +2141,7 @@ public CallSite ReadCallSite (MetadataToken token)
 			return call_site;
 		}
 
-		public VariableDefinitionCollection ReadVariables (MetadataToken local_var_token)
+		public VariableDefinitionCollection ReadVariables (MetadataToken local_var_token, MethodDefinition method = null)
 		{
 			if (!MoveTo (Table.StandAloneSig, local_var_token.RID))
 				return null;
@@ -2156,7 +2156,7 @@ public VariableDefinitionCollection ReadVariables (MetadataToken local_var_token
 			if (count == 0)
 				return null;
 
-			var variables = new VariableDefinitionCollection ((int) count);
+			var variables = new VariableDefinitionCollection (method, (int) count);
 
 			for (int i = 0; i < count; i++)
 				variables.Add (new VariableDefinition (reader.ReadTypeSignature ()));

Test/Mono.Cecil.Tests/VariableTests.cs

@@ -80,6 +80,45 @@ public void RemoveVariableIndex ()
 			Assert.AreEqual (1, z.Index);
 		}
 
+		[Test]
+		public void RemoveVariableWithDebugInfo ()
+		{
+			var object_ref = new TypeReference ("System", "Object", null, null, false);
+			var method = new MethodDefinition ("foo", MethodAttributes.Static, object_ref);
+			var body = new MethodBody (method);
+			var il = body.GetILProcessor ();
+			il.Emit (OpCodes.Ret);
+
+			var x = new VariableDefinition (object_ref);
+			var y = new VariableDefinition (object_ref);
+			var z = new VariableDefinition (object_ref);
+			var z2 = new VariableDefinition (object_ref);
+
+			body.Variables.Add (x);
+			body.Variables.Add (y);
+			body.Variables.Add (z);
+			body.Variables.Add (z2);
+
+			var scope = new ScopeDebugInformation (body.Instructions [0], body.Instructions [0]);
+			method.DebugInformation = new MethodDebugInformation (method) {
+				Scope = scope
+			};
+			scope.Variables.Add (new VariableDebugInformation (x.index, nameof (x)));
+			scope.Variables.Add (new VariableDebugInformation (y.index, nameof (y)));
+			scope.Variables.Add (new VariableDebugInformation (z.index, nameof (z)));
+			scope.Variables.Add (new VariableDebugInformation (z2, nameof (z2)));
+
+			body.Variables.Remove (y);
+
+			Assert.AreEqual (3, scope.Variables.Count);
+			Assert.AreEqual (x.Index, scope.Variables [0].Index);
+			Assert.AreEqual (nameof (x), scope.Variables [0].Name);
+			Assert.AreEqual (z.Index, scope.Variables [1].Index);
+			Assert.AreEqual (nameof (z), scope.Variables [1].Name);
+			Assert.AreEqual (z2.Index, scope.Variables [2].Index);
+			Assert.AreEqual (nameof (z2), scope.Variables [2].Name);
+		}
+
 		[Test]
 		public void InsertVariableIndex ()
 		{
@@ -104,5 +143,43 @@ public void InsertVariableIndex ()
 			Assert.AreEqual (1, y.Index);
 			Assert.AreEqual (2, z.Index);
 		}
+
+		[Test]
+		public void InsertVariableWithDebugInfo ()
+		{
+			var object_ref = new TypeReference ("System", "Object", null, null, false);
+			var method = new MethodDefinition ("foo", MethodAttributes.Static, object_ref);
+			var body = new MethodBody (method);
+			var il = body.GetILProcessor ();
+			il.Emit (OpCodes.Ret);
+
+			var x = new VariableDefinition (object_ref);
+			var y = new VariableDefinition (object_ref);
+			var z = new VariableDefinition (object_ref);
+			var z2 = new VariableDefinition (object_ref);
+
+			body.Variables.Add (x);
+			body.Variables.Add (z);
+			body.Variables.Add (z2);
+
+			var scope = new ScopeDebugInformation (body.Instructions [0], body.Instructions [0]);
+			method.DebugInformation = new MethodDebugInformation (method) {
+				Scope = scope
+			};
+			scope.Variables.Add (new VariableDebugInformation (x.index, nameof (x)));
+			scope.Variables.Add (new VariableDebugInformation (z.index, nameof (z)));
+			scope.Variables.Add (new VariableDebugInformation (z2, nameof (z2)));
+
+			body.Variables.Insert (1, y);
+
+			// Adding local variable doesn't add debug info for it (since there's no way to deduce the name of the variable)
+			Assert.AreEqual (3, scope.Variables.Count);
+			Assert.AreEqual (x.Index, scope.Variables [0].Index);
+			Assert.AreEqual (nameof (x), scope.Variables [0].Name);
+			Assert.AreEqual (z.Index, scope.Variables [1].Index);
+			Assert.AreEqual (nameof (z), scope.Variables [1].Name);
+			Assert.AreEqual (z2.Index, scope.Variables [2].Index);
+			Assert.AreEqual (nameof (z2), scope.Variables [2].Name);
+		}
 	}
 }