Skip to content

Commit

Permalink
Improve Proxy conformance and support CLR interop better (#1645)
Browse files Browse the repository at this point in the history 
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`
  • Loading branch information
@viruscamp
viruscamp committed Oct 13, 2023
1 parent c905f53 commit 48d512f
Show file tree
Hide file tree
Showing 3 changed files with 277 additions and 15 deletions.
241 changes: 240 additions & 1 deletion Jint.Tests/Runtime/ProxyTests.cs
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
namespace Jint.Tests.Runtime;
using Jint.Native.Error;
using Jint.Runtime;

namespace Jint.Tests.Runtime;

public class ProxyTests
{
Expand Down Expand Up @@ -183,4 +186,240 @@ public void ConstructHandlerInvariant()

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;
""");
}
}
15 changes: 14 additions & 1 deletion Jint/Native/JsValue.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using Jint.Native.Object;
using Jint.Native.Symbol;
using Jint.Runtime;
using Jint.Runtime.Interop;

namespace Jint.Native
{
Expand Down Expand Up @@ -465,6 +466,11 @@ internal virtual bool OrdinaryHasInstance(JsValue v)

internal static bool SameValue(JsValue x, JsValue y)
{
if (ReferenceEquals(x, y))
{
return true;
}

var typea = x.Type;
var typeb = y.Type;

Expand Down Expand Up @@ -510,8 +516,15 @@ internal static bool SameValue(JsValue x, JsValue y)
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;
}
}

Expand Down
36 changes: 23 additions & 13 deletions Jint/Native/Proxy/JsProxy.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
using Jint.Native.Object;
using Jint.Runtime;
using Jint.Runtime.Descriptors;
using Jint.Runtime.Interop;

namespace Jint.Native.Proxy
{
Expand Down Expand Up @@ -134,13 +135,21 @@ public override JsValue Get(JsValue property, JsValue receiver)
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}')");
}
}
}

Expand All @@ -152,7 +161,7 @@ public override JsValue Get(JsValue property, JsValue receiver)
/// </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);
}
Expand Down Expand Up @@ -321,22 +330,23 @@ public override bool Set(JsValue property, JsValue value, JsValue receiver)
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");
}
}

if (targetDesc.IsAccessorDescriptor() && !targetDesc.Configurable)
{
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");
}
}
}
Expand Down Expand Up @@ -405,7 +415,7 @@ private static bool IsCompatiblePropertyDescriptor(bool extensible, PropertyDesc
/// </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);
}
Expand Down Expand Up @@ -474,7 +484,7 @@ public override bool Delete(JsValue property)
/// </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();
}
Expand All @@ -496,7 +506,7 @@ public override bool Extensible
{
get
{
if (!TryCallHandler(TrapIsExtensible, new JsValue[] { _target }, out var result))
if (!TryCallHandler(TrapIsExtensible, new[] { _target }, out var result))
{
return _target.Extensible;
}
Expand All @@ -516,7 +526,7 @@ public override bool Extensible
/// </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;
}
Expand Down

0 comments on commit 48d512f

Please sign in to comment.