Home object bug and implementation issues

[syg]

@camillobruni and I were looking at ApplyDecoratorsToElementDefinition and do not understand how MakeMethod and home objects are supposed to work.

• MakeMethod mutates the [[HomeObject]] on the function, so the function object returned by the decorator has its [[HomeObject]] overwritten.
• What is this supposed to do for Proxies?

Currently MakeMethod is only called from the parser, and the home object is lexically scoped. MDN explicitly calls out the non-rebinding nature of super. Decorators making this dynamically scoped doesn't seem right and makes super usage confusing. And what of direct eval?

Perhaps functions returned from method decorators shouldn't have a [[HomeObject]] at all, since super is kind of a static concept currently, and making it a dynamic concept is a phase mismatch.

Also implementing MakeMethod to be callable from decorators turns out to be kind of hard for V8 because V8 already exploits the lexically scoped nature of super. V8 doesn't have a [[HomeObject]] per function because that uses too much memory. Instead, for those methods that use super or have direct eval, we declare a special internal variable to hold the home object in the enclosing class scope. It is not clear how to implement this efficiently for returned functions from method decorators. For ordinary JS functions, scope surgery is messy and bug prone. For proxies, I have no idea what this is supposed to do.

EDIT: The more I think about it the more strongly I feel that functions returned by method decorators just shouldn't have a [[HomeObject]]. It should not be making a lexically scoped thing suddenly dynamically scoped.

[Jamesernator]

The more I think about it the more strongly I feel that functions returned by method decorators just shouldn't have a [[HomeObject]].

Well it should really just preserve any existing value. e.g. If a decorate returns a method from a different object, then [[HomeObject]] just stays pointing to that original object:

const oProto = {
    x: 10,
};

const o = {
    __proto__: oProto,
    method() {
        return super.x;
    }
};

function decorate() {
    return o.method;
}

class Foo {
    @decorate
    foo() {
    
    }
}

// prints 10
console.log(new Foo().foo());

(Under the proposed spec, presumably this is supposed to print undefined)

[syg]

Well it should really just preserve any existing value. e.g. If a decorate returns a method from a different object, then [[HomeObject]] just stays pointing to that original object:

Thanks for the correction. Yes, this is what I meant. I don't think any returned functions from decorators should get a new [[HomeObject]] different from what it originally was.

[pzuraq]

I think I agree, this may have been something I didn't fully understand the implications of when I was initially writing the spec text. I'll add an item to address this at the upcoming plenary.

[Jamesernator]

Something else to note, the usage of SetFunctionName in ApplyDecoratorsToElementDefinition is basically completely broken. The first line of SetFunctionName has an assertion:

1. Assert: F is an extensible object that does not have a "name" own property.

However by the time a function is returned from a decorator function it will already have a name (possibly just the empty string), so at present basically every method decorator would throw an error.

The purpose of SetFunctionName from what I can tell in the spec is purely for parsing, so it doesn't make sense to apply it to a runtime function object.

I would question if method (and getter/setter) decorators should really should be unconditionally overriding the name of methods at all. It's totally legal to return the same function from a decorator multiple times, in which case the name of the function would be based on the last time it was returned from a decorator. People might well want to preserve the original name, or even set names themselves (which is impossible if ApplyDecorators... uncondtionally sets it).

[ljharb]

Alternatively, SetFunctionName could be expanded to be useful for decorators as well by removing that assertion.

[syg]

Mind splitting out the SetFunctionName thing into a different thread? Seems separate.

[pzuraq]

This has been resolved, dynamic home object is no longer in the spec.