[Bug] Wrapping args in a Proxy throws an unintended assertion

[sandydoo]

🐞 Describe the Bug

If you wrap the args proxy in another proxy and try to get an undefined property, Ember throws an assertion.

Error: Assertion Failed: args proxies do not have real property descriptors, so you should never need to call getOwnPropertyDescriptor yourself. This code exists for enumerability, such as in for-in loops and Object.keys()

🔬 Minimal Reproduction

The repro app will display errors in the console.
https://github.com/sandydoo/ember-repro-args-proxy

let argsProxy = new Proxy(
  this.args,
  {
    get() {
      // Doesn't even matter what you do here.
      // You could return a static value and it will still error out .
      return Reflect.get(...arguments);
    }
  }
);

// If args = { definedProp: true }
argsProxy.definedProp   // => Returns `true`
argsProxy.undefinedProp // => Throws assertion

😕 Actual Behavior

It seems that, as part of its implementation, Proxy calls getOwnPropertyDescriptor on the target object. I don't have control over this 🤷‍♂️. In the case of args, getOwnPropertyDescriptor throws an error if you try to call it for an undefined prop. The purpose of the assertion seems to be to deter people from using getOwnPropertyDescriptor altogether, but since it's necessary for enumeration, it can only throw an error for undefined props.

ember.js/packages/@ember/-internals/glimmer/lib/component-managers/custom.ts

