From 6753ace40e68f8aa6d45eefa90117f5650984c6f Mon Sep 17 00:00:00 2001
From: Stephane Delcroix <stephane@delcroix.org>
Date: Wed, 19 Dec 2018 16:14:21 +0100
Subject: [PATCH] [Xaml] require escaping curly braces (#4723)

This is a breaking change as we weren't throwing errors on that before.

- fixes #2756
---
 .../EmptyViewTemplateGallery.xaml                  |  2 +-
 Xamarin.Forms.Xaml.UnitTests/Issues/Gh2756.xaml    |  7 +++
 Xamarin.Forms.Xaml.UnitTests/Issues/Gh2756.xaml.cs | 34 ++++++++++++
 Xamarin.Forms.Xaml.UnitTests/Issues/Gh3539.xaml    |  6 +--
 Xamarin.Forms.Xaml.UnitTests/Issues/Issue1438.xaml |  4 +-
 Xamarin.Forms.Xaml.UnitTests/Issues/Issue1549.cs   |  2 +-
 .../MarkupExpressionParserTests.cs                 | 22 +++++---
 Xamarin.Forms.Xaml/MarkupExpressionParser.cs       | 14 +++--
 Xamarin.Forms.Xaml/MarkupExtensionParser.cs        | 62 ++++++++++++----------
 9 files changed, 108 insertions(+), 45 deletions(-)
 create mode 100644 Xamarin.Forms.Xaml.UnitTests/Issues/Gh2756.xaml
 create mode 100644 Xamarin.Forms.Xaml.UnitTests/Issues/Gh2756.xaml.cs

diff --git a/Xamarin.Forms.Controls/GalleryPages/CollectionViewGalleries/EmptyViewGalleries/EmptyViewTemplateGallery.xaml b/Xamarin.Forms.Controls/GalleryPages/CollectionViewGalleries/EmptyViewGalleries/EmptyViewTemplateGallery.xaml
index da35604..b94c268 100644
--- a/Xamarin.Forms.Controls/GalleryPages/CollectionViewGalleries/EmptyViewGalleries/EmptyViewTemplateGallery.xaml
+++ b/Xamarin.Forms.Controls/GalleryPages/CollectionViewGalleries/EmptyViewGalleries/EmptyViewTemplateGallery.xaml
@@ -20,7 +20,7 @@
 						<StackLayout> 
 							<Label FontAttributes="Bold" FontSize="18" Margin="10,25,10,10"
 									HorizontalOptions="Fill" HorizontalTextAlignment="Center" 
-									Text="{Binding Filter, StringFormat='Your filter term of {0} did not match any records'}"></Label>
+									Text="{Binding Filter, StringFormat='{}Your filter term of {0} did not match any records'}"></Label>
 						</StackLayout>
 					</DataTemplate>
 				</CollectionView.EmptyViewTemplate>
diff --git a/Xamarin.Forms.Xaml.UnitTests/Issues/Gh2756.xaml b/Xamarin.Forms.Xaml.UnitTests/Issues/Gh2756.xaml
new file mode 100644
index 0000000..c9259f7
--- /dev/null
+++ b/Xamarin.Forms.Xaml.UnitTests/Issues/Gh2756.xaml
@@ -0,0 +1,7 @@
+<?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.Gh2756">
+    <Label Text="{Binding Path=StartTime, StringFormat='{0:yy-MM-dd}'}"/>
+</ContentPage>
diff --git a/Xamarin.Forms.Xaml.UnitTests/Issues/Gh2756.xaml.cs b/Xamarin.Forms.Xaml.UnitTests/Issues/Gh2756.xaml.cs
new file mode 100644
index 0000000..eae564d
--- /dev/null
+++ b/Xamarin.Forms.Xaml.UnitTests/Issues/Gh2756.xaml.cs
@@ -0,0 +1,34 @@
+using NUnit.Framework;
+
+using Xamarin.Forms.Core.UnitTests;
+
+using Xamarin.Forms;
+
+namespace Xamarin.Forms.Xaml.UnitTests
+{
+	[XamlCompilation(XamlCompilationOptions.Skip)]
+	public partial class Gh2756 : ContentPage
+	{
+		public Gh2756() => InitializeComponent();
+		public Gh2756(bool useCompiledXaml)
+		{
+			//this stub will be replaced at compile time
+		}
+
+		[TestFixture]
+		class Tests
+		{
+			[SetUp] public void Setup() => Device.PlatformServices = new MockPlatformServices();
+			[TearDown] public void TearDown() => Device.PlatformServices = null;
+
+			[Test]
+			public void UnescapedBraces([Values(false, true)]bool useCompiledXaml)
+			{
+				if (useCompiledXaml)
+					Assert.Throws<XamlParseException>(() => MockCompiler.Compile(typeof(Gh2756)));
+				else
+					Assert.Throws<XamlParseException>(() => new Gh2756(useCompiledXaml));
+			}
+		}
+	}
+}
diff --git a/Xamarin.Forms.Xaml.UnitTests/Issues/Gh3539.xaml b/Xamarin.Forms.Xaml.UnitTests/Issues/Gh3539.xaml
index a6b9da7..90a0b78 100644
--- a/Xamarin.Forms.Xaml.UnitTests/Issues/Gh3539.xaml
+++ b/Xamarin.Forms.Xaml.UnitTests/Issues/Gh3539.xaml
@@ -12,11 +12,11 @@
         <StackLayout Margin="10, 0">
             <Label Text="{Binding Name}" />
             <Slider Value="{Binding Hue}" />
-            <Label Text="{Binding Hue, StringFormat='Hue = {0:F2}'}" />
+            <Label Text="{Binding Hue, StringFormat='{}Hue = {0:F2}'}" />
             <Slider Value="{Binding Saturation}" />
-            <Label Text="{Binding Saturation, StringFormat='Saturation = {0:F2}'}" />
+            <Label Text="{Binding Saturation, StringFormat='{}Saturation = {0:F2}'}" />
             <Slider Value="{Binding Luminosity}" />
-            <Label Text="{Binding Luminosity, StringFormat='Luminosity = {0:F2}'}" />
+            <Label Text="{Binding Luminosity, StringFormat='{}Luminosity = {0:F2}'}" />
         </StackLayout>
     </StackLayout> 
 </ContentPage>
diff --git a/Xamarin.Forms.Xaml.UnitTests/Issues/Issue1438.xaml b/Xamarin.Forms.Xaml.UnitTests/Issues/Issue1438.xaml
index f31c52a..4fb3672 100644
--- a/Xamarin.Forms.Xaml.UnitTests/Issues/Issue1438.xaml
+++ b/Xamarin.Forms.Xaml.UnitTests/Issues/Issue1438.xaml
@@ -1,11 +1,11 @@
-<?xml version="1.0" encoding="UTF-8"?>
+<?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.Issue1438">
 	<StackLayout>
 		<Label BindingContext="{x:Reference slider}"
 			   x:Name="label"
-			   Text="{Binding Value, StringFormat='Slider value is {0:F3}'}"
+			   Text="{Binding Value, StringFormat='{}Slider value is {0:F3}'}"
 			   Font="Large"
 			   HorizontalOptions="Center"
 			   VerticalOptions="CenterAndExpand" />
diff --git a/Xamarin.Forms.Xaml.UnitTests/Issues/Issue1549.cs b/Xamarin.Forms.Xaml.UnitTests/Issues/Issue1549.cs
index 8b5bf30..8e59a30 100644
--- a/Xamarin.Forms.Xaml.UnitTests/Issues/Issue1549.cs
+++ b/Xamarin.Forms.Xaml.UnitTests/Issues/Issue1549.cs
@@ -82,7 +82,7 @@ xmlns=""http://xamarin.com/schemas/2014/forms""
 <local:SeverityColorConverter x:Key=""SeverityColorConverter"" />
 </ResourceDictionary>
 </ContentPage.Resources>
-				<Label Text=""{Binding value, StringFormat='{0}'}"" 
+				<Label Text=""{Binding value, StringFormat='{}{0}'}"" 
 					WidthRequest=""50"" 
 					TextColor=""Black""
 					x:Name=""label""
diff --git a/Xamarin.Forms.Xaml.UnitTests/MarkupExpressionParserTests.cs b/Xamarin.Forms.Xaml.UnitTests/MarkupExpressionParserTests.cs
index 37ba170..c62523b 100644
--- a/Xamarin.Forms.Xaml.UnitTests/MarkupExpressionParserTests.cs
+++ b/Xamarin.Forms.Xaml.UnitTests/MarkupExpressionParserTests.cs
@@ -248,17 +248,25 @@ namespace Xamarin.Forms.Xaml.UnitTests
 		}
 
 		[Test]
