Improve Proxy conformance and support CLR interop better (#1645)

provide detailed message for exceptions in JsProxy
do not use ReferenceEquals on data property value comparing
improve `JsValue.SameValue` to compare `ObjectWrapper`
add a new test `ProxyHandlerGetDataPropertyShouldNotCheckClrType` for `JsString` and `ConcatenatedString`

Jint.Tests/Runtime/ProxyTests.cs

@@ -1,4 +1,7 @@
-namespace Jint.Tests.Runtime;
+using Jint.Native.Error;
+using Jint.Runtime;
+
+namespace Jint.Tests.Runtime;
 
 public class ProxyTests
 {
@@ -183,4 +186,240 @@ public class ProxyTests
 
         Assert.True(_engine.Evaluate(Script).AsBoolean());
     }
+
+    [Fact]
+    public void ProxyHandlerGetDataPropertyShouldNotUseReferenceEquals()
+    {
+        // There are two JsString which should be treat as same value,
+        // but they are not ReferenceEquals.
+        _engine.Execute("""
+            let o = Object.defineProperty({}, 'value', {
+              configurable: false,
+              value: 'in',
+            });
+            const handler = {
+              get(target, property, receiver) {
+                return 'Jint'.substring(1,3);
+              }
+            };
+            let p = new Proxy(o, handler);
+            let pv = p.value;
+        """);
+    }
+
+    [Fact]
+    public void ProxyHandlerGetDataPropertyShouldNotCheckClrType()
+    {
+        // There are a JsString and a ConcatenatedString which should be treat as same value,
+        // but they are different CLR Type.
+        _engine.Execute("""
+            let o = Object.defineProperty({}, 'value', {
+              configurable: false,
+              value: 'Jint',
+            });
+            const handler = {
+              get(target, property, receiver) {
+                return 'Ji'.concat('nt');
+              }
+            };
+            let p = new Proxy(o, handler);
+            let pv = p.value;
+        """);
+    }
+
+    class TestClass
+    {
+        public static readonly TestClass Instance = new TestClass();
+        public string StringValue => "StringValue";
+        public int IntValue => 42424242; // avoid small numbers cache
+        public TestClass ObjectWrapper => Instance;
+    }
+
+    [Fact]
+    public void ProxyClrPropertyPrimitiveString()
+    {
+        _engine.SetValue("testClass", TestClass.Instance);
+        var result = _engine.Evaluate("""
+            const handler = {
+              get(target, property, receiver) {
+                return Reflect.get(target, property, receiver);
+              }
+            };
+            const p = new Proxy(testClass, handler);
+            return p.StringValue;
+        """);
+        Assert.Equal(TestClass.Instance.StringValue, result.AsString());
+    }
+
+    [Fact]
+    public void ProxyClrPropertyPrimitiveInt()
+    {
+        _engine.SetValue("testClass", TestClass.Instance);
+        var result = _engine.Evaluate("""
+            const handler = {
+              get(target, property, receiver) {
+                return Reflect.get(target, property, receiver);
+              }
+            };
+            const p = new Proxy(testClass, handler);
+            return p.IntValue;
+        """);
+        Assert.Equal(TestClass.Instance.IntValue, result.AsInteger());
+    }
+
+    [Fact]
+    public void ProxyClrPropertyObjectWrapper()
+    {
+        _engine.SetValue("testClass", TestClass.Instance);
+        var result = _engine.Evaluate("""
+            const handler = {
+              get(target, property, receiver) {
+                return Reflect.get(target, property, receiver);
+              }
+            };
+            const p = new Proxy(testClass, handler);
+            return p.ObjectWrapper;
+        """);
+    }
+
+    private static ErrorPrototype TypeErrorPrototype(Engine engine)
+        => engine.Realm.Intrinsics.TypeError.PrototypeObject;
+
+    private static void AssertJsTypeError(Engine engine, JavaScriptException ex, string msg)
+    {
+        Assert.Same(TypeErrorPrototype(engine), ex.Error.AsObject().Prototype);
+        Assert.Equal(msg, ex.Message);
+    }
+
+    // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy/Proxy/get#invariants
+    // The value reported for a property must be the same as
+    // the value ofthe corresponding target object property,
+    // if the target object property is
+    // a non-writable, non-configurable own data property.
+    [Fact]
+    public void ProxyHandlerGetInvariantsDataPropertyReturnsDifferentValue()
+    {
+        _engine.Execute("""
+            let o = Object.defineProperty({}, 'value', {
+              writable: false,
+              configurable: false,
+              value: 42,
+            });
+            const handler = {
+              get(target, property, receiver) {
+                return 32;
+              }
+            };
+            let p = new Proxy(o, handler);
+        """);
+        var ex = Assert.Throws<JavaScriptException>(() => _engine.Evaluate("p.value"));
+        AssertJsTypeError(_engine, ex, "'get' on proxy: property 'value' is a read-only and non-configurable data property on the proxy target but the proxy did not return its actual value (expected '42' but got '32')");
+    }
+
+    // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy/Proxy/get#invariants
+    // The value reported for a property must be undefined,
+    // if the corresponding target object property is
+    // a non-configurable own accessor property
+    // that has undefined as its [[Get]] attribute.
+    [Fact]
+    public void ProxyHandlerGetInvariantsAccessorPropertyWithoutGetButReturnsValue()
+    {
+        _engine.Execute("""
+            let o = Object.defineProperty({}, 'value', {
+              configurable: false,
+              set() {},
+            });
+            const handler = {
+              get(target, property, receiver) {
+                return 32;
+              }
+            };
+            let p = new Proxy(o, handler);
+        """);
+        var ex = Assert.Throws<JavaScriptException>(() => _engine.Evaluate("p.value"));
+        AssertJsTypeError(_engine, ex, "'get' on proxy: property 'value' is a non-configurable accessor property on the proxy target and does not have a getter function, but the trap did not return 'undefined' (got '32')");
+    }
+
+    private const string ScriptProxyHandlerSetInvariantsDataPropertyImmutable = """
+        let o = Object.defineProperty({}, 'value', {
+          writable: false,
+          configurable: false,
+          value: 42,
+        });
+        const handler = {
+          set(target, property, value, receiver) {
+            return true;
+          }
+        };
+        let p = new Proxy(o, handler);
+    """;
+
+    // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy/Proxy/set#invariants
+    // Cannot change the value of a property to be different from
+    // the value of the corresponding target object property,
+    // if the corresponding target object property is
+    // a non-writable, non-configurable data property.
+    [Fact]
+    public void ProxyHandlerSetInvariantsDataPropertyImmutableChangeValue()
+    {
+        _engine.Execute(ScriptProxyHandlerSetInvariantsDataPropertyImmutable);
+        var ex = Assert.Throws<JavaScriptException>(() => _engine.Evaluate("p.value = 32"));
+        AssertJsTypeError(_engine, ex, "'set' on proxy: trap returned truish for property 'value' which exists in the proxy target as a non-configurable and non-writable data property with a different value");
+    }
+
+    [Fact]
+    public void ProxyHandlerSetInvariantsDataPropertyImmutableSetSameValue()
+    {
+        _engine.Execute(ScriptProxyHandlerSetInvariantsDataPropertyImmutable);
+        _engine.Evaluate("p.value = 42");
+    }
+
+    // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy/Proxy/set#invariants
+    // Cannot set the value of a property, 
+    // if the corresponding target object property is
+    // a non-configurable accessor property
+    // that has undefined as its [[Set]] attribute.
+    [Fact]
+    public void ProxyHandlerSetInvariantsAccessorPropertyWithoutSetChange()
+    {
+        _engine.Execute("""
+            let o = Object.defineProperty({}, 'value', {
+              configurable: false,
+              get() { return 42; },
+            });
+            const handler = {
+              set(target, property, value, receiver) {
+                return true;
+              }
+            };
+            let p = new Proxy(o, handler);
+        """);
+        var ex = Assert.Throws<JavaScriptException>(() => _engine.Evaluate("p.value = 42"));
+        AssertJsTypeError(_engine, ex, "'set' on proxy: trap returned truish for property 'value' which exists in the proxy target as a non-configurable and non-writable accessor property without a setter");
+    }
+
+    // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy/Proxy/set#invariants
+    // In strict mode, a false return value from the set() handler
+    // will throw a TypeError exception.
+    [Fact]
+    public void ProxyHandlerSetInvariantsReturnsFalseInStrictMode()
+    {
+        var ex = Assert.Throws<JavaScriptException>(() => _engine.Evaluate("""
+            'use strict';
+            let p = new Proxy({}, { set: () => false });
+            p.value = 42;
+        """));
+        // V8: "'set' on proxy: trap returned falsish for property 'value'",
+        AssertJsTypeError(_engine, ex, "Cannot assign to read only property 'value' of [object Object]");
+    }
+
+    [Fact]
+    public void ProxyHandlerSetInvariantsReturnsFalseInNonStrictMode()
+    {
+        _engine.Evaluate("""
+            // 'use strict';
+            let p = new Proxy({}, { set: () => false });
+            p.value = 42;
+        """);
+    }
 }

