[XamlC] test for null in Binding paths (#4521) fixes #4102

* [XamlC] test for null in Binding paths

Instead of relying on a NRE behing thrown while trying to evaluate a
compiled binding getter, detect the null value and fail faster, without
throwing. This require changing the type of the getter so they return a
(TProperty, bool) value tuple indicating the success.

- fixes #4102

* depend on ValueTuple for ns1.0
This commit is contained in:
Stephane Delcroix 2018-12-04 19:21:36 +01:00 коммит произвёл Rui Marinho
Родитель f4fa78967d
Коммит 8a758a2474
8 изменённых файлов: 173 добавлений и 76 удалений

Просмотреть файл

@ -51,5 +51,16 @@ namespace Xamarin.Forms.Build.Tasks
var t = ((GenericInstanceType)declaringTypeRef).GenericArguments[((GenericParameter)self.ReturnType).Position];
return t;
}
public static bool HasCustomAttributes(this MethodDefinition self, TypeReference attribute)
{
if (!self.HasCustomAttributes)
return false;
foreach (var arg in self.CustomAttributes) {
if (TypeRefComparer.Default.Equals(arg.AttributeType, attribute))
return true;
}
return false;
}
}
}

Просмотреть файл

@ -214,8 +214,7 @@ namespace Xamarin.Forms.Build.Tasks
public static bool TryGetPropertyName(INode node, INode parentNode, out XmlName name)
{
name = default(XmlName);
var parentElement = parentNode as IElementNode;
if (parentElement == null)
if (!(parentNode is IElementNode parentElement))
return false;
foreach (var kvp in parentElement.Properties)
{
@ -229,8 +228,7 @@ namespace Xamarin.Forms.Build.Tasks
static bool IsCollectionItem(INode node, INode parentNode)
{
var parentList = parentNode as IListNode;
if (parentList == null)
if (!(parentNode is IListNode parentList))
return false;
return parentList.CollectionItems.Contains(node);
}
@ -360,14 +358,12 @@ namespace Xamarin.Forms.Build.Tasks
//TODO support casting operators
var module = context.Module;
INode pathNode;
if (!node.Properties.TryGetValue(new XmlName("", "Path"), out pathNode) && node.CollectionItems.Any())
pathNode = node.CollectionItems [0];
if (!node.Properties.TryGetValue(new XmlName("", "Path"), out INode pathNode) && node.CollectionItems.Any())
pathNode = node.CollectionItems[0];
var path = (pathNode as ValueNode)?.Value as string;
BindingMode declaredmode;
if ( !node.Properties.TryGetValue(new XmlName("", "Mode"), out INode modeNode)
|| !Enum.TryParse((modeNode as ValueNode)?.Value as string, true, out declaredmode))
declaredmode = BindingMode.TwoWay; //meaning the mode isn't specified in the Binding extension. generate getters, setters, handlers
|| !Enum.TryParse((modeNode as ValueNode)?.Value as string, true, out BindingMode declaredmode))
declaredmode = BindingMode.TwoWay; //meaning the mode isn't specified in the Binding extension. generate getters, setters, handlers
INode dataTypeNode = null;
IElementNode n = node;
@ -376,8 +372,7 @@ namespace Xamarin.Forms.Build.Tasks
break;
n = n.Parent as IElementNode;
}
var dataType = (dataTypeNode as ValueNode)?.Value as string;
if (dataType == null)
if (!((dataTypeNode as ValueNode)?.Value is string dataType))
yield break; //throw
var prefix = dataType.Contains(":") ? dataType.Substring(0, dataType.IndexOf(":", StringComparison.Ordinal)) : "";
@ -398,17 +393,22 @@ namespace Xamarin.Forms.Build.Tasks
tPropertyRef = lastProp.property.ResolveGenericPropertyType(lastProp.propDeclTypeRef, module);
}
tPropertyRef = module.ImportReference(tPropertyRef);
var funcRef = module.ImportReference(module.ImportReference(("mscorlib", "System", "Func`2")).MakeGenericInstanceType(new [] { tSourceRef, tPropertyRef }));
var valuetupleRef = context.Module.ImportReference(module.ImportReference(("mscorlib", "System", "ValueTuple`2")).MakeGenericInstanceType(new[] { tPropertyRef, module.TypeSystem.Boolean }));
var funcRef = module.ImportReference(module.ImportReference(("mscorlib", "System", "Func`2")).MakeGenericInstanceType(new [] { tSourceRef, valuetupleRef }));
var actionRef = module.ImportReference(module.ImportReference(("mscorlib", "System", "Action`2")).MakeGenericInstanceType(new [] { tSourceRef, tPropertyRef }));
var funcObjRef = module.ImportReference(module.ImportReference(("mscorlib", "System", "Func`2")).MakeGenericInstanceType(new [] { tSourceRef, module.TypeSystem.Object }));
var tupleRef = module.ImportReference(module.ImportReference(("mscorlib", "System", "Tuple`2")).MakeGenericInstanceType(new [] { funcObjRef, module.TypeSystem.String}));
var typedBindingRef = module.ImportReference(module.ImportReference(("Xamarin.Forms.Core", "Xamarin.Forms.Internals", "TypedBinding`2")).MakeGenericInstanceType(new [] { tSourceRef, tPropertyRef}));
var ctorInfo = module.ImportReference(typedBindingRef.ResolveCached().Methods.FirstOrDefault(md => md.IsConstructor && !md.IsStatic && md.Parameters.Count == 3 ));
//FIXME: make sure the non-deprecated one is used
var ctorInfo = module.ImportReference(typedBindingRef.ResolveCached().Methods.FirstOrDefault(md =>
md.IsConstructor
&& !md.IsStatic
&& md.Parameters.Count == 3
&& !md.HasCustomAttributes (module.ImportReference(("mscorlib", "System", "ObsoleteAttribute")))));
var ctorinforef = ctorInfo.MakeGeneric(typedBindingRef, funcRef, actionRef, tupleRef);
yield return Instruction.Create(OpCodes.Ldloc, bindingExt);
yield return Create(Ldloc, bindingExt);
foreach (var instruction in CompiledBindingGetGetter(tSourceRef, tPropertyRef, properties, node, context))
yield return instruction;
if (declaredmode != BindingMode.OneTime && declaredmode != BindingMode.OneWay) { //if the mode is explicitly 1w, or 1t, no need for setters
@ -421,8 +421,8 @@ namespace Xamarin.Forms.Build.Tasks
yield return instruction;
} else
yield return Create(Ldnull);
yield return Instruction.Create(OpCodes.Newobj, module.ImportReference(ctorinforef));
yield return Instruction.Create(OpCodes.Callvirt, module.ImportPropertySetterReference(("Xamarin.Forms.Xaml", "Xamarin.Forms.Xaml", "BindingExtension"), propertyName: "TypedBinding"));
yield return Create(Newobj, module.ImportReference(ctorinforef));
yield return Create(Callvirt, module.ImportPropertySetterReference(("Xamarin.Forms.Xaml", "Xamarin.Forms.Xaml", "BindingExtension"), propertyName: "TypedBinding"));
}
static IList<(PropertyDefinition property, TypeReference propDeclTypeRef, string indexArg)> ParsePath(string path, TypeReference tSourceRef, IXmlLineInfo lineInfo, ModuleDefinition module)
@ -476,32 +476,44 @@ namespace Xamarin.Forms.Build.Tasks
static IEnumerable<Instruction> CompiledBindingGetGetter(TypeReference tSourceRef, TypeReference tPropertyRef, IList<(PropertyDefinition property, TypeReference propDeclTypeRef, string indexArg)> properties, ElementNode node, ILContext context)
{
// .method private static hidebysig default string '<Main>m__0' (class ViewModel vm) cil managed
// {
// .custom instance void class [mscorlib]System.Runtime.CompilerServices.CompilerGeneratedAttribute::'.ctor'() = (01 00 00 00 ) // ...
//
// IL_0000: ldarg.0
// IL_0001: callvirt instance class ViewModel class ViewModel::get_Model()
// IL_0006: callvirt instance string class ViewModel::get_Text()
// IL_0006: ret
// }
// .method private static hidebysig default valuetype[mscorlib] System.ValueTuple`2<string, bool> '<Main>m__0' (class ViewModel A_0) cil managed
// {
// .custom instance void class [mscorlib] System.Runtime.CompilerServices.CompilerGeneratedAttribute::'.ctor'() = (01 00 00 00 ) // ....
// IL_0000: ldarg.0
// IL_0001: dup
// IL_0002: ldnull
// IL_0003: ceq
// IL_0005: brfalse IL_0013
// IL_000a: pop
// IL_000b: ldnull
// IL_000c: ldc.i4.0
// IL_000d: newobj instance void valuetype[mscorlib]System.ValueTuple`2<string, bool>::'.ctor'(!0, !1)
// IL_0012: ret
// IL_0013: nop
// IL_0014: call instance string class ViewModel::get_Text()
// IL_0019: ldc.i4.1
// IL_001a: newobj instance void valuetype[mscorlib]System.ValueTuple`2<string, bool>::'.ctor'(!0, !1)
// IL_001f: ret
// }
var module = context.Module;
var tupleRef = module.ImportReference(module.ImportReference(("mscorlib", "System", "ValueTuple`2")).MakeGenericInstanceType(new[] { tPropertyRef, module.TypeSystem.Boolean }));
var tupleCtorRef = module.ImportCtorReference(tupleRef, 2);
tupleCtorRef = module.ImportReference(tupleCtorRef.ResolveGenericParameters(tupleRef, module));
var getter = new MethodDefinition($"<{context.Body.Method.Name}>typedBindingsM__{typedBindingCount++}",
MethodAttributes.Private | MethodAttributes.HideBySig | MethodAttributes.Static,
tPropertyRef) {
Parameters = {
new ParameterDefinition(tSourceRef)
},
CustomAttributes = {
new CustomAttribute (module.ImportCtorReference(("mscorlib", "System.Runtime.CompilerServices", "CompilerGeneratedAttribute"), parameterTypes: null))
}
tupleRef) {
Parameters = { new ParameterDefinition(tSourceRef) },
CustomAttributes = { new CustomAttribute (module.ImportCtorReference(("mscorlib", "System.Runtime.CompilerServices", "CompilerGeneratedAttribute"), parameterTypes: null)) }
};
getter.Body.InitLocals = true;
var il = getter.Body.GetILProcessor();
if (properties == null || properties.Count == 0) { //return self
il.Emit(Ldarg_0);
il.Emit(Ldc_I4_1); //true
il.Emit(Newobj, tupleCtorRef);
il.Emit(Ret);
}
else {
@ -510,39 +522,60 @@ namespace Xamarin.Forms.Build.Tasks
else
il.Emit(Ldarg_0);
foreach (var propTuple in properties) {
var property = propTuple.property;
var propDeclType = propTuple.propDeclTypeRef;
var indexerArg = propTuple.indexArg;
if (indexerArg != null) {
if (property.GetMethod.Parameters[0].ParameterType == module.TypeSystem.String)
il.Emit(Ldstr, indexerArg);
else if (property.GetMethod.Parameters[0].ParameterType == module.TypeSystem.Int32) {
if (!int.TryParse(indexerArg, out int index))
throw new XamlParseException($"Binding: {indexerArg} could not be parsed as an index for a {property.Name}", node as IXmlLineInfo);
il.Emit(Ldc_I4, index);
for (int i = 0; i < properties.Count; i++) {
(PropertyDefinition property, TypeReference propDeclTypeRef, string indexArg) = properties[i];
if (!property.PropertyType.IsValueType) { //if part of the path is null, return (default(T), false)
var nop = Create(Nop);
il.Emit(Dup);
il.Emit(Ldnull);
il.Emit(Ceq);
il.Emit(Brfalse, nop);
il.Emit(Pop);
if (tPropertyRef.IsValueType) {
var defaultValueVarDef = new VariableDefinition(tPropertyRef);
getter.Body.Variables.Add(defaultValueVarDef);
il.Emit(Ldloca_S, defaultValueVarDef);
il.Emit(Initobj, tPropertyRef);
il.Emit(Ldloc, defaultValueVarDef);
}
else
il.Emit(Ldnull);
il.Emit(Ldc_I4_0); //false
il.Emit(Newobj, tupleCtorRef);
il.Emit(Ret);
il.Append(nop);
}
var getMethod = module.ImportReference(property.GetMethod);
getMethod = module.ImportReference(getMethod.ResolveGenericParameters(propDeclType, module));
if (indexArg != null) {
if (property.GetMethod.Parameters[0].ParameterType == module.TypeSystem.String)
il.Emit(Ldstr, indexArg);
else if (property.GetMethod.Parameters[0].ParameterType == module.TypeSystem.Int32 && int.TryParse(indexArg, out int index))
il.Emit(Ldc_I4, index);
else
throw new XamlParseException($"Binding: {indexArg} could not be parsed as an index for a {property.Name}", node as IXmlLineInfo);
}
var getMethod = module.ImportReference((module.ImportReference(property.GetMethod)).ResolveGenericParameters(propDeclTypeRef, module));
if (property.GetMethod.IsVirtual)
il.Emit(Callvirt, getMethod);
else
il.Emit(Call, getMethod);
}
}
il.Emit(Ldc_I4_1); //true
il.Emit(Newobj, tupleCtorRef);
il.Emit(Ret);
}
context.Body.Method.DeclaringType.Methods.Add(getter);
// IL_0007: ldnull
// IL_0008: ldftn string class Test::'<Main>m__0'(class ViewModel)
// IL_000e: newobj instance void class [mscorlib]System.Func`2<class ViewModel, string>::'.ctor'(object, native int)
// IL_02fa: ldnull
// IL_02fb: ldftn valuetype[mscorlib]System.ValueTuple`2 <string,bool> class Test::'<Main>m__0'(class ViewModel)
// IL_0301: newobj instance void class [mscorlib] System.Func`2<class ViewModel, valuetype[mscorlib] System.ValueTuple`2<string, bool>>::'.ctor'(object, native int)
yield return Create(Ldnull);
yield return Create(Ldftn, getter);
yield return Create(Newobj, module.ImportCtorReference(("mscorlib", "System", "Func`2"), paramCount: 2, classArguments: new[] { tSourceRef, tPropertyRef }));
yield return Create(Newobj, module.ImportCtorReference(("mscorlib", "System", "Func`2"), paramCount: 2, classArguments: new[] { tSourceRef, tupleRef }));
}
static IEnumerable<Instruction> CompiledBindingGetSetter(TypeReference tSourceRef, TypeReference tPropertyRef, IList<(PropertyDefinition property, TypeReference propDeclTypeRef, string indexArg)> properties, ElementNode node, ILContext context)
@ -552,16 +585,16 @@ namespace Xamarin.Forms.Build.Tasks
yield break;
}
// .method private static hidebysig default void '<Main>m__1' (class ViewModel vm, string s) cil managed
// {
// .custom instance void class [mscorlib]System.Runtime.CompilerServices.CompilerGeneratedAttribute::'.ctor'() = (01 00 00 00 ) // ....
//
// IL_0000: ldarg.0
// IL_0001: callvirt instance class ViewModel class ViewModel::get_Model()
// IL_0006: ldarg.1
// IL_0007: callvirt instance void class ViewModel::set_Text(string)
// IL_000c: ret
// }
// .method private static hidebysig default void '<Main>m__1' (class ViewModel vm, string s) cil managed
// {
// .custom instance void class [mscorlib]System.Runtime.CompilerServices.CompilerGeneratedAttribute::'.ctor'() = (01 00 00 00 ) // ....
//
// IL_0000: ldarg.0
// IL_0001: callvirt instance class ViewModel class ViewModel::get_Model()
// IL_0006: ldarg.1
// IL_0007: callvirt instance void class ViewModel::set_Text(string)
// IL_000c: ret
// }
var module = context.Module;
var setter = new MethodDefinition($"<{context.Body.Method.Name}>typedBindingsM__{typedBindingCount++}",

Просмотреть файл

@ -51,7 +51,7 @@ namespace Xamarin.Forms.Core.UnitTests
[Test]
public void InvalidCtor()
{
Assert.Throws<ArgumentNullException>(() => new TypedBinding<MockViewModel, string>(null, (mvm, s) => mvm.Text = s, null), "Allowed null getter");
Assert.Throws<ArgumentNullException>(() => new TypedBinding<MockViewModel, string>((Func<MockViewModel, string>)null, (mvm, s) => mvm.Text = s, null), "Allowed null getter");
}
[Test, NUnit.Framework.Category("[Binding] Set Value")]

Просмотреть файл

@ -58,11 +58,19 @@ namespace Xamarin.Forms.Internals
[EditorBrowsable(EditorBrowsableState.Never)]
public sealed class TypedBinding<TSource, TProperty> : TypedBindingBase
{
readonly Func<TSource, TProperty> _getter;
readonly Func<TSource, (TProperty value, bool success)> _getter;
readonly Action<TSource, TProperty> _setter;
readonly PropertyChangedProxy [] _handlers;
[Obsolete("deprecated one. kept for backcompat")]
public TypedBinding(Func<TSource, TProperty> getter, Action<TSource, TProperty> setter, Tuple<Func<TSource, object>, string> [] handlers)
: this (s=>(getter(s), true), setter, handlers)
{
if (getter == null)
throw new ArgumentNullException(nameof(getter));
}
public TypedBinding(Func<TSource, (TProperty value, bool success)> getter, Action<TSource, TProperty> setter, Tuple<Func<TSource, object>, string>[] handlers)
{
_getter = getter ?? throw new ArgumentNullException(nameof(getter));
_setter = setter;
@ -70,9 +78,9 @@ namespace Xamarin.Forms.Internals
if (handlers == null)
return;
_handlers = new PropertyChangedProxy [handlers.Length];
_handlers = new PropertyChangedProxy[handlers.Length];
for (var i = 0; i < handlers.Length; i++)
_handlers [i] = new PropertyChangedProxy(handlers [i].Item1, handlers [i].Item2, this);
_handlers[i] = new PropertyChangedProxy(handlers[i].Item1, handlers[i].Item2, this);
}
readonly WeakReference<object> _weakSource = new WeakReference<object>(null);
@ -196,7 +204,9 @@ namespace Xamarin.Forms.Internals
var value = FallbackValue ?? property.GetDefaultValue(target);
if (isTSource) {
try {
value = GetSourceValue(_getter((TSource)sourceObject), property.ReturnType);
(var retval, bool success) = _getter((TSource)sourceObject);
if (success) //if the getter failed, return the FallbackValue
value = GetSourceValue(retval, property.ReturnType);
} catch (Exception ex) when (ex is NullReferenceException || ex is KeyNotFoundException || ex is IndexOutOfRangeException) {
}
}

Просмотреть файл

@ -18,7 +18,7 @@
<Compile Include="Internals\Legacy\**" />
<PackageReference Include="System.ComponentModel" Version="4.3.0" />
<PackageReference Include="System.Dynamic.Runtime" Version="4.3.0" />
<PackageReference Include="System.ValueTuple" Version="4.4.0" />
<PackageReference Include="System.ValueTuple" Version="4.5.0" />
</ItemGroup>
<Import Project="..\Xamarin.Flex\Xamarin.Flex.projitems" Label="Shared" Condition="Exists('..\Xamarin.Flex\Xamarin.Flex.projitems')" />
<UsingTask TaskName="XFCorePostProcessor.Tasks.FixXFCoreAssembly" AssemblyFile="..\XFCorePostProcessor.Tasks\bin\Debug\net461\XFCorePostProcessor.Tasks.dll" />

Просмотреть файл

@ -0,0 +1,9 @@
<?xml version="1.0" encoding="UTF-8"?>
<ContentPage
xmlns="http://xamarin.com/schemas/2014/forms"
xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml"
xmlns:local="using:Xamarin.Forms.Xaml.UnitTests"
x:Class="Xamarin.Forms.Xaml.UnitTests.Gh4102"
x:DataType="local:Gh4102VM">
<Label x:Name="label" Text="{Binding SomeNullValue.SomeProperty}"/>
</ContentPage>

Просмотреть файл

@ -0,0 +1,38 @@
using System;
using System.Collections.Generic;
using NUnit.Framework;
using Xamarin.Forms;
using Xamarin.Forms.Core.UnitTests;
namespace Xamarin.Forms.Xaml.UnitTests
{
public class Gh4102VM
{
public Gh4102VM SomeNullValue { get; set; }
public string SomeProperty { get; set; } = "Foo";
}
public partial class Gh4102 : ContentPage
{
public Gh4102() => InitializeComponent();
public Gh4102(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;
[TestCase(true), TestCase(false)]
public void CompiledBindingsNullInPath(bool useCompiledXaml)
{
var layout = new Gh4102(useCompiledXaml) { BindingContext = new Gh4102VM() };
Assert.That(layout.label.Text, Is.EqualTo(null));
}
}
}
}

Просмотреть файл

@ -10,14 +10,10 @@ namespace Xamarin.Forms.Xaml.UnitTests
{
public string SomeNullableValue { get; set; } = "initial";
}
public partial class Gh4103 : ContentPage
{
private string _someNullableValue = "initial";
public Gh4103()
{
InitializeComponent();
}
public Gh4103() => InitializeComponent();
public Gh4103(bool useCompiledXaml)
{