[XamlC] detect duplicate x:Name at compile time (#655)

* [XamlC] detect duplicate x:Name at compile time

* invoking methods with the right arguments produces better results

Xamarin.Forms.Build.Tasks/ILContext.cs

@@ -1,6 +1,9 @@
+using System;
 using System.Collections.Generic;
+
 using Mono.Cecil;
 using Mono.Cecil.Cil;
+
 using Xamarin.Forms.Xaml;
 
 namespace Xamarin.Forms.Build.Tasks
@@ -13,7 +16,7 @@ namespace Xamarin.Forms.Build.Tasks
 			Body = body;
 			Values = new Dictionary<IValueNode, object>();
 			Variables = new Dictionary<IElementNode, VariableDefinition>();
-			Scopes = new Dictionary<INode, VariableDefinition>();
+			Scopes = new Dictionary<INode, Tuple<VariableDefinition, IList<string>>>();
 			TypeExtensions = new Dictionary<INode, TypeReference>();
 			ParentContextValues = parentContextValues;
 			Module = module;
@@ -23,7 +26,7 @@ namespace Xamarin.Forms.Build.Tasks
 
 		public Dictionary<IElementNode, VariableDefinition> Variables { get; private set; }
 
-		public Dictionary<INode, VariableDefinition> Scopes { get; private set; }
+		public Dictionary<INode, Tuple<VariableDefinition, IList<string>>> Scopes { get; private set; }
 
 		public Dictionary<INode, TypeReference> TypeExtensions { get; }
 

Xamarin.Forms.Build.Tasks/NodeILExtensions.cs

@@ -488,7 +488,7 @@ namespace Xamarin.Forms.Build.Tasks
 				yield return Instruction.Create(OpCodes.Dup); //Duplicate the namescopeProvider
 				var setNamescope = module.Import(typeof (NameScopeProvider).GetProperty("NameScope").GetSetMethod());
 
-				yield return Instruction.Create(OpCodes.Ldloc, context.Scopes[node]);
+				yield return Instruction.Create(OpCodes.Ldloc, context.Scopes[node].Item1);
 				yield return Instruction.Create(OpCodes.Callvirt, setNamescope);
 				yield return Instruction.Create(OpCodes.Callvirt, addService);
 			}

Xamarin.Forms.Build.Tasks/SetNamescopesAndRegisterNamesVisitor.cs

@@ -1,4 +1,6 @@
+using System.Collections.Generic;
 using System.Linq;
+using System.Xml;
 using Mono.Cecil.Cil;
 using Xamarin.Forms.Internals;
 using Xamarin.Forms.Xaml;
@@ -33,7 +35,7 @@ namespace Xamarin.Forms.Build.Tasks
 		{
 			Context.Scopes[node] = Context.Scopes[parentNode];
 			if (IsXNameProperty(node, parentNode))
-				RegisterName((string)node.Value, Context.Scopes[node], Context.Variables[(IElementNode)parentNode], node);
+				RegisterName((string)node.Value, Context.Scopes[node].Item1, Context.Scopes[node].Item2, Context.Variables[(IElementNode)parentNode], node);
 		}
 
 		public void Visit(MarkupNode node, INode parentNode)
