Fix inheriting from TypeReference backed interop type

* make OrdinaryCreateFromConstructor generic
sebastienros · May 15, 2022 · 71cece6 · 71cece6
1 parent 8c89089
commit 71cece6
Show file tree

Hide file tree

Showing 16 changed files with 133 additions and 99 deletions.
diff --git a/Jint.Tests/Runtime/InteropTests.cs b/Jint.Tests/Runtime/InteropTests.cs
@@ -789,20 +789,64 @@ public void CanUseDelegateAsFunction()
         [Fact]
         public void JavaScriptClassCanExtendClrType()
         {
-            var engine = new Engine();
-            engine.SetValue("TestClass", TypeReference.CreateTypeReference<TestClass>(engine));
+            _engine.SetValue("TestClass", TypeReference.CreateTypeReference<TestClass>(_engine));
+
+            _engine.Execute("class ExtendedType extends TestClass { constructor() { super(); this.a = 1; } get aProp() { return 'A'; } }");
+            _engine.Execute("class MyExtendedType extends ExtendedType { constructor() { super(); this.b = 2; } get bProp() { return 'B'; } }");
+            _engine.Evaluate("let obj = new MyExtendedType();");
 
-            engine.Execute("class ExtendedType extends TestClass { constructor() { super(); this.a = 1; } }");
-            engine.Execute("class MyExtendedType extends ExtendedType { constructor() { super(); this.b = 2; } }");
-            engine.Evaluate("let obj = new MyExtendedType();");
+            _engine.Evaluate("obj.setString('Hello World!');");
 
-            engine.Evaluate("obj.setString('Hello World!');");
+            Assert.Equal("Hello World!", _engine.Evaluate("obj.string"));
+            Assert.Equal(1, _engine.Evaluate("obj.a"));
+            Assert.Equal(2, _engine.Evaluate("obj.b"));
 
-            Assert.Equal("Hello World!", engine.Evaluate("obj.string"));
-            Assert.Equal(1, engine.Evaluate("obj.a"));
-            Assert.Equal(2, engine.Evaluate("obj.b"));
+            Assert.Equal("A", _engine.Evaluate("obj.aProp"));
+            Assert.Equal("B", _engine.Evaluate("obj.bProp"));
+
+            // TODO we should have a special prototype based on wrapped type so we could differentiate between own and type properties 
+            // Assert.Equal("[\"a\"]", _engine.Evaluate("JSON.stringify(Object.getOwnPropertyNames(new ExtendedType()))"));
+            // Assert.Equal("[\"a\",\"b\"]", _engine.Evaluate("JSON.stringify(Object.getOwnPropertyNames(new MyExtendedType()))"));
         }
+
+        [Fact]
+        public void ShouldAllowMethodsOnClrExtendedTypes()
+        {
+            _engine.SetValue("ClrBaseType", TypeReference.CreateTypeReference<TestClass>(_engine));
+            _engine.Evaluate(@"
+                class JsBaseType {}
+                class ExtendsFromJs extends JsBaseType {
+                    constructor() {
+                        super();
+                        this.a = 1;
+                    }
+
+                    getA() {
+                        return this.a;
+                    }
+                }
 
+                class ExtendsFromClr extends ClrBaseType {
+                    constructor() {
+                        super();
+                        this.a = 1;
+                    }
+
+                    getA() {
+                        return this.a;
+                    }
+                }
+            ");
+
+            var extendsFromJs = _engine.Construct("ExtendsFromJs");
+            Assert.Equal(1, _engine.Evaluate("new ExtendsFromJs().getA();"));
+            Assert.NotEqual(JsValue.Undefined, extendsFromJs.Get("getA"));
+
+            var extendsFromClr = _engine.Construct("ExtendsFromClr");
+            Assert.Equal(1, _engine.Evaluate("new ExtendsFromClr().getA();"));
+            Assert.NotEqual(JsValue.Undefined, extendsFromClr.Get("getA"));
+        }
+
         private struct TestStruct
         {
             public int Value;

diff --git a/Jint/Engine.cs b/Jint/Engine.cs
@@ -558,7 +558,7 @@ internal void PutValue(Reference reference, JsValue value)
                 var succeeded = baseValue.Set(reference.GetReferencedName(), value, reference.GetThisValue());
                 if (!succeeded && reference.IsStrictReference())
                 {
-                    ExceptionHelper.ThrowTypeError(Realm);
+                    ExceptionHelper.ThrowTypeError(Realm, $"Unable to set value {value} to {reference.GetThisValue()}.{reference.GetReferencedName()}");
                 }
             }
             else

diff --git a/Jint/Native/DataView/DataViewConstructor.cs b/Jint/Native/DataView/DataViewConstructor.cs
@@ -80,7 +80,7 @@ ObjectInstance IConstructor.Construct(JsValue[] arguments, JsValue newTarget)
             var o = OrdinaryCreateFromConstructor(
                 newTarget,
                 static intrinsics => intrinsics.DataView.PrototypeObject,
-                static (engine, realm, state) => new DataViewInstance(engine));
+                static (Engine engine, Realm _, object _) => new DataViewInstance(engine));
 
             if (buffer.IsDetachedBuffer)
             {

diff --git a/Jint/Native/Date/DateConstructor.cs b/Jint/Native/Date/DateConstructor.cs
@@ -212,7 +212,7 @@ private ObjectInstance Construct(JsValue[] arguments, JsValue newTarget)
             var o = OrdinaryCreateFromConstructor(
                 newTarget,
                 static intrinsics => intrinsics.Date.PrototypeObject,
-                static (engine, realm, _) => new DateInstance(engine));
+                static (Engine engine, Realm _, object _) => new DateInstance(engine));
             o.DateValue = dv;
             return o;
         }