Jint/Native/JsValue.cs

@@ -9,6 +9,7 @@ using Jint.Native.Number;
 using Jint.Native.Object;
 using Jint.Native.Symbol;
 using Jint.Runtime;
+using Jint.Runtime.Interop;
 
 namespace Jint.Native
 {
@@ -465,6 +466,11 @@ namespace Jint.Native
 
         internal static bool SameValue(JsValue x, JsValue y)
         {
+            if (ReferenceEquals(x, y))
+            {
+                return true;
+            }
+
             var typea = x.Type;
             var typeb = y.Type;
 
@@ -510,8 +516,15 @@ namespace Jint.Native
                     return true;
                 case Types.Symbol:
                     return x == y;
+                case Types.Object:
+                    if (x is ObjectWrapper xo && y is ObjectWrapper yo)
+                    {
+                        return ReferenceEquals(xo.Target, yo.Target)
+                            && xo.ClrType == yo.ClrType;
+                    }
+                    return false;
                 default:
-                    return ReferenceEquals(x, y);
+                    return false;
             }
         }
 

Jint/Native/Proxy/JsProxy.cs

@@ -2,6 +2,7 @@ using Jint.Native.Function;
 using Jint.Native.Object;
 using Jint.Runtime;
 using Jint.Runtime.Descriptors;