Lines 220 to 224 in e6c38ec

	getOwnPropertyDescriptor(_target, prop) {
	assert(
	'args proxies do not have real property descriptors, so you should never need to call getOwnPropertyDescriptor yourself. This code exists for enumerability, such as in for-in loops and Object.keys()',
	namedArgs[prop as string] !== undefined
	);

🤔 Expected Behavior

I should be able to transparently access properties on this.args even via a Proxy.

🌍 Environment

• Ember: 3.21.1
• OS: MacOS
• Browser: Firefox, Chrome, and Safari

🧠 Thoughts

The assertion strikes me as a bit problematic for a few reasons:

1. It doesn't account for the native behaviour of Proxy.
2. Since you can't tell who the callee is and the function is actually necessary in some cases, the assertion is thrown inconsistently. For example, I can call getOwnPropertyDescriptor for any defined prop without issues. I'm not sure of what use that would be though...

I would appreciate some feedback on this issue. Are proxies a no-go with args or can we live with the possibility of people calling getOwnPropertyDescriptor?

[lifeart]

Hi @sandydoo, could you mention real-life use case where you have to wrap args into proxy?

[sandydoo]

I'm trying to migrate ember-google-maps to Octane. I was hoping I could avoid converting this.args into a POJO every time a map component has to be updated. The goal is to filter/modify the arguments passed to the map component before sending them on to Google Maps. It works quite well, aside from accusing me of calling getOwnPropertyDescriptor...

[sandydoo]

It seems nobody else has run into this problem, so I’m going to close this issue as “likely too esoteric and niche” 😆.

For anyone reading this, here’s the TL;DR: don’t wrap this.args in a proxy.

[lifeart]

Hi @sandydoo, may it help you? glimmerjs/glimmer-vm#1304

[pzuraq]

So, the reason we put that assertion there at all was just to discourage people from attempting strange things like dynamically defining properties on args. It doesn't seem like your use case is all that strange, and I do think it should be supported and work in general.

What's odd to me is that you're triggering getOwnPropertyDescriptor just using Reflect.get? That's really strange, because that would be super non-performant. I don't think the spec would require that. Is something else attempting to get the property descriptor? Like, is some code you don't control trying to do Object.getOwnPropertyDescriptor on your proxy, which is then forwarding it to the args proxy?

In either case, I think we could relax this restriction.

[sandydoo]

I don't think the spec would require that. Is something else attempting to get the property descriptor? Like, is some code you don't control trying to do Object.getOwnPropertyDescriptor on your proxy, which is then forwarding it to the args proxy?

The thing is, you can replicate the behaviour like so:

let firstProxy = new Proxy(Object.create(null), {
  get() {
    console.log('checking the first proxy');
    return;
  },

  getOwnPropertyDescriptor() {
    console.log('I should not be called');
    return;
  },
});

let secondProxy = new Proxy(firstProxy, {
  get() {
    return Reflect.get(...arguments);
  },
});

secondProxy.undefinedProp;

Safari, Firefox...they both end up calling getOwnPropertyDescriptor.

The repro I made is essentially the same. If you run through the debugger step-by-step, you’ll notice how we suddenly jump to getOwnPropertyDescriptor in Ember’s proxy handler right before we’re supposed to return from my proxy’s get handler. The getOwnPropertyDescriptor on my proxy is never run.

That's really strange, because that would be super non-performant.

🤷 Could be fun to see what happens if you then give it some value in getOwnPropertyDescriptor...

Spoiler

Nothing fun happens. It just returns `undefined`. But, it does complain if you do something funny like set `configurable: false`, so it is actually checking what you return from `getOwnPropertyDescriptor`. Weird.

So, the reason we put that assertion there at all was just to discourage people from attempting strange things like dynamically defining properties on args.

Ah, that makes sense.

Hi @sandydoo, may it help you? glimmerjs/glimmer-vm#1304

@lifeart, thanks for keeping an eye out ❤️

[pzuraq]

Huh, really interesting... it must be something that the proxy is doing for some reason. I'll have to read the spec on that at some point.

I think we can remove that assertion for now though, and instead just return a blank/default property descriptor if the property doesn't exist.

[sandydoo]

Huh, really interesting... it must be something that the proxy is doing for some reason. I'll have to read the spec on that at some point.

Alright, alright, you got me curious 🤣 Now I have to look!

Here’s the relevant bit:

Because proxy objects permit the implementation of internal methods to be provided by arbitrary ECMAScript code, it is possible to define a proxy object whose handler methods violates the invariants defined in 6.1.7.3. Some of the internal method invariants defined in 6.1.7.3 are essential integrity invariants. These invariants are explicitly enforced by the proxy object internal methods specified in this section. An ECMAScript implementation must be robust in the presence of all possible invariant violations. — https://262.ecma-international.org/11.0/#sec-proxy-object-internal-methods-and-internal-slots

And here’s what Proxy actually does in the get handler. There’s our call to GetOwnProperty!

https://262.ecma-international.org/11.0/#sec-proxy-object-internal-methods-and-internal-slots-get-p-receiver

9.5.8 [[Get]] ( P, Receiver )

When the [[Get]] internal method of a Proxy exotic object O is called with property key P and ECMAScript language value Receiver, the following steps are taken:

    Assert: IsPropertyKey(P) is true.
    Let handler be O.[[ProxyHandler]].
    If handler is null, throw a TypeError exception.
    Assert: Type(handler) is Object.
    Let target be O.[[ProxyTarget]].
    Let trap be ? GetMethod(handler, "get").
    If trap is undefined, then
        Return ? target.[[Get]](P, Receiver).
    Let trapResult be ? Call(trap, handler, « target, P, Receiver »).
    Let targetDesc be ? target.[[GetOwnProperty]](P).
    If targetDesc is not undefined and targetDesc.[[Configurable]] is false, then
        If IsDataDescriptor(targetDesc) is true and targetDesc.[[Writable]] is false, then
            If SameValue(trapResult, targetDesc.[[Value]]) is false, throw a TypeError exception.
        If IsAccessorDescriptor(targetDesc) is true and targetDesc.[[Get]] is undefined, then
            If trapResult is not undefined, throw a TypeError exception.
    Return trapResult.

And here are those “invariants”:

https://262.ecma-international.org/11.0/#sec-invariants-of-the-essential-internal-methods

[[Get]] ( P, Receiver )
If P was previously observed as a non-configurable, non-writable own data property of the target with value V, then [[Get]] must return the SameValue as V.
If P was previously observed as a non-configurable own accessor property of the target whose [[Get]] attribute is undefined, the [[Get]] operation must return undefined.

[[GetOwnProperty]] ( P )
If P is described as a non-configurable, non-writable own data property, all future calls to [[GetOwnProperty]] ( P ) must return Property Descritor whose [[Value]] is SameValue as P's [[Value]] attribute.

Well, I guess you pay a price for that flexibility 🤷 Now I’m curious what could happen if Proxy could break these “essential integrity invariants”... What a rabbit hole 🙄

[patricklx]

i also had this issue. simple workaround if possible: dont set args as the proxy target.

const origArgs = context.args;
    return new Proxy({}, {
      get(target: any, p: string | symbol, receiver: any): any {
        if (p in origArgs) {
          return origArgs[p];
        }
        return defaultArgs[p];
      }
    });