diff --git a/Jint/Native/Error/ErrorConstructor.cs b/Jint/Native/Error/ErrorConstructor.cs
@@ -47,7 +47,7 @@ private ObjectInstance Construct(JsValue[] arguments, JsValue newTarget)
             var o = OrdinaryCreateFromConstructor(
                 newTarget,
                 _intrinsicDefaultProto,
-                static (engine, realm, state) => new ErrorInstance(engine));
+                static (Engine engine, Realm _, object _) => new ErrorInstance(engine));
 
             var jsValue = arguments.At(0);
             if (!jsValue.IsUndefined())

diff --git a/Jint/Native/Function/FunctionInstance.cs b/Jint/Native/Function/FunctionInstance.cs
@@ -227,11 +227,11 @@ internal void SetFunctionName(JsValue name, string prefix = null, bool force = f
         /// In spec intrinsicDefaultProto is string pointing to intrinsic, but we do a selector.
         /// </remarks>
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        internal T OrdinaryCreateFromConstructor<T>(
+        internal T OrdinaryCreateFromConstructor<T, TState>(
             JsValue constructor,
             Func<Intrinsics, ObjectInstance> intrinsicDefaultProto,
-            Func<Engine, Realm, JsValue, T> objectCreator,
-            JsValue state = null) where T : ObjectInstance
+            Func<Engine, Realm, TState, T> objectCreator,
+            TState state = default) where T : ObjectInstance
         {
             var proto = GetPrototypeFromConstructor(constructor, intrinsicDefaultProto);
 

diff --git a/Jint/Native/Function/ScriptFunctionInstance.cs b/Jint/Native/Function/ScriptFunctionInstance.cs
@@ -11,7 +11,7 @@ namespace Jint.Native.Function
 {
     public sealed class ScriptFunctionInstance : FunctionInstance, IConstructor
     {
-        internal bool _isClassConstructor;
+        private bool _isClassConstructor;
 
         /// <summary>
         /// http://www.ecma-international.org/ecma-262/5.1/#sec-13.2
@@ -121,7 +121,7 @@ ObjectInstance IConstructor.Construct(JsValue[] arguments, JsValue newTarget)
                 thisArgument = OrdinaryCreateFromConstructor(
                     newTarget,
                     static intrinsics => intrinsics.Object.PrototypeObject,
-                    static (engine, realm, _) => new ObjectInstance(engine));
+                    static (Engine engine, Realm _, object _) => new ObjectInstance(engine));
             }
 
             var calleeContext = PrepareForOrdinaryCall(newTarget);

diff --git a/Jint/Native/Map/MapConstructor.cs b/Jint/Native/Map/MapConstructor.cs
@@ -61,7 +61,8 @@ ObjectInstance IConstructor.Construct(JsValue[] arguments, JsValue newTarget)
             var map = OrdinaryCreateFromConstructor(
                 newTarget,
                 static intrinsics => intrinsics.Map.PrototypeObject,
-                static (engine, realm, _) => new MapInstance(engine, realm));
+                static (Engine engine, Realm realm, object _) => new MapInstance(engine, realm));
+
             if (arguments.Length > 0 && !arguments[0].IsNullOrUndefined())
             {
                 var adder = map.Get("set");

diff --git a/Jint/Native/Object/ObjectConstructor.cs b/Jint/Native/Object/ObjectConstructor.cs
@@ -163,7 +163,7 @@ private ObjectInstance Construct(JsValue[] arguments, JsValue newTarget)
                 return OrdinaryCreateFromConstructor(
                     newTarget,
                     static intrinsics => intrinsics.Object.PrototypeObject,
-                    (engine, realm, state) => new ObjectInstance(engine));
+                    static (Engine engine, Realm _, object _) => new ObjectInstance(engine));
             }
 
             if (arguments.Length > 0)