+using Jint.Runtime.Interop;
 
 namespace Jint.Native.Proxy
 {
@@ -134,13 +135,21 @@ namespace Jint.Native.Proxy
             var targetDesc = target.GetOwnProperty(property);
             if (targetDesc != PropertyDescriptor.Undefined)
             {
-                if (targetDesc.IsDataDescriptor() && !targetDesc.Configurable && !targetDesc.Writable && !ReferenceEquals(result, targetDesc._value))
+                if (targetDesc.IsDataDescriptor())
                 {
-                   ExceptionHelper.ThrowTypeError(_engine.Realm);
+                    var targetValue = targetDesc.Value;
+                    if (!targetDesc.Configurable && !targetDesc.Writable && !SameValue(result, targetValue))
+                    {
+                        ExceptionHelper.ThrowTypeError(_engine.Realm, $"'get' on proxy: property '{property}' is a read-only and non-configurable data property on the proxy target but the proxy did not return its actual value (expected '{targetValue}' but got '{result}')");
+                    }
                 }
-                if (targetDesc.IsAccessorDescriptor() && !targetDesc.Configurable && (targetDesc.Get ?? Undefined).IsUndefined() && !result.IsUndefined())
+
+                if (targetDesc.IsAccessorDescriptor())
                 {
-                   ExceptionHelper.ThrowTypeError(_engine.Realm, $"'get' on proxy: property '{property}' is a non-configurable accessor property on the proxy target and does not have a getter function, but the trap did not return 'undefined' (got '{result}')");
+                    if (!targetDesc.Configurable && (targetDesc.Get ?? Undefined).IsUndefined() && !result.IsUndefined())
+                    {
+                        ExceptionHelper.ThrowTypeError(_engine.Realm, $"'get' on proxy: property '{property}' is a non-configurable accessor property on the proxy target and does not have a getter function, but the trap did not return 'undefined' (got '{result}')");
+                    }
                 }
             }
 
@@ -152,7 +161,7 @@ namespace Jint.Native.Proxy
         /// </summary>
         public override List<JsValue> GetOwnPropertyKeys(Types types = Types.None | Types.String | Types.Symbol)
         {
-            if (!TryCallHandler(TrapOwnKeys, new JsValue[] { _target }, out var result))
+            if (!TryCallHandler(TrapOwnKeys, new[] { _target }, out var result))
             {
                 return _target.GetOwnPropertyKeys(types);
             }
@@ -321,14 +330,15 @@ namespace Jint.Native.Proxy
                 return false;
             }
 
-            var targetDesc  = _target.GetOwnProperty(property);
+            var targetDesc = _target.GetOwnProperty(property);
             if (targetDesc != PropertyDescriptor.Undefined)
             {
                 if (targetDesc.IsDataDescriptor() && !targetDesc.Configurable && !targetDesc.Writable)
                 {
-                    if (targetDesc.Value != value)
+                    var targetValue = targetDesc.Value;
+                    if (!SameValue(targetValue, value))
                     {
-                        ExceptionHelper.ThrowTypeError(_engine.Realm);
+                        ExceptionHelper.ThrowTypeError(_engine.Realm, $"'set' on proxy: trap returned truish for property '{property}' which exists in the proxy target as a non-configurable and non-writable data property with a different value");
                     }
                 }
 
@@ -336,7 +346,7 @@ namespace Jint.Native.Proxy
                 {
                     if ((targetDesc.Set ?? Undefined).IsUndefined())
                     {
-                        ExceptionHelper.ThrowTypeError(_engine.Realm);
+                        ExceptionHelper.ThrowTypeError(_engine.Realm, $"'set' on proxy: trap returned truish for property '{property}' which exists in the proxy target as a non-configurable and non-writable accessor property without a setter");
                     }
                 }
             }
@@ -405,7 +415,7 @@ namespace Jint.Native.Proxy
         /// </summary>
         public override bool HasProperty(JsValue property)
         {
-            if (!TryCallHandler(TrapHas, new [] { _target, TypeConverter.ToPropertyKey(property) }, out var jsValue))
+            if (!TryCallHandler(TrapHas, new[] { _target, TypeConverter.ToPropertyKey(property) }, out var jsValue))
             {
                 return _target.HasProperty(property);
             }
@@ -474,7 +484,7 @@ namespace Jint.Native.Proxy
         /// </summary>
         public override bool PreventExtensions()
         {
-            if (!TryCallHandler(TrapPreventExtensions, new JsValue[] { _target }, out var result))
+            if (!TryCallHandler(TrapPreventExtensions, new[] { _target }, out var result))
             {
                 return _target.PreventExtensions();
             }
@@ -496,7 +506,7 @@ namespace Jint.Native.Proxy
         {
             get
             {
-                if (!TryCallHandler(TrapIsExtensible, new JsValue[] { _target }, out var result))
+                if (!TryCallHandler(TrapIsExtensible, new[] { _target }, out var result))
                 {
                     return _target.Extensible;
                 }
@@ -516,7 +526,7 @@ namespace Jint.Native.Proxy
         /// </summary>
         protected internal override ObjectInstance? GetPrototypeOf()
         {
-            if (!TryCallHandler(TrapGetProtoTypeOf, new JsValue[] { _target }, out var handlerProto ))
+            if (!TryCallHandler(TrapGetProtoTypeOf, new[] { _target }, out var handlerProto))
             {
                 return _target.Prototype;
             }