@@ -43,26 +45,27 @@ namespace Xamarin.Forms.Build.Tasks
 
 		public void Visit(ElementNode node, INode parentNode)
 		{
-			VariableDefinition ns;
-			if (parentNode == null || IsDataTemplate(node, parentNode) || IsStyle(node, parentNode))
-				ns = CreateNamescope();
-			else
-				ns = Context.Scopes[parentNode];
-			if (
-				Context.Variables[node].VariableType.InheritsFromOrImplements(
-					Context.Body.Method.Module.Import(typeof (BindableObject))))
-				SetNameScope(node, ns);
-			Context.Scopes[node] = ns;
+			VariableDefinition namescopeVarDef;
+			IList<string> namesInNamescope;
+			if (parentNode == null || IsDataTemplate(node, parentNode) || IsStyle(node, parentNode)) {
+				namescopeVarDef = CreateNamescope();
+				namesInNamescope = new List<string>();
+			} else {
+				namescopeVarDef = Context.Scopes[parentNode].Item1;
+				namesInNamescope = Context.Scopes[parentNode].Item2;
+			}
+			if (Context.Variables[node].VariableType.InheritsFromOrImplements(Context.Body.Method.Module.Import(typeof (BindableObject))))
+				SetNameScope(node, namescopeVarDef);
+			Context.Scopes[node] = new System.Tuple<VariableDefinition, IList<string>>(namescopeVarDef, namesInNamescope);
 		}
 
 		public void Visit(RootNode node, INode parentNode)
 		{
-			var ns = CreateNamescope();
-			if (
-				Context.Variables[node].VariableType.InheritsFromOrImplements(
-					Context.Body.Method.Module.Import(typeof (BindableObject))))
-				SetNameScope(node, ns);
-			Context.Scopes[node] = ns;
+			var namescopeVarDef = CreateNamescope();
+			IList<string> namesInNamescope = new List<string>();
+			if (Context.Variables[node].VariableType.InheritsFromOrImplements(Context.Body.Method.Module.Import(typeof (BindableObject))))
+				SetNameScope(node, namescopeVarDef);
+			Context.Scopes[node] = new System.Tuple<VariableDefinition, IList<string>>(namescopeVarDef, namesInNamescope);
 		}
 
 		public void Visit(ListNode node, INode parentNode)
@@ -121,18 +124,21 @@ namespace Xamarin.Forms.Build.Tasks
 			Context.IL.Emit(OpCodes.Call, setNS);
 		}
 
-		void RegisterName(string str, VariableDefinition scope, VariableDefinition element, INode node)
+		void RegisterName(string str, VariableDefinition namescopeVarDef, IList<string> namesInNamescope, VariableDefinition element, INode node)
 		{
+			if (namesInNamescope.Contains(str))
+				throw new XamlParseException($"An element with the name \"{str}\" already exists in this NameScope", node as IXmlLineInfo);
+			namesInNamescope.Add(str);
+
 			var module = Context.Body.Method.Module;
 			var nsRef = module.Import(typeof (INameScope));
 			var nsDef = nsRef.Resolve();
-			var registerInfo = nsDef.Methods.First(md => md.Name == "RegisterName" && md.Parameters.Count == 3);
+			var registerInfo = nsDef.Methods.First(md => md.Name == "RegisterName" && md.Parameters.Count == 2);
 			var register = module.Import(registerInfo);
 
-			Context.IL.Emit(OpCodes.Ldloc, scope);
+			Context.IL.Emit(OpCodes.Ldloc, namescopeVarDef);
 			Context.IL.Emit(OpCodes.Ldstr, str);
 			Context.IL.Emit(OpCodes.Ldloc, element);
-			Context.IL.Append(node.PushXmlLineInfo(Context));
 			Context.IL.Emit(OpCodes.Callvirt, register);
 		}
 	}

Xamarin.Forms.Core/Element.cs

@@ -280,6 +280,7 @@ namespace Xamarin.Forms
 			namescope.RegisterName(name, scopedElement);
 		}
 
+		[Obsolete]
 		void INameScope.RegisterName(string name, object scopedElement, IXmlLineInfo xmlLineInfo)
 		{
 			INameScope namescope = GetNameScope();

Xamarin.Forms.Core/Internals/INameScope.cs

@@ -1,3 +1,4 @@
+using System;
 using System.Xml;
 
 namespace Xamarin.Forms.Internals
@@ -6,7 +7,7 @@ namespace Xamarin.Forms.Internals
 	{
 		object FindByName(string name);
 		void RegisterName(string name, object scopedElement);
-		void RegisterName(string name, object scopedElement, IXmlLineInfo xmlLineInfo);
 		void UnregisterName(string name);
+		[Obsolete]void RegisterName(string name, object scopedElement, IXmlLineInfo xmlLineInfo);
 	}
 }

Xamarin.Forms.Core/Internals/NameScope.cs

@@ -26,6 +26,7 @@ namespace Xamarin.Forms.Internals
 			_names[name] = scopedElement;
 		}
 