diff --git a/Jint/Native/Promise/PromiseConstructor.cs b/Jint/Native/Promise/PromiseConstructor.cs
@@ -87,7 +87,7 @@ ObjectInstance IConstructor.Construct(JsValue[] arguments, JsValue newTarget)
             var instance = OrdinaryCreateFromConstructor(
                 newTarget,
                 static intrinsics => intrinsics.Promise.PrototypeObject,
-                static(engine, realm, _) => new PromiseInstance(engine));
+                static (Engine engine, Realm _, object _) => new PromiseInstance(engine));
 
             var (resolve, reject) = instance.CreateResolvingFunctions();
             promiseExecutor.Call(Undefined, new JsValue[] {resolve, reject});

diff --git a/Jint/Native/RegExp/RegExpConstructor.cs b/Jint/Native/RegExp/RegExpConstructor.cs
@@ -134,7 +134,7 @@ private RegExpInstance RegExpAlloc(JsValue newTarget)
             var r = OrdinaryCreateFromConstructor(
                 newTarget,
                 static intrinsics => intrinsics.RegExp.PrototypeObject,
-                static (engine, realm, _) => new RegExpInstance(engine));
+                static (Engine engine, Realm _, object _) => new RegExpInstance(engine));
             return r;
         }
 

diff --git a/Jint/Native/Set/SetConstructor.cs b/Jint/Native/Set/SetConstructor.cs
@@ -61,7 +61,8 @@ ObjectInstance IConstructor.Construct(JsValue[] arguments, JsValue newTarget)
             var set = OrdinaryCreateFromConstructor(
                 newTarget,
                 static intrinsics => intrinsics.Set.PrototypeObject,
-                static (engine, realm, _) => new SetInstance(engine));
+                static (Engine engine, Realm _, object _) => new SetInstance(engine));
+
             if (arguments.Length > 0 && !arguments[0].IsNullOrUndefined())
             {
                 var adderValue = set.Get("add");

diff --git a/Jint/Native/WeakMap/WeakMapConstructor.cs b/Jint/Native/WeakMap/WeakMapConstructor.cs
@@ -41,7 +41,7 @@ ObjectInstance IConstructor.Construct(JsValue[] arguments, JsValue newTarget)
             var map = OrdinaryCreateFromConstructor(
                 newTarget,
                 static intrinsics =>  intrinsics.WeakMap.PrototypeObject,
-                static (engine, realm, _) => new WeakMapInstance(engine));
+                static (Engine engine, Realm _, object _) => new WeakMapInstance(engine));
             if (arguments.Length > 0 && !arguments[0].IsNullOrUndefined())
             {
                 var adder = map.Get("set");

diff --git a/Jint/Native/WeakSet/WeakSetConstructor.cs b/Jint/Native/WeakSet/WeakSetConstructor.cs
@@ -40,7 +40,8 @@ ObjectInstance IConstructor.Construct(JsValue[] arguments, JsValue newTarget)
             var set = OrdinaryCreateFromConstructor(
                 newTarget,
                 static intrinsics => intrinsics.WeakSet.PrototypeObject,
-                static (engine, realm, _) => new WeakSetInstance(engine));
+                static (Engine engine, Realm _, object _) => new WeakSetInstance(engine));
+
             if (arguments.Length > 0 && !arguments[0].IsNullOrUndefined())
             {
                 var adder = set.Get("add") as ICallable;

diff --git a/Jint/Runtime/Interop/ObjectWrapper.cs b/Jint/Runtime/Interop/ObjectWrapper.cs
@@ -1,3 +1,5 @@
+#nullable enable
+
 using System;
 using System.Collections;
 using System.Collections.Generic;
@@ -18,7 +20,6 @@ namespace Jint.Runtime.Interop
 	public sealed class ObjectWrapper : ObjectInstance, IObjectWrapper, IEquatable<ObjectWrapper>
     {
         private readonly TypeDescriptor _typeDescriptor;
-        internal bool _allowAddingProperties;
 
         public ObjectWrapper(Engine engine, object obj)
             : base(engine)
@@ -53,7 +54,7 @@ public override bool Set(JsValue property, JsValue value, JsValue receiver)
                     // can try utilize fast path
                     var accessor = _engine.Options.Interop.TypeResolver.GetAccessor(_engine, Target.GetType(), member);
 
-                    if (ReferenceEquals(accessor, ConstantValueAccessor.NullAccessor) && _allowAddingProperties)
+                    if (ReferenceEquals(accessor, ConstantValueAccessor.NullAccessor))
                     {
                         // there's no such property, but we can allow extending by calling base
                         // which will add properties, this allows for example JS class to extend a CLR type
@@ -92,12 +93,6 @@ private bool SetSlow(JsValue property, JsValue value)
             }
 
             var ownDesc = GetOwnProperty(property);
-
-            if (ownDesc == null)
-            {
-                return false;
-            }
-
             ownDesc.Value = value;
             return true;
         }
@@ -133,18 +128,14 @@ public override IEnumerable<KeyValuePair<JsValue, PropertyDescriptor>> GetOwnPro
 
         private IEnumerable<JsValue> EnumerateOwnPropertyKeys(Types types)
         {
-            var basePropertyKeys = base.GetOwnPropertyKeys(types);
             // prefer object order, add possible other properties after
-            var processed = basePropertyKeys.Count > 0 ? new HashSet<JsValue>() : null;
-
             var includeStrings = (types & Types.String) != 0;
             if (includeStrings && _typeDescriptor.IsStringKeyedGenericDictionary) // expando object for instance
             {
                 var keys = _typeDescriptor.GetKeys(Target);
                 foreach (var key in keys)
                 {
                     var jsString = JsString.Create(key);
-                    processed?.Add(jsString);
                     yield return jsString;
                 }
             }
@@ -153,12 +144,11 @@ private IEnumerable<JsValue> EnumerateOwnPropertyKeys(Types types)
                 // we take values exposed as dictionary keys only
                 foreach (var key in dictionary.Keys)
                 {
-                    object stringKey = key as string;
+                    object? stringKey = key as string;
                     if (stringKey is not null
                         || _engine.ClrTypeConverter.TryConvert(key, typeof(string), CultureInfo.InvariantCulture, out stringKey))
                     {
                         var jsString = JsString.Create((string) stringKey);
-                        processed?.Add(jsString);
                         yield return jsString;
                     }
                 }
@@ -173,31 +163,16 @@ private IEnumerable<JsValue> EnumerateOwnPropertyKeys(Types types)
                     if (indexParameters.Length == 0)
                     {
                         var jsString = JsString.Create(p.Name);
-                        processed?.Add(jsString);
                         yield return jsString;
                     }
                 }
 
                 foreach (var f in type.GetFields(BindingFlags.Static | BindingFlags.Instance | BindingFlags.Public))
                 {
                     var jsString = JsString.Create(f.Name);
-                    processed?.Add(jsString);
                     yield return jsString;
                 }
             }
