Fix inheriting from TypeReference backed interop type (#1168)

* remove old checks for can write property and use proper logic
* make OrdinaryCreateFromConstructor generic
* cleanup some build warnings

Jint.Tests.Test262/Jint.Tests.Test262.csproj

@@ -5,6 +5,7 @@
     <AssemblyOriginatorKeyFile>..\Jint\Jint.snk</AssemblyOriginatorKeyFile>
     <SignAssembly>true</SignAssembly>
     <IsPackable>false</IsPackable>
+    <NoWarn>$(NoWarn);CS8002</NoWarn>
   </PropertyGroup>
   <ItemGroup>
     <ProjectReference Include="..\Jint\Jint.csproj" />

Jint.Tests/Runtime/GenericMethodTests.cs

@@ -79,6 +79,7 @@ public class GenericMethodTests
     // we _may_ be able to address this by simply instantiating generic types using System.Object for the generic arguments
     // This test currently generates the following error:
     // No public methods with the specified arguments were found.
+    [Fact(Skip = "not supported yet")]
     public void TestGenericClassDeriveFromGenericInterface()
     {
         var engine = new Engine(cfg => cfg.AllowClr(typeof(OpenGenericTest<>).Assembly));
@@ -206,17 +207,15 @@ public class GenericMethodTests
             private set;
         }
 
-        TState _stateSubject;
-
         public ReduxStore()
         {
             SelectInvoked = false;
         }
 
-        public IObservable<TResult> Select<TResult>(ISelectorWithoutProps<TState, TResult> selector, string? optionsStr = null)
+        public IObservable<TResult> Select<TResult>(ISelectorWithoutProps<TState, TResult> selector, string optionsStr = null)
         {
             SelectInvoked = true;
-            return selector.Apply(_stateSubject);
+            return selector.Apply(null);
         }
     }
 

Jint.Tests/Runtime/InteropTests.cs

@@ -789,20 +789,64 @@ namespace Jint.Tests.Runtime
         [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;

Jint.Tests/Runtime/ModuleTests.cs

@@ -1,5 +1,4 @@
 #if(NET6_0_OR_GREATER)
-using System;
 using System.IO;
 using System.Reflection;
 #endif

Jint/Native/BigInt/BigIntConstructor.cs

@@ -115,7 +115,9 @@ public sealed class BigIntConstructor : FunctionInstance, IConstructor
         var o = OrdinaryCreateFromConstructor(
             newTarget,
             static intrinsics => intrinsics.BigInt.PrototypeObject,
-            static (engine, realm, state) => new BigIntInstance(engine, (JsBigInt) state), value);
+            static (engine, realm, state) => new BigIntInstance(engine, state),
+            value);
+        
         return o;
     }
 

Jint/Native/BigInt/BigIntPrototype.cs

@@ -150,4 +150,4 @@ public sealed class BigIntPrototype : ObjectInstance
                 return JsBigInt.One;
         }
     }
-}
+}

Jint/Native/DataView/DataViewConstructor.cs

@@ -80,7 +80,7 @@ namespace Jint.Native.DataView
             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)
             {

Jint/Native/Date/DateConstructor.cs

@@ -212,7 +212,7 @@ namespace Jint.Native.Date
             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;
         }

Jint/Native/Error/ErrorConstructor.cs

@@ -47,7 +47,7 @@ namespace Jint.Native.Error
             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())

Jint/Native/Function/FunctionInstance.cs

@@ -227,11 +227,11 @@ namespace Jint.Native.Function
         /// 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);
 

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 @@ namespace Jint.Native.Function
                 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);

Jint/Native/Map/MapConstructor.cs

@@ -61,7 +61,8 @@ namespace Jint.Native.Map
             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");

Jint/Native/Object/ObjectConstructor.cs

@@ -163,7 +163,7 @@ namespace Jint.Native.Object
                 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)

Jint/Native/Promise/PromiseConstructor.cs

@@ -87,7 +87,7 @@ namespace Jint.Native.Promise
             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});

Jint/Native/RegExp/RegExpConstructor.cs

@@ -134,7 +134,7 @@ namespace Jint.Native.RegExp
             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;
         }
 

