Refine interop member search with readable/writable requirement (#1687)

Jint.Tests/Runtime/InteropTests.cs

@@ -3333,5 +3333,93 @@ public void CanDestructureInteropTargetMethod()
             var result = engine.Evaluate("const { getStrings } = test; getStrings().Count;");
             Assert.Equal(3, result);
         }
+
+        private class MetadataWrapper : IDictionary<string, object?>
+        {
+            public IEnumerator<KeyValuePair<string, object>> GetEnumerator() => throw new NotImplementedException();
+            IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
+            public void Add(KeyValuePair<string, object> item) => throw new NotImplementedException();
+            public void Clear() => throw new NotImplementedException();
+            public bool Contains(KeyValuePair<string, object> item) => throw new NotImplementedException();
+            public void CopyTo(KeyValuePair<string, object>[] array, int arrayIndex) => throw new NotImplementedException();
+            public bool Remove(KeyValuePair<string, object> item) => throw new NotImplementedException();
+            public int Count { get; set; }
+            public bool IsReadOnly { get; set; }
+            public bool ContainsKey(string key) => throw new NotImplementedException();
+            public void Add(string key, object value) => throw new NotImplementedException();
+            public bool Remove(string key) => throw new NotImplementedException();
+            public bool TryGetValue(string key, out object value)
+            {
+                value = "from-wrapper";
+                return true;
+            }
+
+            public object this[string key]
+            {
+                get => "from-wrapper";
+                set
+                {
+                }
+            }
+
+            public ICollection<string> Keys { get; set; }
+            public ICollection<object> Values { get; set; }
+        }
+
+        private class ShadowedGetter : IReadOnlyDictionary<string, object>
+        {
+            private Dictionary<string, object> _dictionary = new();
+
+            public void SetInitial(object? value, string? key)
+            {
+                _dictionary[key] = value;
+            }
+
+            public IEnumerator<KeyValuePair<string, object>> GetEnumerator() => throw new NotImplementedException();
+
+            IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
+
+            public int Count { get; }
+            public bool ContainsKey(string key) => _dictionary.ContainsKey(key);
+
+            public bool TryGetValue(string key, out object value) => _dictionary.TryGetValue(key, out value);
+
+            public object this[string key]
+            {
+                get
+                {
+                    _dictionary.TryGetValue(key, out var value);
+                    return value;
+                }
+            }
+
+            public IEnumerable<string> Keys { get; set; }
+            public IEnumerable<object> Values { get; set; }
+        }
+
+        private class ShadowingSetter : ShadowedGetter
+        {
+            public Dictionary<string, int> Metadata
+            {
+                set
+                {
+                    SetInitial(new MetadataWrapper(), "metadata");
+                }
+            }
+        }
+
+        [Fact]
+        public void CanSelectShadowedPropertiesBasedOnReadableAndWritable()
+        {
+            var engine = new Engine();
+            engine.SetValue("test", new ShadowingSetter
+            {
+                Metadata = null
+            });
+
+            engine.Evaluate("test.metadata['abc'] = 123");
+            var result = engine.Evaluate("test.metadata['abc']");
+            Assert.Equal("from-wrapper", result);
+        }
     }
 }

Jint/Runtime/Descriptors/Specialized/ReflectionDescriptor.cs

@@ -43,13 +43,14 @@ protected internal override JsValue? CustomValue
             set => DoSet(null, value);
         }
 
-        JsValue DoGet(JsValue? thisObj)
+        private JsValue DoGet(JsValue? thisObj)
         {
             var value = _reflectionAccessor.GetValue(_engine, _target);
             var type = _reflectionAccessor.MemberType;
             return JsValue.FromObjectWithType(_engine, value, type);
         }