-
-            if (processed != null)
-            {
-                // we have base keys
-                for (var i = 0; i < basePropertyKeys.Count; i++)
-                {
-                    var key = basePropertyKeys[i];
-                    if (processed.Add(key))
-                    {
-                        yield return key;
-                    }
-                }
-            }
         }
 
         public override PropertyDescriptor GetOwnProperty(JsValue property)
@@ -244,8 +219,9 @@ public override PropertyDescriptor GetOwnProperty(JsValue property)
 
             var accessor = _engine.Options.Interop.TypeResolver.GetAccessor(_engine, Target.GetType(), member);
             var descriptor = accessor.CreatePropertyDescriptor(_engine, Target, enumerable: !isDictionary);
-            if (!isDictionary)
+            if (!isDictionary && !ReferenceEquals(descriptor, PropertyDescriptor.Undefined))
             {
+                // cache the accessor for faster subsequent accesses
                 SetProperty(member, descriptor);
             }
             return descriptor;
@@ -255,7 +231,7 @@ public override PropertyDescriptor GetOwnProperty(JsValue property)
         public static PropertyDescriptor GetPropertyDescriptor(Engine engine, object target, MemberInfo member)
         {
             // fast path which uses slow search if not found for some reason
-            ReflectionAccessor Factory()
+            ReflectionAccessor? Factory()
             {
                 return member switch
                 {
@@ -283,17 +259,17 @@ private static JsValue GetLength(JsValue thisObj, JsValue[] arguments)
             return JsNumber.Create((int) wrapper._typeDescriptor.LengthProperty.GetValue(wrapper.Target));
         }
 
-        public override bool Equals(JsValue obj)
+        public override bool Equals(JsValue? obj)
         {
             return Equals(obj as ObjectWrapper);
         }
 
-        public override bool Equals(object obj)
+        public override bool Equals(object? obj)
         {
             return Equals(obj as ObjectWrapper);
         }
 
-        public bool Equals(ObjectWrapper other)
+        public bool Equals(ObjectWrapper? other)
         {
             if (ReferenceEquals(null, other))
             {