Fix ToPropertyDescriptor Proxy issues (#1335)

Jint.Tests/Runtime/ProxyTests.cs

@@ -1,40 +1,75 @@
-namespace Jint.Tests.Runtime
+namespace Jint.Tests.Runtime;
+
+public class ProxyTests
 {
-    public class ProxyTests
+    [Fact]
+    public void ProxyCanBeRevokedWithoutContext()
     {
-        [Fact]
-        public void ProxyCanBeRevokedWithoutContext()
-        {
-            new Engine()
-                .Execute(@"
+        new Engine()
+            .Execute(@"
                     var revocable = Proxy.revocable({}, {});
                     var revoke = revocable.revoke;
                     revoke.call(null);
                 ");
-        }
+    }
 
-        [Fact]
-        public void ProxyToStringUseTarget()
-        {
-            var engine = new Engine().Execute(@"
+    [Fact]
+    public void ProxyToStringUseTarget()
+    {
+        var engine = new Engine().Execute(@"
                 const targetWithToString = {toString: () => 'target'}
             ");
-            Assert.Equal("target", engine.Evaluate("new Proxy(targetWithToString, {}).toString()").AsString());
-            Assert.Equal("target", engine.Evaluate("`${new Proxy(targetWithToString, {})}`").AsString());
-        }
-
-        [Fact]
-        public void ProxyToStringUseHandler()
-        {
-            var engine = new Engine().Execute(@"
+        Assert.Equal("target", engine.Evaluate("new Proxy(targetWithToString, {}).toString()").AsString());
+        Assert.Equal("target", engine.Evaluate("`${new Proxy(targetWithToString, {})}`").AsString());
+    }
+
+    [Fact]
+    public void ProxyToStringUseHandler()
+    {
+        var engine = new Engine().Execute(@"
                 const handler = { get: (target, prop, receiver) => prop === 'toString' ? () => 'handler' : Reflect.get(target, prop, receiver) }
                 const targetWithToString = {toString: () => 'target'}
             ");
 
-            Assert.Equal("handler", engine.Evaluate("new Proxy({}, handler).toString()").AsString());
-            Assert.Equal("handler", engine.Evaluate("new Proxy(targetWithToString, handler).toString()").AsString());
-            Assert.Equal("handler", engine.Evaluate("`${new Proxy({}, handler)}`").AsString());
-            Assert.Equal("handler", engine.Evaluate("`${new Proxy(targetWithToString, handler)}`").AsString());
-        }
+        Assert.Equal("handler", engine.Evaluate("new Proxy({}, handler).toString()").AsString());
+        Assert.Equal("handler", engine.Evaluate("new Proxy(targetWithToString, handler).toString()").AsString());
+        Assert.Equal("handler", engine.Evaluate("`${new Proxy({}, handler)}`").AsString());
+        Assert.Equal("handler", engine.Evaluate("`${new Proxy(targetWithToString, handler)}`").AsString());
+    }
+
+    [Fact]
+    public void ToPropertyDescriptor()
+    {
+        const string Script = @"
+            var get = [];
+            var p = new Proxy({
+                enumerable: true, configurable: true, value: true,
+                writable: true, get: Function(), set: Function()
+              }, { get: function(o, k) { get.push(k); return o[k]; }});
+            try {
+              // This will throw, since it will have true for both ""get"" and ""value"",
+              // but not before performing a Get on every property.
+              Object.defineProperty({}, ""foo"", p);
+            } catch(e) {
+              return get + '';
+            }
+            return 'did not fail as expected'";
+
+        var engine = new Engine();
+        Assert.Equal("enumerable,configurable,value,writable,get,set", engine.Evaluate(Script));
+    }
+
+    [Fact]
+    public void DefineProperties()
+    {
+        const string Script = @"
+            // Object.defineProperties -> Get -> [[Get]]
+            var get = [];
+            var p = new Proxy({foo:{}, bar:{}}, { get: function(o, k) { get.push(k); return o[k]; }});
+            Object.defineProperties({}, p);
+            return get + '';";
+
+        var engine = new Engine();
+        Assert.Equal("foo,bar", engine.Evaluate(Script));
     }
 }

Jint/Native/Array/ArrayConstructor.cs

@@ -73,7 +73,7 @@ namespace Jint.Native.Array
                 return _realm.Intrinsics.Array.ArrayCreate(0);
             }
 
-            if (objectInstance is IObjectWrapper wrapper && wrapper.Target is IEnumerable enumerable)
+            if (objectInstance is IObjectWrapper { Target: IEnumerable enumerable })
             {
                 return ConstructArrayFromIEnumerable(enumerable);
             }
@@ -134,7 +134,7 @@ namespace Jint.Native.Array
             for (uint i = 0; i < length; i++)
             {
                 JsValue jsValue;
-                source.TryGetValue(i, out var value);
+                var value = source.Get(i);
                 if (!ReferenceEquals(callable, null))
                 {
                     args![0] = value;

Jint/Native/Array/ArrayPrototype.cs

@@ -1204,7 +1204,10 @@ namespace Jint.Native.Array
             return o.Target;
         }
 
-        internal JsValue Join(JsValue thisObj, JsValue[] arguments)
+        /// <summary>
+        /// https://tc39.es/ecma262/#sec-array.prototype.join
+        /// </summary>
+        private JsValue Join(JsValue thisObj, JsValue[] arguments)
         {
             var separator = arguments.At(0);
             var o = ArrayOperations.For(_realm, thisObj);
@@ -1428,7 +1431,7 @@ namespace Jint.Native.Array
         /// </summary>
         public JsValue Push(JsValue thisObject, JsValue[] arguments)
         {
-            if (thisObject is ArrayInstance arrayInstance && arrayInstance.CanUseFastAccess)
+            if (thisObject is ArrayInstance { CanUseFastAccess: true } arrayInstance)
             {
                 return arrayInstance.Push(arguments);
             }

Jint/Native/Object/ObjectConstructor.cs

@@ -318,10 +318,10 @@ namespace Jint.Native.Object
         /// </summary>
         private JsValue DefineProperty(JsValue thisObject, JsValue[] arguments)
         {
-            var o = arguments.At(0) as ObjectInstance;
-            if (o is null)
+            if (arguments.At(0) is not ObjectInstance o)
             {
                 ExceptionHelper.ThrowTypeError(_realm, "Object.defineProperty called on non-object");
+                return null;
             }
 
             var p = arguments.At(1);
@@ -329,6 +329,7 @@ namespace Jint.Native.Object
 
             var attributes = arguments.At(2);
             var desc = PropertyDescriptor.ToPropertyDescriptor(_realm, attributes);
+
             o.DefinePropertyOrThrow(name, desc);
 
             return arguments.At(0);
@@ -366,7 +367,7 @@ namespace Jint.Native.Object
                     continue;
                 }
 
-                var descObj = props.UnwrapJsValue(propDesc);
+                var descObj = props.Get(nextKey);
                 var desc = PropertyDescriptor.ToPropertyDescriptor(_realm, descObj);
                 descriptors.Add(new KeyValuePair<JsValue, PropertyDescriptor>(nextKey, desc));
             }

Jint/Runtime/Descriptors/PropertyDescriptor.cs

@@ -233,76 +233,103 @@ namespace Jint.Runtime.Descriptors
                 return null;
             }
 
-            var getProperty = obj.GetProperty(CommonProperties.Get);
-            var hasGetProperty = getProperty != Undefined;
-            var setProperty = obj.GetProperty(CommonProperties.Set);
-            var hasSetProperty = setProperty != Undefined;
+            bool? enumerable = null;
+            var hasEnumerable = obj.HasProperty(CommonProperties.Enumerable);
+            if (hasEnumerable)
+            {
+                enumerable = TypeConverter.ToBoolean(obj.Get(CommonProperties.Enumerable));
+            }
 
-            if ((obj.HasProperty(CommonProperties.Value) || obj.HasProperty(CommonProperties.Writable)) &&
-                (hasGetProperty || hasSetProperty))
+            bool? configurable = null;
+            var hasConfigurable = obj.HasProperty(CommonProperties.Configurable);
+            if (hasConfigurable)
             {
-                ExceptionHelper.ThrowTypeError(realm);
+                configurable = TypeConverter.ToBoolean(obj.Get(CommonProperties.Configurable));
+            }
+
+            JsValue? value = null;
+            var hasValue = obj.HasProperty(CommonProperties.Value);
+            if (hasValue)
+            {
+                value = obj.Get(CommonProperties.Value);
+            }
+
+            bool? writable = null;
+            var hasWritable = obj.HasProperty(CommonProperties.Writable);
+            if (hasWritable)
+            {
+                writable = TypeConverter.ToBoolean(obj.Get(CommonProperties.Writable));
+            }
+
+            JsValue? get = null;
+            var hasGet = obj.HasProperty(CommonProperties.Get);
+            if (hasGet)
+            {
+                get = obj.Get(CommonProperties.Get);
+            }
+
+            JsValue? set = null;
+            var hasSet = obj.HasProperty(CommonProperties.Set);
+            if (hasSet)
+            {
+                set = obj.Get(CommonProperties.Set);
             }
 
-            var desc = hasGetProperty || hasSetProperty
+            if ((hasValue || hasWritable) && (hasGet || hasSet))
+            {
+                ExceptionHelper.ThrowTypeError(realm, "Invalid property descriptor. Cannot both specify accessors and a value or writable attribute");
+            }
+
+            var desc = hasGet || hasSet
                 ? new GetSetPropertyDescriptor(null, null, PropertyFlag.None)
                 : new PropertyDescriptor(PropertyFlag.None);
 
-            var enumerableProperty = obj.GetProperty(CommonProperties.Enumerable);
-            if (enumerableProperty != Undefined)
+            if (hasEnumerable)
             {
-                desc.Enumerable = TypeConverter.ToBoolean(obj.UnwrapJsValue(enumerableProperty));
+                desc.Enumerable = enumerable!.Value;
                 desc.EnumerableSet = true;
             }
 
-            var configurableProperty = obj.GetProperty(CommonProperties.Configurable);
-            if (configurableProperty != Undefined)
+            if (hasConfigurable)
             {
-                desc.Configurable = TypeConverter.ToBoolean(obj.UnwrapJsValue(configurableProperty));
+                desc.Configurable = configurable!.Value;
                 desc.ConfigurableSet = true;
             }
 
-            var valueProperty = obj.GetProperty(CommonProperties.Value);
-            if (valueProperty != Undefined)
+            if (hasValue)
             {
-                desc.Value = obj.UnwrapJsValue(valueProperty);
+                desc.Value = value!;
             }
 
-            var writableProperty = obj.GetProperty(CommonProperties.Writable);
-            if (writableProperty != Undefined)
+            if (hasWritable)
             {
-                desc.Writable = TypeConverter.ToBoolean(obj.UnwrapJsValue(writableProperty));
+                desc.Writable = TypeConverter.ToBoolean(writable!.Value);
                 desc.WritableSet = true;
             }
 
-            if (hasGetProperty)
+            if (hasGet)
             {
-                var getter = obj.UnwrapJsValue(getProperty);
-                if (!getter.IsUndefined() && getter.TryCast<ICallable>() == null)
+                if (!get!.IsUndefined() && get!.TryCast<ICallable>() == null)
                 {
                     ExceptionHelper.ThrowTypeError(realm);
                 }
 
-                ((GetSetPropertyDescriptor) desc).SetGet(getter);
+                ((GetSetPropertyDescriptor) desc).SetGet(get!);
             }
 
-            if (hasSetProperty)
+            if (hasSet)
             {
-                var setter = obj.UnwrapJsValue(setProperty);
-                if (!setter.IsUndefined() && setter.TryCast<ICallable>() == null)
+                if (!set!.IsUndefined() && set!.TryCast<ICallable>() is null)
                 {
                     ExceptionHelper.ThrowTypeError(realm);
                 }
 
-                ((GetSetPropertyDescriptor) desc).SetSet(setter);
+                ((GetSetPropertyDescriptor) desc).SetSet(set!);
             }
 
-            if (!ReferenceEquals(desc.Get, null))
+            if ((hasSet || hasGet) && (hasValue || hasWritable))
             {
-                if (!ReferenceEquals(desc.Value, null) || desc.WritableSet)
-                {
-                    ExceptionHelper.ThrowTypeError(realm);
-                }
+                ExceptionHelper.ThrowTypeError(realm);
             }
 
             return desc;

Jint/Runtime/TypeConverter.cs

@@ -912,7 +912,7 @@ namespace Jint.Runtime
         }
 
         /// <summary>
-        /// http://www.ecma-international.org/ecma-262/6.0/#sec-tostring
+        /// https://tc39.es/ecma262/#sec-tostring
         /// </summary>
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         public static string ToString(JsValue o)