Jint/Native/Set/SetConstructor.cs

@@ -61,7 +61,8 @@ namespace Jint.Native.Set
             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");

Jint/Native/String/StringPrototype.cs

@@ -1,5 +1,4 @@
 using System;
-using System.Collections.Generic;
 using System.Linq;
 using System.Runtime.CompilerServices;
 using System.Text;

Jint/Native/WeakMap/WeakMapConstructor.cs

@@ -41,7 +41,7 @@ namespace Jint.Native.WeakMap
             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");

Jint/Native/WeakSet/WeakSetConstructor.cs

@@ -40,7 +40,8 @@ namespace Jint.Native.WeakSet
             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;

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 @@ namespace Jint.Runtime.Interop
                     // 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 @@ namespace Jint.Runtime.Interop
             }
 
             var ownDesc = GetOwnProperty(property);
-
-            if (ownDesc == null)
-            {
-                return false;
-            }
-
             ownDesc.Value = value;
             return true;
         }
@@ -133,10 +128,7 @@ namespace Jint.Runtime.Interop
 
         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
             {
@@ -144,7 +136,6 @@ namespace Jint.Runtime.Interop
                 foreach (var key in keys)
                 {
                     var jsString = JsString.Create(key);
-                    processed?.Add(jsString);
                     yield return jsString;
                 }
             }
@@ -153,12 +144,11 @@ namespace Jint.Runtime.Interop
                 // 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,7 +163,6 @@ namespace Jint.Runtime.Interop
                     if (indexParameters.Length == 0)
                     {
                         var jsString = JsString.Create(p.Name);
-                        processed?.Add(jsString);
                         yield return jsString;
                     }
                 }
