From 895b8d4e669a6995f8d4ac2834d51845896169b4 Mon Sep 17 00:00:00 2001 From: Marko Lahma Date: Sun, 25 Feb 2024 17:54:33 +0200 Subject: [PATCH] Improve interop performance against BCL list types (#1792) --- Jint.Benchmark/ListInteropBenchmark.cs | 114 +++++++ Jint/Native/Array/ArrayOperations.cs | 80 ++++- .../Specialized/ReflectionDescriptor.cs | 102 ++++--- Jint/Runtime/Interop/ObjectWrapper.cs | 56 +++- .../Reflection/ConstantValueAccessor.cs | 6 +- .../Reflection/DynamicObjectAccessor.cs | 106 ++++--- .../Reflection/ExtensionMethodCache.cs | 18 +- .../Interop/Reflection/FieldAccessor.cs | 35 ++- .../Interop/Reflection/IndexerAccessor.cs | 282 +++++++++--------- .../Interop/Reflection/MethodAccessor.cs | 38 ++- .../Interop/Reflection/NestedTypeAccessor.cs | 8 +- .../Interop/Reflection/PropertyAccessor.cs | 42 ++- .../Interop/Reflection/ReflectionAccessor.cs | 164 +++++----- Jint/Runtime/Interop/TypeReference.cs | 2 +- Jint/Runtime/Interop/TypeResolver.cs | 22 +- 15 files changed, 658 insertions(+), 417 deletions(-) create mode 100644 Jint.Benchmark/ListInteropBenchmark.cs diff --git a/Jint.Benchmark/ListInteropBenchmark.cs b/Jint.Benchmark/ListInteropBenchmark.cs new file mode 100644 index 0000000000..3ff8375089 --- /dev/null +++ b/Jint.Benchmark/ListInteropBenchmark.cs @@ -0,0 +1,114 @@ +using System.Collections; +using BenchmarkDotNet.Attributes; +using Jint.Native; +using Jint.Runtime.Interop; + +namespace Jint.Benchmark; + +[MemoryDiagnoser] +public class ListInteropBenchmark +{ + private static bool IsArrayLike(Type type) + { + if (typeof(IDictionary).IsAssignableFrom(type)) + { + return false; + } + + if (typeof(ICollection).IsAssignableFrom(type)) + { + return true; + } + + foreach (var typeInterface in type.GetInterfaces()) + { + if (!typeInterface.IsGenericType) + { + continue; + } + + if (typeInterface.GetGenericTypeDefinition() == typeof(IReadOnlyCollection<>) + || typeInterface.GetGenericTypeDefinition() == typeof(ICollection<>)) + { + return true; + } + } + + return false; + } + + private const int Count = 10_00; + private Engine _engine; + private JsValue[] _properties; + + [GlobalSetup] + public void Setup() + { + _engine = new Engine(options => + { + options + .SetWrapObjectHandler((engine, target, type) => + { + var instance = new ObjectWrapper(engine, target); + var isArrayLike = IsArrayLike(instance.Target.GetType()); + if (isArrayLike) + { + instance.Prototype = engine.Intrinsics.Array.PrototypeObject; + } + + return instance; + }) + ; + }); + + _properties = new JsValue[Count]; + var input = new List(Count); + for (var i = 0; i < Count; ++i) + { + input.Add(new Data { Category = new Category { Name = i % 2 == 0 ? "Pugal" : "Beagle" } }); + _properties[i] = JsNumber.Create(i); + } + + _engine.SetValue("input", input); + _engine.SetValue("CONST", new { category = "Pugal" }); + } + + [Benchmark] + public void Filter() + { + var value = (Data) _engine.Evaluate("input.filter(i => i.category?.name === CONST.category)[0]").ToObject(); + if (value.Category.Name != "Pugal") + { + throw new InvalidOperationException(); + } + } + + [Benchmark] + public void Indexing() + { + _engine.Evaluate("for (var i = 0; i < input.length; ++i) { input[i]; }"); + } + + [Benchmark] + public void HasProperty() + { + var input = (ObjectWrapper) _engine.GetValue("input"); + for (var i = 0; i < _properties.Length; ++i) + { + if (!input.HasProperty(_properties[i])) + { + throw new InvalidOperationException(); + } + } + } + + private class Data + { + public Category Category { get; set; } + } + + private class Category + { + public string Name { get; set; } + } +} diff --git a/Jint/Native/Array/ArrayOperations.cs b/Jint/Native/Array/ArrayOperations.cs index 57f487b4a9..b97e1aed18 100644 --- a/Jint/Native/Array/ArrayOperations.cs +++ b/Jint/Native/Array/ArrayOperations.cs @@ -3,6 +3,7 @@ using Jint.Native.Object; using Jint.Native.String; using Jint.Runtime; +using Jint.Runtime.Interop; namespace Jint.Native.Array { @@ -46,6 +47,15 @@ public static ArrayOperations For(ObjectInstance instance, bool forWrite) return new JsTypedArrayOperations(typedArrayInstance); } + if (instance is ObjectWrapper wrapper) + { + var descriptor = wrapper._typeDescriptor; + if (descriptor.IsArrayLike && wrapper.Target is ICollection) + { + return new IndexWrappedOperations(wrapper); + } + } + return new ObjectOperations(instance); } @@ -121,7 +131,9 @@ public JsValue Current { return !_initialized ? JsValue.Undefined - : _obj.TryGetValue(_current, out var temp) ? temp : JsValue.Undefined; + : _obj.TryGetValue(_current, out var temp) + ? temp + : JsValue.Undefined; } } @@ -356,7 +368,7 @@ public override void DeletePropertyOrThrow(ulong index) public override bool HasProperty(ulong index) => _target.HasProperty(index); } - private sealed class JsStringOperations : ArrayOperations + private sealed class JsStringOperations : ArrayOperations { private readonly Realm _realm; private readonly JsString _target; @@ -459,6 +471,70 @@ public override bool TryGetValue(ulong index, out JsValue value) public override void DeletePropertyOrThrow(ulong index) => throw new NotSupportedException(); } + + private sealed class IndexWrappedOperations : ArrayOperations + { + private readonly ObjectWrapper _target; + private readonly ICollection _collection; + private readonly IList _list; + + public IndexWrappedOperations(ObjectWrapper wrapper) + { + _target = wrapper; + _collection = (ICollection) wrapper.Target; + _list = (IList) wrapper.Target; + } + + public override ObjectInstance Target => _target; + + public override ulong GetSmallestIndex(ulong length) => 0; + + public override uint GetLength() => (uint) _collection.Count; + + public override ulong GetLongLength() => GetLength(); + + public override void SetLength(ulong length) => throw new NotSupportedException(); + + public override void EnsureCapacity(ulong capacity) + { + } + + public override JsValue Get(ulong index) => index < (ulong) _collection.Count ? ReadValue((int) index) : JsValue.Undefined; + + public override bool TryGetValue(ulong index, out JsValue value) + { + if (index < (ulong) _collection.Count) + { + value = ReadValue((int) index); + return true; + } + + value = JsValue.Undefined; + return false; + } + + private JsValue ReadValue(int index) + { + if (_list is not null) + { + return (uint) index < _list.Count ? JsValue.FromObject(_target.Engine, _list[index]) : JsValue.Undefined; + } + + // via reflection is slow, but better than nothing + return JsValue.FromObject(_target.Engine, _target._typeDescriptor.IntegerIndexerProperty!.GetValue(Target, [index])); + } + + public override bool HasProperty(ulong index) => index < (ulong) _collection.Count; + + public override void CreateDataPropertyOrThrow(ulong index, JsValue value) + => _target.CreateDataPropertyOrThrow(index, value); + + public override void Set(ulong index, JsValue value, bool updateLength = false, bool throwOnError = true) + => _target[(int) index] = value; + + public override void DeletePropertyOrThrow(ulong index) + => _target.DeletePropertyOrThrow(index); + } } /// diff --git a/Jint/Runtime/Descriptors/Specialized/ReflectionDescriptor.cs b/Jint/Runtime/Descriptors/Specialized/ReflectionDescriptor.cs index 87c8abaaff..473e2c5e24 100644 --- a/Jint/Runtime/Descriptors/Specialized/ReflectionDescriptor.cs +++ b/Jint/Runtime/Descriptors/Specialized/ReflectionDescriptor.cs @@ -3,63 +3,81 @@ using Jint.Runtime.Interop; using Jint.Runtime.Interop.Reflection; -namespace Jint.Runtime.Descriptors.Specialized +namespace Jint.Runtime.Descriptors.Specialized; + +internal sealed class ReflectionDescriptor : PropertyDescriptor { - internal sealed class ReflectionDescriptor : PropertyDescriptor + private readonly Engine _engine; + private readonly ReflectionAccessor _reflectionAccessor; + private readonly object _target; + private readonly string _propertyName; + + private JsValue? _get; + private JsValue? _set; + + public ReflectionDescriptor( + Engine engine, + ReflectionAccessor reflectionAccessor, + object target, + string propertyName, + bool enumerable) + : base((enumerable ? PropertyFlag.Enumerable : PropertyFlag.None) | PropertyFlag.CustomJsValue) { - private readonly Engine _engine; - private readonly ReflectionAccessor _reflectionAccessor; - private readonly object _target; + _flags |= PropertyFlag.NonData; + _engine = engine; + _reflectionAccessor = reflectionAccessor; + _target = target; + _propertyName = propertyName; + } - public ReflectionDescriptor( - Engine engine, - ReflectionAccessor reflectionAccessor, - object target, - bool enumerable) - : base((enumerable ? PropertyFlag.Enumerable : PropertyFlag.None) | PropertyFlag.CustomJsValue) + public override JsValue? Get + { + get { - _flags |= PropertyFlag.NonData; - _engine = engine; - _reflectionAccessor = reflectionAccessor; - _target = target; - - if (reflectionAccessor.Writable && engine.Options.Interop.AllowWrite) + if (_reflectionAccessor.Readable) { - Set = new SetterFunction(_engine, DoSet); + return _get ??= new GetterFunction(_engine, DoGet); } - if (reflectionAccessor.Readable) + + return null; + } + } + + public override JsValue? Set + { + get + { + if (_reflectionAccessor.Writable && _engine.Options.Interop.AllowWrite) { - Get = new GetterFunction(_engine, DoGet); + return _set ??= new SetterFunction(_engine, DoSet); } - } - public override JsValue? Get { get; } - public override JsValue? Set { get; } + return null; + } + } + protected internal override JsValue? CustomValue + { + get => DoGet(thisObj: null); + set => DoSet(thisObj: null, value); + } - protected internal override JsValue? CustomValue - { - get => DoGet(null); - set => DoSet(null, value); - } + private JsValue DoGet(JsValue? thisObj) + { + var value = _reflectionAccessor.GetValue(_engine, _target, _propertyName); + var type = _reflectionAccessor.MemberType; + return JsValue.FromObjectWithType(_engine, value, type); + } - private JsValue DoGet(JsValue? thisObj) + private void DoSet(JsValue? thisObj, JsValue? v) + { + try { - var value = _reflectionAccessor.GetValue(_engine, _target); - var type = _reflectionAccessor.MemberType; - return JsValue.FromObjectWithType(_engine, value, type); + _reflectionAccessor.SetValue(_engine, _target, _propertyName, v!); } - - private void DoSet(JsValue? thisObj, JsValue? v) + catch (TargetInvocationException exception) { - try - { - _reflectionAccessor.SetValue(_engine, _target, v!); - } - catch (TargetInvocationException exception) - { - ExceptionHelper.ThrowMeaningfulException(_engine, exception); - } + ExceptionHelper.ThrowMeaningfulException(_engine, exception); } } } diff --git a/Jint/Runtime/Interop/ObjectWrapper.cs b/Jint/Runtime/Interop/ObjectWrapper.cs index 6ff7657910..73a76d58f3 100644 --- a/Jint/Runtime/Interop/ObjectWrapper.cs +++ b/Jint/Runtime/Interop/ObjectWrapper.cs @@ -19,7 +19,7 @@ namespace Jint.Runtime.Interop /// public sealed class ObjectWrapper : ObjectInstance, IObjectWrapper, IEquatable { - private readonly TypeDescriptor _typeDescriptor; + internal readonly TypeDescriptor _typeDescriptor; public ObjectWrapper( Engine engine, @@ -72,7 +72,7 @@ public override bool Set(JsValue property, JsValue value, JsValue receiver) return false; } - accessor.SetValue(_engine, Target, value); + accessor.SetValue(_engine, Target, member, value); return true; } } @@ -115,12 +115,52 @@ public override void RemoveOwnProperty(JsValue property) } } + public override bool HasProperty(JsValue property) + { + if (property.IsNumber()) + { + var value = ((JsNumber) property)._value; + if (TypeConverter.IsIntegralNumber(value)) + { + var index = (int) value; + if (Target is ICollection collection && index < collection.Count) + { + return true; + } + } + } + + return base.HasProperty(property); + } + public override JsValue Get(JsValue property, JsValue receiver) { - if (property.IsInteger() && Target is IList list) + if (property.IsInteger()) { var index = (int) ((JsNumber) property)._value; - return (uint) index < list.Count ? FromObject(_engine, list[index]) : Undefined; + if (Target is IList list) + { + return (uint) index < list.Count ? FromObject(_engine, list[index]) : Undefined; + } + + if (Target is ICollection collection + && _typeDescriptor.IntegerIndexerProperty is not null) + { + // via reflection is slow, but better than nothing + if (index < collection.Count) + { + return FromObject(_engine, _typeDescriptor.IntegerIndexerProperty.GetValue(Target, [index])); + } + + return Undefined; + } + } + + if (!_typeDescriptor.IsDictionary + && Target is ICollection c + && CommonProperties.Length.Equals(property)) + { + return JsNumber.Create(c.Count); } var desc = GetOwnProperty(property, mustBeReadable: true, mustBeWritable: false); @@ -253,7 +293,7 @@ private PropertyDescriptor GetOwnProperty(JsValue property, bool mustBeReadable, } var accessor = _engine.Options.Interop.TypeResolver.GetAccessor(_engine, ClrType, member, mustBeReadable, mustBeWritable); - var descriptor = accessor.CreatePropertyDescriptor(_engine, Target, enumerable: !isDictionary); + var descriptor = accessor.CreatePropertyDescriptor(_engine, Target, member, enumerable: !isDictionary); if (!isDictionary && !ReferenceEquals(descriptor, PropertyDescriptor.Undefined) && (!mustBeReadable || accessor.Readable) @@ -274,15 +314,15 @@ public static PropertyDescriptor GetPropertyDescriptor(Engine engine, object tar { return member switch { - PropertyInfo pi => new PropertyAccessor(pi.Name, pi), - MethodBase mb => new MethodAccessor(target.GetType(), member.Name, MethodDescriptor.Build(new[] { mb })), + PropertyInfo pi => new PropertyAccessor(pi), + MethodBase mb => new MethodAccessor(target.GetType(), MethodDescriptor.Build(new[] { mb })), FieldInfo fi => new FieldAccessor(fi), _ => null }; } var accessor = engine.Options.Interop.TypeResolver.GetAccessor(engine, target.GetType(), member.Name, mustBeReadable: false, mustBeWritable: false, Factory); - return accessor.CreatePropertyDescriptor(engine, target); + return accessor.CreatePropertyDescriptor(engine, target, member.Name); } internal static Type GetClrType(object obj, Type? type) diff --git a/Jint/Runtime/Interop/Reflection/ConstantValueAccessor.cs b/Jint/Runtime/Interop/Reflection/ConstantValueAccessor.cs index d6d60d785b..86c46666e9 100644 --- a/Jint/Runtime/Interop/Reflection/ConstantValueAccessor.cs +++ b/Jint/Runtime/Interop/Reflection/ConstantValueAccessor.cs @@ -16,17 +16,17 @@ public ConstantValueAccessor(JsValue? value) : base(null!, null!) protected override JsValue? ConstantValue { get; } - protected override object? DoGetValue(object target) + protected override object? DoGetValue(object target, string memberName) { return ConstantValue; } - protected override void DoSetValue(object target, object? value) + protected override void DoSetValue(object target, string memberName, object? value) { throw new InvalidOperationException(); } - public override PropertyDescriptor CreatePropertyDescriptor(Engine engine, object target, bool enumerable = true) + public override PropertyDescriptor CreatePropertyDescriptor(Engine engine, object target, string memberName, bool enumerable = true) { return ConstantValue is null ? PropertyDescriptor.Undefined diff --git a/Jint/Runtime/Interop/Reflection/DynamicObjectAccessor.cs b/Jint/Runtime/Interop/Reflection/DynamicObjectAccessor.cs index 3b0f6cbe45..2c445d7f16 100644 --- a/Jint/Runtime/Interop/Reflection/DynamicObjectAccessor.cs +++ b/Jint/Runtime/Interop/Reflection/DynamicObjectAccessor.cs @@ -4,78 +4,74 @@ #pragma warning disable IL2092 -namespace Jint.Runtime.Interop.Reflection +namespace Jint.Runtime.Interop.Reflection; + +internal sealed class DynamicObjectAccessor : ReflectionAccessor { - internal sealed class DynamicObjectAccessor : ReflectionAccessor + private JintSetMemberBinder? _setter; + private JintGetMemberBinder? _getter; + + public DynamicObjectAccessor( + Type memberType, + PropertyInfo? indexer = null) : base(memberType, indexer) { - private readonly string _memberName; - private JintSetMemberBinder? _setter; - private JintGetMemberBinder? _getter; + } - public DynamicObjectAccessor( - Type memberType, - string memberName, - PropertyInfo? indexer = null) : base(memberType, memberName, indexer) - { - _memberName = memberName; - } + public override bool Writable => true; - public override bool Writable => true; + protected override object? DoGetValue(object target, string memberName) + { + var dynamicObject = (DynamicObject) target; + var getter = _getter ??= new JintGetMemberBinder(memberName, ignoreCase: true); + dynamicObject.TryGetMember(getter, out var result); + return result; + } + + protected override void DoSetValue(object target, string memberName, object? value) + { + var dynamicObject = (DynamicObject) target; + var setter = _setter ??= new JintSetMemberBinder(memberName, ignoreCase: true); + dynamicObject.TrySetMember(setter, value); + } - protected override object? DoGetValue(object target) + protected override object? ConvertValueToSet(Engine engine, object value) + { + // we expect value to be generally CLR type, convert when possible + return value switch { - var dynamicObject = (DynamicObject) target; - var getter = _getter ??= new JintGetMemberBinder(_memberName, ignoreCase: true); - dynamicObject.TryGetMember(getter, out var result); - return result; - } + JsBoolean jsBoolean => jsBoolean._value ? JsBoolean.BoxedTrue : JsBoolean.BoxedFalse, + JsString jsString => jsString.ToString(), + JsNumber jsNumber => jsNumber._value, + JsNull => null, + JsUndefined => null, + _ => value + }; + } - protected override void DoSetValue(object target, object? value) + private sealed class JintGetMemberBinder : GetMemberBinder + { + public JintGetMemberBinder(string name, bool ignoreCase) : base(name, ignoreCase) { - var dynamicObject = (DynamicObject) target; - var setter = _setter ??= new JintSetMemberBinder(_memberName, ignoreCase: true); - dynamicObject.TrySetMember(setter, value); } - protected override object? ConvertValueToSet(Engine engine, object value) + public override DynamicMetaObject FallbackGetMember(DynamicMetaObject target, DynamicMetaObject? errorSuggestion) { - // we expect value to be generally CLR type, convert when possible - return value switch - { - JsBoolean jsBoolean => jsBoolean._value ? JsBoolean.BoxedTrue : JsBoolean.BoxedFalse, - JsString jsString => jsString.ToString(), - JsNumber jsNumber => jsNumber._value, - JsNull => null, - JsUndefined => null, - _ => value - }; + throw new NotImplementedException(nameof(FallbackGetMember) + " not implemented"); } + } - private sealed class JintGetMemberBinder : GetMemberBinder + private sealed class JintSetMemberBinder : SetMemberBinder + { + public JintSetMemberBinder(string name, bool ignoreCase) : base(name, ignoreCase) { - public JintGetMemberBinder(string name, bool ignoreCase) : base(name, ignoreCase) - { - } - - public override DynamicMetaObject FallbackGetMember(DynamicMetaObject target, DynamicMetaObject? errorSuggestion) - { - throw new NotImplementedException(nameof(FallbackGetMember) + " not implemented"); - } } - private sealed class JintSetMemberBinder : SetMemberBinder + public override DynamicMetaObject FallbackSetMember( + DynamicMetaObject target, + DynamicMetaObject value, + DynamicMetaObject? errorSuggestion) { - public JintSetMemberBinder(string name, bool ignoreCase) : base(name, ignoreCase) - { - } - - public override DynamicMetaObject FallbackSetMember( - DynamicMetaObject target, - DynamicMetaObject value, - DynamicMetaObject? errorSuggestion) - { - throw new NotImplementedException(nameof(FallbackSetMember) + " not implemented"); - } + throw new NotImplementedException(nameof(FallbackSetMember) + " not implemented"); } } } diff --git a/Jint/Runtime/Interop/Reflection/ExtensionMethodCache.cs b/Jint/Runtime/Interop/Reflection/ExtensionMethodCache.cs index ea185ea39f..abecc8d588 100644 --- a/Jint/Runtime/Interop/Reflection/ExtensionMethodCache.cs +++ b/Jint/Runtime/Interop/Reflection/ExtensionMethodCache.cs @@ -36,8 +36,7 @@ internal static ExtensionMethodCache Build(List extensionMethodContainerTy Type GetTypeDefinition(Type type) { - return type.IsConstructedGenericType && type.GenericTypeArguments.Any(x => x.IsGenericParameter) ? - type.GetGenericTypeDefinition() : type; + return type.IsConstructedGenericType && type.GenericTypeArguments.Any(x => x.IsGenericParameter) ? type.GetGenericTypeDefinition() : type; } var methodsByTarget = extensionMethodContainerTypes @@ -50,8 +49,14 @@ Type GetTypeDefinition(Type type) public bool HasMethods => _allExtensionMethods.Count > 0; - public bool TryGetExtensionMethods(Type objectType, [NotNullWhen((true))] out MethodInfo[]? methods) + public bool TryGetExtensionMethods(Type objectType, [NotNullWhen(true)] out MethodInfo[]? methods) { + if (_allExtensionMethods.Count == 0) + { + methods = Array.Empty(); + return false; + } + var methodLookup = _extensionMethods; if (methodLookup.TryGetValue(objectType, out methods)) @@ -73,14 +78,11 @@ public bool TryGetExtensionMethods(Type objectType, [NotNullWhen((true))] out Me } } - // don't create generic methods bound to an array of object - as this will prevent value types and other generics that don't support covariants/contravariants + // don't create generic methods bound to an array of object - as this will prevent value types and other generics that don't support covariants/contravariants methods = results.ToArray(); // racy, we don't care, worst case we'll catch up later - Interlocked.CompareExchange(ref _extensionMethods, new Dictionary(methodLookup) - { - [objectType] = methods - }, methodLookup); + Interlocked.CompareExchange(ref _extensionMethods, new Dictionary(methodLookup) { [objectType] = methods }, methodLookup); return methods.Length > 0; } diff --git a/Jint/Runtime/Interop/Reflection/FieldAccessor.cs b/Jint/Runtime/Interop/Reflection/FieldAccessor.cs index 721ba960c2..3bc9f85a24 100644 --- a/Jint/Runtime/Interop/Reflection/FieldAccessor.cs +++ b/Jint/Runtime/Interop/Reflection/FieldAccessor.cs @@ -1,27 +1,26 @@ using System.Reflection; -namespace Jint.Runtime.Interop.Reflection +namespace Jint.Runtime.Interop.Reflection; + +internal sealed class FieldAccessor : ReflectionAccessor { - internal sealed class FieldAccessor : ReflectionAccessor - { - private readonly FieldInfo _fieldInfo; + private readonly FieldInfo _fieldInfo; - public FieldAccessor(FieldInfo fieldInfo, string? memberName = null, PropertyInfo? indexer = null) - : base(fieldInfo.FieldType, memberName, indexer) - { - _fieldInfo = fieldInfo; - } + public FieldAccessor(FieldInfo fieldInfo, PropertyInfo? indexer = null) + : base(fieldInfo.FieldType, indexer) + { + _fieldInfo = fieldInfo; + } - public override bool Writable => !_fieldInfo.Attributes.HasFlag(FieldAttributes.InitOnly); + public override bool Writable => !_fieldInfo.Attributes.HasFlag(FieldAttributes.InitOnly); - protected override object? DoGetValue(object target) - { - return _fieldInfo.GetValue(target); - } + protected override object? DoGetValue(object target, string memberName) + { + return _fieldInfo.GetValue(target); + } - protected override void DoSetValue(object target, object? value) - { - _fieldInfo.SetValue(target, value); - } + protected override void DoSetValue(object target, string memberName, object? value) + { + _fieldInfo.SetValue(target, value); } } diff --git a/Jint/Runtime/Interop/Reflection/IndexerAccessor.cs b/Jint/Runtime/Interop/Reflection/IndexerAccessor.cs index ddce54617b..5a86c5bf1a 100644 --- a/Jint/Runtime/Interop/Reflection/IndexerAccessor.cs +++ b/Jint/Runtime/Interop/Reflection/IndexerAccessor.cs @@ -8,198 +8,196 @@ #pragma warning disable IL2067 -namespace Jint.Runtime.Interop.Reflection +namespace Jint.Runtime.Interop.Reflection; + +internal sealed class IndexerAccessor : ReflectionAccessor { - internal sealed class IndexerAccessor : ReflectionAccessor + private readonly object _key; + + internal readonly PropertyInfo _indexer; + private readonly MethodInfo? _getter; + private readonly MethodInfo? _setter; + private readonly MethodInfo? _containsKey; + + private IndexerAccessor(PropertyInfo indexer, MethodInfo? containsKey, object key) : base(indexer.PropertyType) { - private readonly object _key; + _indexer = indexer; + _containsKey = containsKey; + _key = key; - internal readonly PropertyInfo _indexer; - private readonly MethodInfo? _getter; - private readonly MethodInfo? _setter; - private readonly MethodInfo? _containsKey; + _getter = indexer.GetGetMethod(); + _setter = indexer.GetSetMethod(); + } - private IndexerAccessor(PropertyInfo indexer, MethodInfo? containsKey, object key) - : base(indexer.PropertyType, key) - { - _indexer = indexer; - _containsKey = containsKey; - _key = key; + internal static bool TryFindIndexer( + Engine engine, + [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods | DynamicallyAccessedMemberTypes.PublicProperties)] Type targetType, + string propertyName, + [NotNullWhen(true)] out IndexerAccessor? indexerAccessor, + [NotNullWhen(true)] out PropertyInfo? indexer) + { + indexerAccessor = null; + indexer = null; + var paramTypeArray = new Type[1]; - _getter = indexer.GetGetMethod(); - _setter = indexer.GetSetMethod(); - } + // integer keys can be ambiguous as we only know string keys + int? integerKey = null; - internal static bool TryFindIndexer( - Engine engine, - [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods | DynamicallyAccessedMemberTypes.PublicProperties)] Type targetType, - string propertyName, - [NotNullWhen(true)] out IndexerAccessor? indexerAccessor, - [NotNullWhen(true)] out PropertyInfo? indexer) + if (int.TryParse(propertyName, NumberStyles.Integer, CultureInfo.InvariantCulture, out var intKeyTemp)) { - indexerAccessor = null; - indexer = null; - var paramTypeArray = new Type[1]; - - // integer keys can be ambiguous as we only know string keys - int? integerKey = null; + integerKey = intKeyTemp; + } - if (int.TryParse(propertyName, NumberStyles.Integer, CultureInfo.InvariantCulture, out var intKeyTemp)) + IndexerAccessor? ComposeIndexerFactory(PropertyInfo candidate, Type paramType) + { + object? key = null; + // int key is quite common case + if (paramType == typeof(int) && integerKey is not null) { - integerKey = intKeyTemp; + key = integerKey; + } + else + { + engine.TypeConverter.TryConvert(propertyName, paramType, CultureInfo.InvariantCulture, out key); } - IndexerAccessor? ComposeIndexerFactory(PropertyInfo candidate, Type paramType) + if (key is not null) { - object? key = null; - // int key is quite common case - if (paramType == typeof(int) && integerKey is not null) + // the key can be converted for this indexer + var indexerProperty = candidate; + // get contains key method to avoid index exception being thrown in dictionaries + paramTypeArray[0] = paramType; + var containsKeyMethod = targetType.GetMethod(nameof(IDictionary.ContainsKey), paramTypeArray); + if (containsKeyMethod is null && targetType.IsAssignableFrom(typeof(IDictionary))) { - key = integerKey; - } - else - { - engine.TypeConverter.TryConvert(propertyName, paramType, CultureInfo.InvariantCulture, out key); + paramTypeArray[0] = typeof(object); + containsKeyMethod = targetType.GetMethod(nameof(IDictionary.Contains), paramTypeArray); } - if (key is not null) - { - // the key can be converted for this indexer - var indexerProperty = candidate; - // get contains key method to avoid index exception being thrown in dictionaries - paramTypeArray[0] = paramType; - var containsKeyMethod = targetType.GetMethod(nameof(IDictionary.ContainsKey), paramTypeArray); - if (containsKeyMethod is null && targetType.IsAssignableFrom(typeof(IDictionary))) - { - paramTypeArray[0] = typeof(object); - containsKeyMethod = targetType.GetMethod(nameof(IDictionary.Contains), paramTypeArray); - } + return new IndexerAccessor(indexerProperty, containsKeyMethod, key); + } - return new IndexerAccessor(indexerProperty, containsKeyMethod, key); - } + // the key type doesn't work for this indexer + return null; + } - // the key type doesn't work for this indexer - return null; - } + var filter = new Func(m => engine.Options.Interop.TypeResolver.Filter(engine, m)); - var filter = new Func(m => engine.Options.Interop.TypeResolver.Filter(engine, m)); + // default indexer wins + var descriptor = TypeDescriptor.Get(targetType); + if (descriptor.IntegerIndexerProperty is not null && filter(descriptor.IntegerIndexerProperty)) + { + indexerAccessor = ComposeIndexerFactory(descriptor.IntegerIndexerProperty, typeof(int)); + if (indexerAccessor != null) + { + indexer = descriptor.IntegerIndexerProperty; + return true; + } + } - // default indexer wins - var descriptor = TypeDescriptor.Get(targetType); - if (descriptor.IntegerIndexerProperty is not null && filter(descriptor.IntegerIndexerProperty)) + // try to find first indexer having either public getter or setter with matching argument type + PropertyInfo? fallbackIndexer = null; + foreach (var candidate in targetType.GetProperties()) + { + if (!filter(candidate)) { - indexerAccessor = ComposeIndexerFactory(descriptor.IntegerIndexerProperty, typeof(int)); - if (indexerAccessor != null) - { - indexer = descriptor.IntegerIndexerProperty; - return true; - } + continue; } - // try to find first indexer having either public getter or setter with matching argument type - PropertyInfo? fallbackIndexer = null; - foreach (var candidate in targetType.GetProperties()) + var indexParameters = candidate.GetIndexParameters(); + if (indexParameters.Length != 1) { - if (!filter(candidate)) - { - continue; - } + continue; + } - var indexParameters = candidate.GetIndexParameters(); - if (indexParameters.Length != 1) + if (candidate.GetGetMethod() != null || candidate.GetSetMethod() != null) + { + var paramType = indexParameters[0].ParameterType; + indexerAccessor = ComposeIndexerFactory(candidate, paramType); + if (indexerAccessor != null) { - continue; - } + if (paramType != typeof(string) || integerKey is null) + { + // exact match, we don't need to check for integer key + indexer = candidate; + return true; + } - if (candidate.GetGetMethod() != null || candidate.GetSetMethod() != null) - { - var paramType = indexParameters[0].ParameterType; - indexerAccessor = ComposeIndexerFactory(candidate, paramType); - if (indexerAccessor != null) + if (fallbackIndexer is null) { - if (paramType != typeof(string) || integerKey is null) - { - // exact match, we don't need to check for integer key - indexer = candidate; - return true; - } - - if (fallbackIndexer is null) - { - // our fallback - fallbackIndexer = candidate; - } + // our fallback + fallbackIndexer = candidate; } } } + } - if (fallbackIndexer is not null) - { - indexer = fallbackIndexer; - // just to keep compiler happy, we know we have a value - indexerAccessor = indexerAccessor ?? new IndexerAccessor(indexer, null, null!); - return true; - } - - indexerAccessor = default; - indexer = default; - return false; + if (fallbackIndexer is not null) + { + indexer = fallbackIndexer; + // just to keep compiler happy, we know we have a value + indexerAccessor ??= new IndexerAccessor(indexer, containsKey: null, key: null!); + return true; } - public override bool Readable => _indexer.CanRead; + indexerAccessor = default; + indexer = default; + return false; + } - public override bool Writable => _indexer.CanWrite; + public override bool Readable => _indexer.CanRead; - protected override object? DoGetValue(object target) - { - if (_getter is null) - { - ExceptionHelper.ThrowInvalidOperationException("Indexer has no public getter."); - return null; - } + public override bool Writable => _indexer.CanWrite; - object[] parameters = { _key }; + protected override object? DoGetValue(object target, string memberName) + { + if (_getter is null) + { + ExceptionHelper.ThrowInvalidOperationException("Indexer has no public getter."); + return null; + } - if (_containsKey != null) - { - if (_containsKey.Invoke(target, parameters) as bool? != true) - { - return JsValue.Undefined; - } - } + object[] parameters = { _key }; - try - { - return _getter.Invoke(target, parameters); - } - catch (TargetInvocationException tie) when (tie.InnerException is KeyNotFoundException) + if (_containsKey != null) + { + if (_containsKey.Invoke(target, parameters) as bool? != true) { return JsValue.Undefined; } } - protected override void DoSetValue(object target, object? value) + try { - if (_setter is null) - { - ExceptionHelper.ThrowInvalidOperationException("Indexer has no public setter."); - } + return _getter.Invoke(target, parameters); + } + catch (TargetInvocationException tie) when (tie.InnerException is KeyNotFoundException) + { + return JsValue.Undefined; + } + } - object?[] parameters = { _key, value }; - _setter.Invoke(target, parameters); + protected override void DoSetValue(object target, string memberName, object? value) + { + if (_setter is null) + { + ExceptionHelper.ThrowInvalidOperationException("Indexer has no public setter."); } - public override PropertyDescriptor CreatePropertyDescriptor(Engine engine, object target, bool enumerable = true) + object?[] parameters = { _key, value }; + _setter.Invoke(target, parameters); + } + + public override PropertyDescriptor CreatePropertyDescriptor(Engine engine, object target, string memberName, bool enumerable = true) + { + if (_containsKey != null) { - if (_containsKey != null) + if (_containsKey.Invoke(target, [_key]) as bool? != true) { - if (_containsKey.Invoke(target, new[] { _key }) as bool? != true) - { - return PropertyDescriptor.Undefined; - } + return PropertyDescriptor.Undefined; } - - return new ReflectionDescriptor(engine, this, target, enumerable: true); } + + return new ReflectionDescriptor(engine, this, target, memberName, enumerable: true); } } diff --git a/Jint/Runtime/Interop/Reflection/MethodAccessor.cs b/Jint/Runtime/Interop/Reflection/MethodAccessor.cs index 58163bb573..a53acc93c9 100644 --- a/Jint/Runtime/Interop/Reflection/MethodAccessor.cs +++ b/Jint/Runtime/Interop/Reflection/MethodAccessor.cs @@ -1,30 +1,28 @@ using Jint.Runtime.Descriptors; -namespace Jint.Runtime.Interop.Reflection +namespace Jint.Runtime.Interop.Reflection; + +internal sealed class MethodAccessor : ReflectionAccessor { - internal sealed class MethodAccessor : ReflectionAccessor - { - private readonly Type _targetType; - private readonly string _name; - private readonly MethodDescriptor[] _methods; + private readonly Type _targetType; + private readonly MethodDescriptor[] _methods; - public MethodAccessor(Type targetType, string name, MethodDescriptor[] methods) - : base(null!, name) - { - _targetType = targetType; - _name = name; - _methods = methods; - } + public MethodAccessor(Type targetType, MethodDescriptor[] methods) : base(null!) + { + _targetType = targetType; + _methods = methods; + } - public override bool Writable => false; + public override bool Writable => false; - protected override object? DoGetValue(object target) => null; + protected override object? DoGetValue(object target, string memberName) => null; - protected override void DoSetValue(object target, object? value) { } + protected override void DoSetValue(object target, string memberName, object? value) + { + } - public override PropertyDescriptor CreatePropertyDescriptor(Engine engine, object target, bool enumerable = true) - { - return new(new MethodInfoFunction(engine, _targetType, target, _name, _methods), PropertyFlag.AllForbidden); - } + public override PropertyDescriptor CreatePropertyDescriptor(Engine engine, object target, string memberName, bool enumerable = true) + { + return new(new MethodInfoFunction(engine, _targetType, target, memberName, _methods), PropertyFlag.AllForbidden); } } diff --git a/Jint/Runtime/Interop/Reflection/NestedTypeAccessor.cs b/Jint/Runtime/Interop/Reflection/NestedTypeAccessor.cs index 6d68e40083..b067f5ff21 100644 --- a/Jint/Runtime/Interop/Reflection/NestedTypeAccessor.cs +++ b/Jint/Runtime/Interop/Reflection/NestedTypeAccessor.cs @@ -6,18 +6,18 @@ internal sealed class NestedTypeAccessor : ReflectionAccessor { private readonly TypeReference _typeReference; - public NestedTypeAccessor(TypeReference typeReference, string name) : base(typeof(Type), name) + public NestedTypeAccessor(TypeReference typeReference) : base(typeof(Type)) { _typeReference = typeReference; } public override bool Writable => false; - protected override object? DoGetValue(object target) => null; + protected override object? DoGetValue(object target, string memberName) => null; - protected override void DoSetValue(object target, object? value) { } + protected override void DoSetValue(object target, string memberName, object? value) { } - public override PropertyDescriptor CreatePropertyDescriptor(Engine engine, object target, bool enumerable = true) + public override PropertyDescriptor CreatePropertyDescriptor(Engine engine, object target, string memberName, bool enumerable = true) { return new(_typeReference, PropertyFlag.AllForbidden); } diff --git a/Jint/Runtime/Interop/Reflection/PropertyAccessor.cs b/Jint/Runtime/Interop/Reflection/PropertyAccessor.cs index 9f33768947..6881509a29 100644 --- a/Jint/Runtime/Interop/Reflection/PropertyAccessor.cs +++ b/Jint/Runtime/Interop/Reflection/PropertyAccessor.cs @@ -1,32 +1,30 @@ using System.Reflection; -namespace Jint.Runtime.Interop.Reflection +namespace Jint.Runtime.Interop.Reflection; + +internal sealed class PropertyAccessor : ReflectionAccessor { - internal sealed class PropertyAccessor : ReflectionAccessor - { - private readonly PropertyInfo _propertyInfo; + private readonly PropertyInfo _propertyInfo; - public PropertyAccessor( - string memberName, - PropertyInfo propertyInfo, - PropertyInfo? indexerToTry = null) - : base(propertyInfo.PropertyType, memberName, indexerToTry) - { - _propertyInfo = propertyInfo; - } + public PropertyAccessor( + PropertyInfo propertyInfo, + PropertyInfo? indexerToTry = null) + : base(propertyInfo.PropertyType, indexerToTry) + { + _propertyInfo = propertyInfo; + } - public override bool Readable => _propertyInfo.CanRead; + public override bool Readable => _propertyInfo.CanRead; - public override bool Writable => _propertyInfo.CanWrite; + public override bool Writable => _propertyInfo.CanWrite; - protected override object? DoGetValue(object target) - { - return _propertyInfo.GetValue(target, index: null); - } + protected override object? DoGetValue(object target, string memberName) + { + return _propertyInfo.GetValue(target, index: null); + } - protected override void DoSetValue(object target, object? value) - { - _propertyInfo.SetValue(target, value, index: null); - } + protected override void DoSetValue(object target, string memberName, object? value) + { + _propertyInfo.SetValue(target, value, index: null); } } diff --git a/Jint/Runtime/Interop/Reflection/ReflectionAccessor.cs b/Jint/Runtime/Interop/Reflection/ReflectionAccessor.cs index ffc684fb05..3f6ea0b9c7 100644 --- a/Jint/Runtime/Interop/Reflection/ReflectionAccessor.cs +++ b/Jint/Runtime/Interop/Reflection/ReflectionAccessor.cs @@ -10,119 +10,115 @@ #pragma warning disable IL2072 #pragma warning disable IL2077 -namespace Jint.Runtime.Interop.Reflection +namespace Jint.Runtime.Interop.Reflection; + +/// +/// Strategy to read and write CLR object properties and fields. +/// +internal abstract class ReflectionAccessor { - /// - /// Strategy to read and write CLR object properties and fields. - /// - internal abstract class ReflectionAccessor - { - protected readonly Type _memberType; - private readonly object? _memberName; - private readonly PropertyInfo? _indexer; + private readonly Type _memberType; + private readonly PropertyInfo? _indexer; - public Type MemberType => _memberType; + public Type MemberType => _memberType; - protected ReflectionAccessor( - Type memberType, - object? memberName, - PropertyInfo? indexer = null) - { - _memberType = memberType; - _memberName = memberName; - _indexer = indexer; - } + protected ReflectionAccessor( + Type memberType, + PropertyInfo? indexer = null) + { + _memberType = memberType; + _indexer = indexer; + } - public virtual bool Readable => true; + public virtual bool Readable => true; - public abstract bool Writable { get; } + public abstract bool Writable { get; } - protected abstract object? DoGetValue(object target); + protected abstract object? DoGetValue(object target, string memberName); - protected abstract void DoSetValue(object target, object? value); + protected abstract void DoSetValue(object target, string memberName, object? value); - public object? GetValue(Engine engine, object target) + public object? GetValue(Engine engine, object target, string memberName) + { + var constantValue = ConstantValue; + if (constantValue is not null) { - var constantValue = ConstantValue; - if (constantValue is not null) - { - return constantValue; - } - - // first check indexer so we don't confuse inherited properties etc - var value = TryReadFromIndexer(target); - - if (value is null) - { - try - { - value = DoGetValue(target); - } - catch (TargetInvocationException tie) - { - ExceptionHelper.ThrowMeaningfulException(engine, tie); - } - } - - return value; + return constantValue; } - protected virtual JsValue? ConstantValue => null; + // first check indexer so we don't confuse inherited properties etc + var value = TryReadFromIndexer(target, memberName); - private object? TryReadFromIndexer(object target) + if (value is null) { - var getter = _indexer?.GetGetMethod(); - if (getter is null) - { - return null; - } - try { - object[] parameters = { _memberName! }; - return getter.Invoke(target, parameters); + value = DoGetValue(target, memberName); } - catch + catch (TargetInvocationException tie) { - return null; + ExceptionHelper.ThrowMeaningfulException(engine, tie); } } - public void SetValue(Engine engine, object target, JsValue value) + return value; + } + + protected virtual JsValue? ConstantValue => null; + + private object? TryReadFromIndexer(object target, string memberName) + { + var getter = _indexer?.GetGetMethod(); + if (getter is null) { - object? converted; - if (_memberType == typeof(JsValue)) - { - converted = value; - } - else if (!ReflectionExtensions.TryConvertViaTypeCoercion(_memberType, engine.Options.Interop.ValueCoercion, value, out converted)) - { - // attempt to convert the JsValue to the target type - converted = value.ToObject(); - if (converted != null && converted.GetType() != _memberType) - { - converted = ConvertValueToSet(engine, converted); - } - } + return null; + } - try - { - DoSetValue(target, converted); - } - catch (TargetInvocationException exception) + try + { + object[] parameters = [memberName]; + return getter.Invoke(target, parameters); + } + catch + { + return null; + } + } + + public void SetValue(Engine engine, object target, string memberName, JsValue value) + { + object? converted; + if (_memberType == typeof(JsValue)) + { + converted = value; + } + else if (!ReflectionExtensions.TryConvertViaTypeCoercion(_memberType, engine.Options.Interop.ValueCoercion, value, out converted)) + { + // attempt to convert the JsValue to the target type + converted = value.ToObject(); + if (converted != null && converted.GetType() != _memberType) { - ExceptionHelper.ThrowMeaningfulException(engine, exception); + converted = ConvertValueToSet(engine, converted); } } - protected virtual object? ConvertValueToSet(Engine engine, [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.PublicFields)] object value) + try { - return engine.TypeConverter.Convert(value, _memberType, CultureInfo.InvariantCulture); + DoSetValue(target, memberName, converted); } - - public virtual PropertyDescriptor CreatePropertyDescriptor(Engine engine, object target, bool enumerable = true) + catch (TargetInvocationException exception) { - return new ReflectionDescriptor(engine, this, target, enumerable); + ExceptionHelper.ThrowMeaningfulException(engine, exception); } } + + protected virtual object? ConvertValueToSet(Engine engine, [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.PublicFields)] object value) + { + return engine.TypeConverter.Convert(value, _memberType, CultureInfo.InvariantCulture); + } + + public virtual PropertyDescriptor CreatePropertyDescriptor(Engine engine, object target, string memberName, bool enumerable = true) + { + return new ReflectionDescriptor(engine, this, target, memberName, enumerable); + } } diff --git a/Jint/Runtime/Interop/TypeReference.cs b/Jint/Runtime/Interop/TypeReference.cs index 9ed77e3d96..bc6dc74cd5 100644 --- a/Jint/Runtime/Interop/TypeReference.cs +++ b/Jint/Runtime/Interop/TypeReference.cs @@ -295,7 +295,7 @@ private PropertyDescriptor CreatePropertyDescriptor(string name) { var key = new MemberAccessorKey(ReferenceType, name); var accessor = _memberAccessors.GetOrAdd(key, x => ResolveMemberAccessor(_engine, x.Type, x.PropertyName)); - return accessor.CreatePropertyDescriptor(_engine, ReferenceType, enumerable: true); + return accessor.CreatePropertyDescriptor(_engine, ReferenceType, name, enumerable: true); } private static ReflectionAccessor ResolveMemberAccessor( diff --git a/Jint/Runtime/Interop/TypeResolver.cs b/Jint/Runtime/Interop/TypeResolver.cs index 1b1640c19e..62ae8a8001 100644 --- a/Jint/Runtime/Interop/TypeResolver.cs +++ b/Jint/Runtime/Interop/TypeResolver.cs @@ -66,6 +66,12 @@ internal ReflectionAccessor GetAccessor( accessor = accessorFactory?.Invoke() ?? ResolvePropertyDescriptorFactory(engine, type, member, mustBeReadable, mustBeWritable); + // don't cache if numeric indexer + if (uint.TryParse(member, out _)) + { + return accessor; + } + // racy, we don't care, worst case we'll catch up later Interlocked.CompareExchange(ref engine._reflectionAccessors, new Dictionary(factories) @@ -101,7 +107,7 @@ private ReflectionAccessor ResolvePropertyDescriptorFactory( if (typeof(DynamicObject).IsAssignableFrom(type)) { - return new DynamicObjectAccessor(type, memberName); + return new DynamicObjectAccessor(type); } var typeResolverMemberNameComparer = MemberNameComparer; @@ -139,7 +145,7 @@ private ReflectionAccessor ResolvePropertyDescriptorFactory( if (list?.Count == 1) { - return new PropertyAccessor(memberName, list[0]); + return new PropertyAccessor(list[0]); } // try to find explicit method implementations @@ -166,7 +172,7 @@ private ReflectionAccessor ResolvePropertyDescriptorFactory( if (explicitMethods?.Count > 0) { - return new MethodAccessor(type, memberName, MethodDescriptor.Build(explicitMethods)); + return new MethodAccessor(type, MethodDescriptor.Build(explicitMethods)); } } @@ -224,7 +230,7 @@ private ReflectionAccessor ResolvePropertyDescriptorFactory( if (matches.Count > 0) { - return new MethodAccessor(type, memberName, MethodDescriptor.Build(matches)); + return new MethodAccessor(type, MethodDescriptor.Build(matches)); } } @@ -305,7 +311,7 @@ internal bool TryFindMemberAccessor( if (property is not null) { - accessor = new PropertyAccessor(memberName, property, indexerToTry); + accessor = new PropertyAccessor(property, indexerToTry); return true; } @@ -330,7 +336,7 @@ internal bool TryFindMemberAccessor( if (field is not null) { - accessor = new FieldAccessor(field, memberName, indexerToTry); + accessor = new FieldAccessor(field, indexerToTry); return true; } @@ -386,7 +392,7 @@ void AddMethod(MethodInfo m) if (methods?.Count > 0) { - accessor = new MethodAccessor(type, memberName, MethodDescriptor.Build(methods)); + accessor = new MethodAccessor(type, MethodDescriptor.Build(methods)); return true; } @@ -395,7 +401,7 @@ void AddMethod(MethodInfo m) if (nestedType != null) { var typeReference = TypeReference.CreateTypeReference(engine, nestedType); - accessor = new NestedTypeAccessor(typeReference, memberName); + accessor = new NestedTypeAccessor(typeReference); return true; }