-		public void BindingStringFormatWithoutEscaping ()
+		public void BracesNeedsToBeEscaped()
 		{
-			var bindingString = "{Binding Foo, StringFormat='{0,20}'}";
+			var bindingString = "{Binding Foo, StringFormat='Hello {0}'}";
 
-			var binding = (new MarkupExtensionParser ()).ParseExpression (ref bindingString, new Internals.XamlServiceProvider (null, null) {
+			Assert.Throws<XamlParseException>(() => new MarkupExtensionParser().ParseExpression(ref bindingString, new Internals.XamlServiceProvider(null, null)
+			{
 				IXamlTypeResolver = typeResolver,
-			});
+			}));
+		}
 
-			Assert.That (binding, Is.InstanceOf<Binding> ());
-			Assert.AreEqual ("Foo", ((Binding)binding).Path);
-			Assert.AreEqual ("{0,20}", ((Binding)binding).StringFormat);
+		[Test]
+		public void BracesNeedsToBeEscaped2()
+		{
+			var bindingString = "{Binding Foo, StringFormat='{0}'}";
+
+			Assert.Throws<XamlParseException>(() => new MarkupExtensionParser().ParseExpression(ref bindingString, new Internals.XamlServiceProvider(null, null)
+			{
+				IXamlTypeResolver = typeResolver,
+			}));
 		}
 
 		[Test]