@@ -181,23 +170,9 @@ namespace Jint.Runtime.Interop
                 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 @@ namespace Jint.Runtime.Interop
 
             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 @@ namespace Jint.Runtime.Interop
         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 @@ namespace Jint.Runtime.Interop
             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))
             {

Jint/Runtime/Interop/TypeReference.cs

@@ -1,4 +1,6 @@
-using System;
+#nullable enable
+
+using System;
 using System.Collections.Concurrent;
 using System.Reflection;
 using Jint.Collections;
@@ -14,8 +16,10 @@ namespace Jint.Runtime.Interop
     {
         private static readonly JsString _name = new JsString("typereference");
         private static readonly ConcurrentDictionary<Type, MethodDescriptor[]> _constructorCache = new();
-        private static readonly ConcurrentDictionary<Tuple<Type, string>, ReflectionAccessor> _memberAccessors = new();
+        private static readonly ConcurrentDictionary<MemberAccessorKey, ReflectionAccessor> _memberAccessors = new();
 
+        private readonly record struct MemberAccessorKey(Type Type, string PropertyName); 
+        
         private TypeReference(Engine engine, Type type)
             : base(engine, engine.Realm, _name, FunctionThisMode.Global, ObjectClass.TypeReference)
         {
@@ -43,50 +47,63 @@ namespace Jint.Runtime.Interop
         protected internal override JsValue Call(JsValue thisObject, JsValue[] arguments)
         {
             // direct calls on a TypeReference constructor object is equivalent to the new operator
-            return Construct(arguments);
+            return Construct(arguments, Undefined);
         }
 
-        ObjectInstance IConstructor.Construct(JsValue[] arguments, JsValue newTarget) => Construct(arguments);
+        ObjectInstance IConstructor.Construct(JsValue[] arguments, JsValue newTarget) => Construct(arguments, newTarget);
 
-        private ObjectInstance Construct(JsValue[] arguments)
+        private ObjectInstance Construct(JsValue[] arguments, JsValue newTarget)
         {
-            ObjectInstance result = null;
-            if (arguments.Length == 0 && ReferenceType.IsValueType)
-            {
-                var instance = Activator.CreateInstance(ReferenceType);
-                result = TypeConverter.ToObject(_realm, FromObject(Engine, instance));
-            }
-            else
+            static ObjectInstance ObjectCreator(Engine engine, Realm realm, ObjectCreateState state)
             {
-                var constructors = _constructorCache.GetOrAdd(
-                    ReferenceType,
-                    t => MethodDescriptor.Build(t.GetConstructors(BindingFlags.Public | BindingFlags.Instance)));
-
-                foreach (var (method, _, _) in TypeConverter.FindBestMatch(_engine, constructors, _ => arguments))
+                var arguments = state.Arguments;
+                var referenceType = state.TypeReference.ReferenceType;
+                
+                ObjectInstance? result = null;
+                if (arguments.Length == 0 && referenceType.IsValueType)
+                {
+                    var instance = Activator.CreateInstance(referenceType);
+                    result = TypeConverter.ToObject(realm, FromObject(engine, instance));
+                }
+                else
                 {
-                    var retVal = method.Call(Engine, null, arguments);
-                    result = TypeConverter.ToObject(_realm, retVal);
+                    var constructors = _constructorCache.GetOrAdd(
+                        referenceType,
+                        t => MethodDescriptor.Build(t.GetConstructors(BindingFlags.Public | BindingFlags.Instance)));
+
+                    foreach (var (method, _, _) in TypeConverter.FindBestMatch(engine, constructors, _ => arguments))
+                    {
+                        var retVal = method.Call(engine, null, arguments);
+                        result = TypeConverter.ToObject(realm, retVal);
 
-                    // todo: cache method info
-                    break;
+                        // todo: cache method info
+                        break;
+                    }
                 }
-            }
 
-            if (result is not null)
-            {
-                if (result is ObjectWrapper objectWrapper)
+                if (result is null)
                 {
-                    // allow class extension
-                    objectWrapper._allowAddingProperties = true;
+                    ExceptionHelper.ThrowTypeError(realm, "No public methods with the specified arguments were found.");
                 }
 
+                result.SetPrototypeOf(state.TypeReference);
+
                 return result;
             }
 
-            ExceptionHelper.ThrowTypeError(_engine.Realm, "No public methods with the specified arguments were found.");
-            return null;
+            // TODO should inject prototype that reflects TypeReference's target's layout
+            var thisArgument = OrdinaryCreateFromConstructor(
+                newTarget,
+                static intrinsics => intrinsics.Object.PrototypeObject,
+                ObjectCreator,
+                new ObjectCreateState(this, arguments));
+
+           
+            return thisArgument;
         }
 
+        private readonly record struct ObjectCreateState(TypeReference TypeReference, JsValue[] Arguments);
+
         internal override bool OrdinaryHasInstance(JsValue v)
         {
             if (v is IObjectWrapper wrapper)
@@ -115,12 +132,6 @@ namespace Jint.Runtime.Interop
             }
 
             var ownDesc = GetOwnProperty(property);
-
-            if (ownDesc == null)
-            {
-                return false;
-            }
-
             ownDesc.Value = value;
             return true;
         }
@@ -151,14 +162,14 @@ namespace Jint.Runtime.Interop
 
         private PropertyDescriptor CreatePropertyDescriptor(string name)
         {
-            var key = new Tuple<Type, string>(ReferenceType, name);
-            var accessor = _memberAccessors.GetOrAdd(key, x => ResolveMemberAccessor(x.Item1, x.Item2));
+            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);
         }
 
-        private ReflectionAccessor ResolveMemberAccessor(Type type, string name)
+        private static ReflectionAccessor ResolveMemberAccessor(Engine engine, Type type, string name)
         {
-            var typeResolver = _engine.Options.Interop.TypeResolver;
+            var typeResolver = engine.Options.Interop.TypeResolver;
 
             if (type.IsEnum)
             {
@@ -186,7 +197,7 @@ namespace Jint.Runtime.Interop
             }
 
             const BindingFlags bindingFlags = BindingFlags.Public | BindingFlags.Static;
-            return typeResolver.TryFindMemberAccessor(_engine, type, name, bindingFlags, indexerToTry: null, out var accessor)
+            return typeResolver.TryFindMemberAccessor(engine, type, name, bindingFlags, indexerToTry: null, out var accessor)
                 ? accessor
                 : ConstantValueAccessor.NullAccessor;
         }