From 32dab1d3c7641688e9435be22297e092bcdb5ee6 Mon Sep 17 00:00:00 2001 From: Stephane Delcroix Date: Fri, 30 Dec 2016 17:58:48 +0100 Subject: [PATCH] [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 | 7 ++- Xamarin.Forms.Build.Tasks/NodeILExtensions.cs | 2 +- .../SetNamescopesAndRegisterNamesVisitor.cs | 48 +++++++++++-------- Xamarin.Forms.Core/Element.cs | 1 + Xamarin.Forms.Core/Internals/INameScope.cs | 3 +- Xamarin.Forms.Core/Internals/NameScope.cs | 1 + .../Issues/Issue2125.xaml | 7 --- .../Issues/Issue2125.xaml.cs | 33 ------------- .../Issues/Issue2450.xaml.cs | 9 +++- .../Xamarin.Forms.Xaml.UnitTests.csproj | 6 --- Xamarin.Forms.Xaml/RegisterXNamesVisitor.cs | 3 +- 11 files changed, 46 insertions(+), 74 deletions(-) delete mode 100644 Xamarin.Forms.Xaml.UnitTests/Issues/Issue2125.xaml delete mode 100644 Xamarin.Forms.Xaml.UnitTests/Issues/Issue2125.xaml.cs diff --git a/Xamarin.Forms.Build.Tasks/ILContext.cs b/Xamarin.Forms.Build.Tasks/ILContext.cs index 8f36227ae..bcd483b14 100644 --- a/Xamarin.Forms.Build.Tasks/ILContext.cs +++ b/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(); Variables = new Dictionary(); - Scopes = new Dictionary(); + Scopes = new Dictionary>>(); TypeExtensions = new Dictionary(); ParentContextValues = parentContextValues; Module = module; @@ -23,7 +26,7 @@ namespace Xamarin.Forms.Build.Tasks public Dictionary Variables { get; private set; } - public Dictionary Scopes { get; private set; } + public Dictionary>> Scopes { get; private set; } public Dictionary TypeExtensions { get; } diff --git a/Xamarin.Forms.Build.Tasks/NodeILExtensions.cs b/Xamarin.Forms.Build.Tasks/NodeILExtensions.cs index ec0687973..cfd22f302 100644 --- a/Xamarin.Forms.Build.Tasks/NodeILExtensions.cs +++ b/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); } diff --git a/Xamarin.Forms.Build.Tasks/SetNamescopesAndRegisterNamesVisitor.cs b/Xamarin.Forms.Build.Tasks/SetNamescopesAndRegisterNamesVisitor.cs index 8e03b925e..860ff44e9 100644 --- a/Xamarin.Forms.Build.Tasks/SetNamescopesAndRegisterNamesVisitor.cs +++ b/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 namesInNamescope; + if (parentNode == null || IsDataTemplate(node, parentNode) || IsStyle(node, parentNode)) { + namescopeVarDef = CreateNamescope(); + namesInNamescope = new List(); + } 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>(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 namesInNamescope = new List(); + if (Context.Variables[node].VariableType.InheritsFromOrImplements(Context.Body.Method.Module.Import(typeof (BindableObject)))) + SetNameScope(node, namescopeVarDef); + Context.Scopes[node] = new System.Tuple>(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 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); } } diff --git a/Xamarin.Forms.Core/Element.cs b/Xamarin.Forms.Core/Element.cs index 19d7d8555..d0f713e4a 100644 --- a/Xamarin.Forms.Core/Element.cs +++ b/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(); diff --git a/Xamarin.Forms.Core/Internals/INameScope.cs b/Xamarin.Forms.Core/Internals/INameScope.cs index ca520605c..6485527b2 100644 --- a/Xamarin.Forms.Core/Internals/INameScope.cs +++ b/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); } } \ No newline at end of file diff --git a/Xamarin.Forms.Core/Internals/NameScope.cs b/Xamarin.Forms.Core/Internals/NameScope.cs index b29534b67..7bb5ff52f 100644 --- a/Xamarin.Forms.Core/Internals/NameScope.cs +++ b/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 diff --git a/Xamarin.Forms.Xaml.UnitTests/Issues/Issue2125.xaml b/Xamarin.Forms.Xaml.UnitTests/Issues/Issue2125.xaml deleted file mode 100644 index c2bc239cb..000000000 --- a/Xamarin.Forms.Xaml.UnitTests/Issues/Issue2125.xaml +++ /dev/null @@ -1,7 +0,0 @@ - - - - - \ No newline at end of file diff --git a/Xamarin.Forms.Xaml.UnitTests/Issues/Issue2125.xaml.cs b/Xamarin.Forms.Xaml.UnitTests/Issues/Issue2125.xaml.cs deleted file mode 100644 index 8c4dcf807..000000000 --- a/Xamarin.Forms.Xaml.UnitTests/Issues/Issue2125.xaml.cs +++ /dev/null @@ -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)); - } - } - } -} \ No newline at end of file diff --git a/Xamarin.Forms.Xaml.UnitTests/Issues/Issue2450.xaml.cs b/Xamarin.Forms.Xaml.UnitTests/Issues/Issue2450.xaml.cs index 0cf6f56c7..2d330934c 100644 --- a/Xamarin.Forms.Xaml.UnitTests/Issues/Issue2450.xaml.cs +++ b/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)); } } } diff --git a/Xamarin.Forms.Xaml.UnitTests/Xamarin.Forms.Xaml.UnitTests.csproj b/Xamarin.Forms.Xaml.UnitTests/Xamarin.Forms.Xaml.UnitTests.csproj index 8864a3883..b66a0824e 100644 --- a/Xamarin.Forms.Xaml.UnitTests/Xamarin.Forms.Xaml.UnitTests.csproj +++ b/Xamarin.Forms.Xaml.UnitTests/Xamarin.Forms.Xaml.UnitTests.csproj @@ -131,9 +131,6 @@ Issue2114.xaml - - Issue2125.xaml - StaticExtensionException.xaml @@ -469,9 +466,6 @@ MSBuild:UpdateDesignTimeXaml - - MSBuild:UpdateDesignTimeXaml - MSBuild:UpdateDesignTimeXaml diff --git a/Xamarin.Forms.Xaml/RegisterXNamesVisitor.cs b/Xamarin.Forms.Xaml/RegisterXNamesVisitor.cs index 7985cff1c..a7da3b987 100644 --- a/Xamarin.Forms.Xaml/RegisterXNamesVisitor.cs +++ b/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); } }