diff --git a/Xamarin.Forms.Xaml/MarkupExpressionParser.cs b/Xamarin.Forms.Xaml/MarkupExpressionParser.cs
index 9123a72..8473627 100644
--- a/Xamarin.Forms.Xaml/MarkupExpressionParser.cs
+++ b/Xamarin.Forms.Xaml/MarkupExpressionParser.cs
@@ -46,7 +46,10 @@ namespace Xamarin.Forms.Xaml
 				return expression.Substring(2);
 
 			if (expression[expression.Length - 1] != '}')
-				throw new Exception("Expression must end with '}'");
+			{
+				var lineInfo = (serviceProvider.GetService(typeof(IXmlLineInfoProvider)) as IXmlLineInfoProvider != null) ? (serviceProvider.GetService(typeof(IXmlLineInfoProvider)) as IXmlLineInfoProvider).XmlLineInfo : new XmlLineInfo();
+				throw new XamlParseException("Expression must end with '}'", lineInfo);
+			}
 
 			int len;
 			string match;
@@ -114,7 +117,6 @@ namespace Xamarin.Forms.Xaml
 
 		protected void HandleProperty(string prop, IServiceProvider serviceProvider, ref string remaining, bool isImplicit)
 		{
-			char next;
 			object value = null;
 			string str_value;
 
@@ -137,7 +139,13 @@ namespace Xamarin.Forms.Xaml
 				str_value = value as string;
 			}
 			else
-				str_value = GetNextPiece(ref remaining, out next);
+				str_value = GetNextPiece(ref remaining, out var next);
+
+			if (str_value != null && !str_value.StartsWith("{}", StringComparison.Ordinal) && str_value.Length > 2 && str_value.IndexOf('{') != -1)
+			{
+				var lineInfo = (serviceProvider.GetService(typeof(IXmlLineInfoProvider)) as IXmlLineInfoProvider != null) ? (serviceProvider.GetService(typeof(IXmlLineInfoProvider)) as IXmlLineInfoProvider).XmlLineInfo : new XmlLineInfo();
+				throw new XamlParseException("Strings containing `{` needs to be escaped. Start the string with `{}`", lineInfo);
+			}
 
 			SetPropertyValue(prop, str_value, value, serviceProvider);
 		}