+		[Obsolete]
 		void INameScope.RegisterName(string name, object scopedElement, IXmlLineInfo xmlLineInfo)
 		{
 			try

Xamarin.Forms.Xaml.UnitTests/Issues/Issue2125.xaml

@@ -1,7 +0,0 @@
-<?xml version="1.0" encoding="UTF-8"?>
-<ContentPage xmlns="http://xamarin.com/schemas/2014/forms" xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml" x:Class="Xamarin.Forms.Xaml.UnitTests.Issue2125">
-	<StackLayout>
-		<Label x:Name="label"/>
-		<Label x:Name="label"/>
-	</StackLayout>
-</ContentPage>

Xamarin.Forms.Xaml.UnitTests/Issues/Issue2125.xaml.cs

@@ -1,33 +0,0 @@
-using System;
-using System.Collections.Generic;
-
-using Xamarin.Forms;
-
-using NUnit.Framework;
-
-namespace Xamarin.Forms.Xaml.UnitTests
-{	
-	public partial class Issue2125 : ContentPage
-	{	
-		public Issue2125 ()
-		{
-			InitializeComponent ();
-		}
-
-		public Issue2125 (bool useCompiledXaml)
-		{
-			//this stub will be replaced at compile time
-		}
-
-		[TestFixture]
-		public class Tests
-		{
-			[TestCase (false)]
-			[TestCase (true)]
-			public void DuplicatexName (bool useCompiledXaml)
-			{
-				Assert.Throws (new XamlParseExceptionConstraint (5, 10), () => new Issue2125 (useCompiledXaml));
-			}
-		}
-	}
-}

Xamarin.Forms.Xaml.UnitTests/Issues/Issue2450.xaml.cs

@@ -5,6 +5,8 @@ using Xamarin.Forms.Core.UnitTests;
 
 namespace Xamarin.Forms.Xaml.UnitTests
 {
+	//this covers Issue2125 as well
+	[XamlCompilation(XamlCompilationOptions.Skip)]
 	public partial class Issue2450 : ContentPage
 	{
 		public Issue2450 ()
@@ -30,7 +32,12 @@ namespace Xamarin.Forms.Xaml.UnitTests
 			[TestCase (true)]
 			public void ThrowMeaningfulExceptionOnDuplicateXName (bool useCompiledXaml)
 			{
-				Assert.Throws (new XamlParseExceptionConstraint (8, 10, m => m == "An element with the name \"label0\" already exists in this NameScope"), () => new Issue2450 (useCompiledXaml));
+				if (useCompiledXaml)
+					Assert.Throws(new XamlParseExceptionConstraint(8, 10, m => m == "An element with the name \"label0\" already exists in this NameScope"),
+								  () => MockCompiler.Compile(typeof(Issue2450)));
+				else
+					Assert.Throws(new XamlParseExceptionConstraint(8, 10, m => m == "An element with the name \"label0\" already exists in this NameScope"),
+								  () => new Issue2450(useCompiledXaml));
 			}
 		}
 	}

Xamarin.Forms.Xaml.UnitTests/Xamarin.Forms.Xaml.UnitTests.csproj

@@ -131,9 +131,6 @@
     <Compile Include="Issues\Issue2114.xaml.cs">
       <DependentUpon>Issue2114.xaml</DependentUpon>
     </Compile>
-    <Compile Include="Issues\Issue2125.xaml.cs">
-      <DependentUpon>Issue2125.xaml</DependentUpon>
-    </Compile>
     <Compile Include="Validation\StaticExtensionException.xaml.cs">
       <DependentUpon>StaticExtensionException.xaml</DependentUpon>
     </Compile>
@@ -469,9 +466,6 @@
     <EmbeddedResource Include="Issues\Issue2114.xaml">
       <Generator>MSBuild:UpdateDesignTimeXaml</Generator>
     </EmbeddedResource>
-    <EmbeddedResource Include="Issues\Issue2125.xaml">
-      <Generator>MSBuild:UpdateDesignTimeXaml</Generator>
-    </EmbeddedResource>
     <EmbeddedResource Include="Validation\StaticExtensionException.xaml">
       <Generator>MSBuild:UpdateDesignTimeXaml</Generator>
     </EmbeddedResource>

Xamarin.Forms.Xaml/RegisterXNamesVisitor.cs

@@ -39,8 +39,7 @@ namespace Xamarin.Forms.Xaml
 			{
 				if (ae.ParamName != "name")
 					throw ae;
-				throw new XamlParseException(
-					string.Format("An element with the name \"{0}\" already exists in this NameScope", (string)node.Value), node);
+				throw new XamlParseException($"An element with the name \"{(string)node.Value}\" already exists in this NameScope", node);
 			}
 		}