-        void DoSet(JsValue? thisObj, JsValue? v)
+
+        private void DoSet(JsValue? thisObj, JsValue? v)
         {
             try
             {

Jint/Runtime/Interop/ObjectWrapper.cs

@@ -50,7 +50,7 @@ public override bool Set(JsValue property, JsValue value, JsValue receiver)
                 if (_properties is null || !_properties.ContainsKey(member))
                 {
                     // can try utilize fast path
-                    var accessor = _engine.Options.Interop.TypeResolver.GetAccessor(_engine, ClrType, member, forWrite: true);
+                    var accessor = _engine.Options.Interop.TypeResolver.GetAccessor(_engine, ClrType, member, mustBeReadable: false, mustBeWritable: true);
 
                     if (ReferenceEquals(accessor, ConstantValueAccessor.NullAccessor))
                     {
@@ -116,7 +116,13 @@ public override JsValue Get(JsValue property, JsValue receiver)
                 return (uint) index < list.Count ? FromObject(_engine, list[index]) : Undefined;
             }
 
-            return base.Get(property, receiver);
+            var desc = GetOwnProperty(property, mustBeReadable: true, mustBeWritable: false);
+            if (desc != PropertyDescriptor.Undefined)
+            {
+                return UnwrapJsValue(desc, receiver);
+            }
+
+            return Prototype?.Get(property, receiver) ?? Undefined;
         }
 
         public override List<JsValue> GetOwnPropertyKeys(Types types = Types.None | Types.String | Types.Symbol)
@@ -182,6 +188,12 @@ private IEnumerable<JsValue> EnumerateOwnPropertyKeys(Types types)
         }
 
         public override PropertyDescriptor GetOwnProperty(JsValue property)
+        {
+            // we do not know if we need to read or write
+            return GetOwnProperty(property, mustBeReadable: false, mustBeWritable: false);
+        }
+
+        private PropertyDescriptor GetOwnProperty(JsValue property, bool mustBeReadable, bool mustBeWritable)
         {
             if (TryGetProperty(property, out var x))
             {
@@ -234,13 +246,17 @@ public override PropertyDescriptor GetOwnProperty(JsValue property)
                 return new PropertyDescriptor(result, PropertyFlag.OnlyEnumerable);
             }
 
-            var accessor = _engine.Options.Interop.TypeResolver.GetAccessor(_engine, ClrType, member);
+            var accessor = _engine.Options.Interop.TypeResolver.GetAccessor(_engine, ClrType, member, mustBeReadable, mustBeWritable);
             var descriptor = accessor.CreatePropertyDescriptor(_engine, Target, enumerable: !isDictionary);
-            if (!isDictionary && !ReferenceEquals(descriptor, PropertyDescriptor.Undefined))
+            if (!isDictionary
+                && !ReferenceEquals(descriptor, PropertyDescriptor.Undefined)
+                && (!mustBeReadable || accessor.Readable)
+                && (!mustBeWritable || accessor.Writable))
             {
                 // cache the accessor for faster subsequent accesses
                 SetProperty(member, descriptor);
             }
+
             return descriptor;
         }
 
@@ -258,7 +274,9 @@ public static PropertyDescriptor GetPropertyDescriptor(Engine engine, object tar
                     _ => null
                 };
             }
-            return engine.Options.Interop.TypeResolver.GetAccessor(engine, target.GetType(), member.Name, Factory).CreatePropertyDescriptor(engine, target);
+
+            var accessor = engine.Options.Interop.TypeResolver.GetAccessor(engine, target.GetType(), member.Name, mustBeReadable: false, mustBeWritable: false, Factory);
+            return accessor.CreatePropertyDescriptor(engine, target);
         }
 
         internal static Type GetClrType(object obj, Type? type)

Jint/Runtime/Interop/TypeResolver.cs

@@ -47,8 +47,9 @@ internal ReflectionAccessor GetAccessor(
             Engine engine,
             Type type,
             string member,
-            Func<ReflectionAccessor?>? accessorFactory = null,
-            bool forWrite = false)
+            bool mustBeReadable,
+            bool mustBeWritable,
+            Func<ReflectionAccessor?>? accessorFactory = null)
         {
             var key = new Engine.ClrPropertyDescriptorFactoriesKey(type, member);
 
@@ -58,7 +59,7 @@ internal ReflectionAccessor GetAccessor(
                 return accessor;
             }
 
-            accessor = accessorFactory?.Invoke() ?? ResolvePropertyDescriptorFactory(engine, type, member, forWrite);
+            accessor = accessorFactory?.Invoke() ?? ResolvePropertyDescriptorFactory(engine, type, member, mustBeReadable, mustBeWritable);
 
             // racy, we don't care, worst case we'll catch up later
             Interlocked.CompareExchange(ref engine._reflectionAccessors,
@@ -74,7 +75,8 @@ private ReflectionAccessor ResolvePropertyDescriptorFactory(
             Engine engine,
             Type type,
             string memberName,
-            bool forWrite)
+            bool mustBeReadable,
+            bool mustBeWritable)
         {
             var isInteger = long.TryParse(memberName, NumberStyles.Integer, CultureInfo.InvariantCulture, out _);
 
@@ -86,7 +88,8 @@ private ReflectionAccessor ResolvePropertyDescriptorFactory(
             // properties and fields cannot be numbers
             if (!isInteger
                 && TryFindMemberAccessor(engine, type, memberName, BindingFlags, indexer, out var temp)
-                && (!forWrite || temp.Writable))
+                && (!mustBeReadable || temp.Readable)
+                && (!mustBeWritable || temp.Writable))
             {
                 return temp;
             }