diff --git a/Xamarin.Forms.Xaml/MarkupExtensionParser.cs b/Xamarin.Forms.Xaml/MarkupExtensionParser.cs
index ca30e3f..39684b2 100644
--- a/Xamarin.Forms.Xaml/MarkupExtensionParser.cs
+++ b/Xamarin.Forms.Xaml/MarkupExtensionParser.cs
@@ -5,27 +5,31 @@ namespace Xamarin.Forms.Xaml
 {
 	internal sealed class MarkupExtensionParser : MarkupExpressionParser, IExpressionParser<object>
 	{
-		IMarkupExtension markupExtension;
+		IMarkupExtension _markupExtension;
 
 		public object Parse(string match, ref string remaining, IServiceProvider serviceProvider)
 		{
 			var typeResolver = serviceProvider.GetService(typeof (IXamlTypeResolver)) as IXamlTypeResolver;
-
-			//shortcut for Binding and StaticResource, to avoid too many reflection calls.
-			if (match == "Binding")
-				markupExtension = new BindingExtension();
-			else if (match == "TemplateBinding")
-				markupExtension = new TemplateBindingExtension();
-			else if (match == "StaticResource")
-				markupExtension = new StaticResourceExtension();
-			else if (match == "OnPlatform")
-				markupExtension = new OnPlatformExtension();
-			else if (match == "OnIdiom")
-				markupExtension = new OnIdiomExtension();
-			else if (match == "DataTemplate")
-				markupExtension = new DataTemplateExtension();
-			else
-			{
+			switch (match) {
+			case "Binding":
+				_markupExtension = new BindingExtension();
+				break;
+			case "TemplateBinding":
+				_markupExtension = new TemplateBindingExtension();
+				break;
+			case "StaticResource":
+				_markupExtension = new StaticResourceExtension();
+				break;
+			case "OnPlatform":
+				_markupExtension = new OnPlatformExtension();
+				break;
+			case "OnIdiom":
+				_markupExtension = new OnIdiomExtension();
+				break;
+			case "DataTemplate":
+				_markupExtension = new DataTemplateExtension();
+				break;
+			default:
 				if (typeResolver == null)
 					return null;
 				Type type;
@@ -33,20 +37,21 @@ namespace Xamarin.Forms.Xaml
 				//The order of lookup is to look for the Extension-suffixed class name first and then look for the class name without the Extension suffix.
 				if (!typeResolver.TryResolve(match + "Extension", out type) && !typeResolver.TryResolve(match, out type))
 					throw new XamlParseException($"MarkupExtension not found for {match}", serviceProvider);
-				markupExtension = Activator.CreateInstance(type) as IMarkupExtension;
+				_markupExtension = Activator.CreateInstance(type) as IMarkupExtension;
+				break;
 			}
 
-			if (markupExtension == null)
+			if (_markupExtension == null)
 				throw new XamlParseException($"Missing public default constructor for MarkupExtension {match}", serviceProvider);
 
 			if (remaining == "}")
-				return markupExtension.ProvideValue(serviceProvider);
+				return _markupExtension.ProvideValue(serviceProvider);
 
 			string piece;
 			while ((piece = GetNextPiece(ref remaining, out char next)) != null)
 				HandleProperty(piece, serviceProvider, ref remaining, next != '=');
 
-			return markupExtension.ProvideValue(serviceProvider);
+			return _markupExtension.ProvideValue(serviceProvider);
 		}
 
 		protected override void SetPropertyValue(string prop, string strValue, object value, IServiceProvider serviceProvider)
@@ -54,7 +59,7 @@ namespace Xamarin.Forms.Xaml
 			MethodInfo setter;
 			if (prop == null) {
 				//implicit property
-				var t = markupExtension.GetType();
+				var t = _markupExtension.GetType();
 				prop = ApplyPropertiesVisitor.GetContentPropertyName(t.GetTypeInfo());
 				if (prop == null)
 					return;
@@ -67,23 +72,24 @@ namespace Xamarin.Forms.Xaml
 			}
 			else {
 				try {
-					setter = markupExtension.GetType().GetRuntimeProperty(prop).SetMethod;
+					setter = _markupExtension.GetType().GetRuntimeProperty(prop).SetMethod;
 				}
 				catch (AmbiguousMatchException e) {
-					throw new XamlParseException($"Multiple properties with name  '{markupExtension.GetType()}.{prop}' found.", serviceProvider, innerException: e);
+					throw new XamlParseException($"Multiple properties with name  '{_markupExtension.GetType()}.{prop}' found.", serviceProvider, innerException: e);
 				}
 
 			}
 			if (value == null && strValue != null) {
 				try {
-					value = strValue.ConvertTo(markupExtension.GetType().GetRuntimeProperty(prop).PropertyType, (Func<TypeConverter>)null, serviceProvider);
+					value = strValue.ConvertTo(_markupExtension.GetType().GetRuntimeProperty(prop).PropertyType,
+						(Func<TypeConverter>)null, serviceProvider);
 				}
 				catch (AmbiguousMatchException e) {
-					throw new XamlParseException($"Multiple properties with name  '{markupExtension.GetType()}.{prop}' found.", serviceProvider, innerException: e);
+					throw new XamlParseException($"Multiple properties with name  '{_markupExtension.GetType()}.{prop}' found.", serviceProvider, innerException: e);
 				}
 			}
 
-			setter.Invoke(markupExtension, new[] { value });
+			setter.Invoke(_markupExtension, new[] { value });
 		}
 	}
-}
\ No newline at end of file
+}
-- 
2.7.4