Skip to content
This repository has been archived by the owner on Jan 25, 2022. It is now read-only.

Problematic that classes are still unnamed in static initializers? #85

Closed
anba opened this issue Mar 5, 2018 · 24 comments
Closed

Problematic that classes are still unnamed in static initializers? #85

anba opened this issue Mar 5, 2018 · 24 comments

Comments

@anba
Copy link
Contributor

anba commented Mar 5, 2018

Class names are assigned after ClassDefinitionEvaluation has finished. So if the name property is accessed in static initializers, it must not be present and the default inherited value from Function.prototype is read when this.name is accessed. Is that a problem usability- or implementation-wise?

Simple test case which should print "true" two times:

class C {
    static foo = this.name;
}
print(C.name === "C");
print(C.foo === "");
@littledan
Copy link
Member

Good catch; this effect was unintentional. Would it be possible to reorganize things to move the name definition before the evaluation of static initializers? It looks like ClassDefinitionEvaluation is always followed by a definition of the name, so I don't see the problem with passing the name in as an argument and defining it a little bit sooner.

Are the current semantics reflected in current tests and implementations? How would the various alternatives (keeping the spec as is, or swapping the name assignment vs static field evaluation) be? cc @gsathya @spectranaut @xanlpz

Note, this bug would technically make more sense to be discussed in https://github.com/tc39/proposal-static-class-features/ since this repository does not include static fields anymore.

@anba
Copy link
Contributor Author

anba commented Mar 5, 2018

Are the current semantics reflected in current tests and implementations? How would the various alternatives (keeping the spec as is, or swapping the name assignment vs static field evaluation) be?

FWIW adding the name property at a different step is likely to be web-compatible, because 3 of 4 implementations are already defining name at a different time.

class C { static foo(){} }
C.bar = 0;
// Should print "length,prototype,foo,name,bar"
// SpiderMonkey: "prototype,foo,bar,length,name"
// JSC: "length,name,prototype,foo,bar"
// Chakra: "prototype,name,foo,bar,length"
print(Object.getOwnPropertyNames(C));

Note, this bug would technically make more sense to be discussed in https://github.com/tc39/proposal-static-class-features/ since this repository does not include static fields anymore.

Oops, yes it should have been created in the other repo.

@littledan
Copy link
Member

Hmm, I'm not sure if we even do want to move the name assignment sooner. Wouldn't we still be in bad shape in a case like this?

const C = class {
  static x = this.name;
}

@ljharb
Copy link
Member

ljharb commented May 1, 2018

I would expect an ordering of that to have no name defined; specifically, the inferred name wouldn’t be set until the entire class was finalized.

@littledan
Copy link
Member

@ljharb What gives you this intuition?

@ljharb
Copy link
Member

ljharb commented May 1, 2018

Because the class expression is a complete thing before it gets assigned to the LHS and the name inferrred - iow, my intuition is that the ordering goes: evaluate LHS, evaluate RHS, apply name inference if applicable.

I can see the argument that it’d be: evaluate LHS, evaluate LHS with the inferred name if applicable - but in that case, I’d indeed expect the name to be set throughout all parts of the body.

The latter does seem strictly more useful, though, now that i think about it.

@littledan
Copy link
Member

Hmm, revisiting this issue, I'm still a little bothered by these semantics. I'm wondering whether we should consider a change. I'd like to hear more from people who have been through implementing or testing this specification, who have probably thought through these sorts of interactions in detail, cc @jridgewell @rricard @gsathya @spectranaut @caitp @xanlpz

@caitp
Copy link

caitp commented Jul 9, 2018

in all cases where you have name name inference, the name is statically determined right?

Is there any reason why we couldn't just update it to be a static behaviour instead of a runtime behaviour?

let MyClass = class {
  ["name"]() {} // this is the only reason why we can't do this statically, right?
                // But, we could simply overwrite the inferred name property with
                // the method, right?
};

@littledan
Copy link
Member

@caitp Function name inference actually takes computed property names into account, e.g.,

> x = { ["z"]: class { } }
> x.z.name
"z"

@caitp
Copy link

caitp commented Jul 9, 2018

Right, I guess we're out of luck then :)

@rricard
Copy link
Member

rricard commented Jul 10, 2018

@littledan I have to admit I'm a bit struggling with that in my babel implementation. I'm currently refactoring thanks to @jridgewell comments but once I'm done with that I'll attempt those situations and I don't expect them to work right out of the box unfortunately...

@littledan
Copy link
Member

@rricard Which sort of aspects are you struggling with? Does one of these possible semantics seem more natural to you?

@rricard
Copy link
Member

rricard commented Jul 11, 2018

Well, in the case of static private fields (which I'm focusing on), my implementation would stupidly transform this:

class C {
  static #foo = this.bar;
  static bar = "baz";
}

to this:

class C {}
var _CStatics = {};
_CStatics._foo = this.bar;
C.bar = "baz";

which does not make any sense except if we agree that this is the context in which C is being declared. If we want to do the other semantic (if I understand correctly) I would have to do the following:

class C {}
var _CStatics = {};
C.bar = "baz";
_CStatics._foo = C.bar;

which means that I have to start to be very careful with ordering things, and honestly, it will make the codebase there a bit more complicated...

And I'm just talking about static private here (what I'm focusing on) but I'm pretty sure that the way babel handles public static will be similar.

So to sum up, implementation-wise, in the current state, I would expect this to assert true within babel:

this.name = "bar";

class C {
    static foo = this.name;
}
print(C.name === "C");
print(C.foo === "bar");

@nicolo-ribaudo
Copy link
Member

Doesn't this refer to C, like it would do in static methods?

@rricard
Copy link
Member

rricard commented Jul 11, 2018

Not currently in babel: repl example - in the console, you'll see true appear twice

@littledan
Copy link
Member

Function name inference is the kind of thing where I won't be very upset if the initial version of Babel isn't spec-compliant. However, my intuition differs from @ljharb 's and matches @anba 's on this question.

@littledan
Copy link
Member

littledan commented Oct 16, 2018

@gsathya in V8 has just landed @anba 's proposed semantics (after previously having semantics which were just inconsistent). Let's revisit this issue in the November 2018 TC39 meeting, and draft a PR (against static-class-features) before then.

@littledan
Copy link
Member

@rricard Apologies for the late response--I think you'll have to make sure that this within a static field initializer refers to the class, rather than the outer this value. The phenomenon comes up in more places that has nothing to do with the name, e.g., you could do the following:

class C {
  static x = 123;
  static y = this.x;  // Should be initialized to 123
}

@littledan
Copy link
Member

Hmm, it's hard to see how to refactor the spec to make this change. The spec sets the function name last because the evaluation of the whole class expression comes as a unit, and that would include static fields.

@anba
Copy link
Contributor Author

anba commented Dec 10, 2018

The changes made in this branch should be enough to ensure the class name is set as part of ClassDefinitionEvaluation, which should fix this issue and the one in tc39/proposal-decorators#168.

@littledan
Copy link
Member

@anba This change looks great! It should also help us get rid of the awkward [[IsAnonymousFunctionExpression]] (which I had no idea how to integrate with decorators).

Would you be interested in making a PR of it in ecma262? It would be interesting to hear with eshost how it compares to web reality with respect to its observable change in enumeration order (I think you previously reported that engines are not consistent here).

@anba
Copy link
Contributor Author

anba commented Dec 17, 2018

It should also help us get rid of the awkward [[IsAnonymousFunctionExpression]] (which I had no idea how to integrate with decorators).

Hmm, I'm not sure, it could be an orthogonal issue. Here's my current thought process.

I think we first need to decide if the name inference is a property of the class field or the class initializer. By that I mean if a class field decorator replaces the initializer function, does the previously inferred name still apply for the new initializer? Let's assume the name inference doesn't apply for that case, because the new initializer may return any value (an already named function or a primitive value or ...). That'd mean name inference is actually a property of the field initializer function and therefore should happen as part of the evaluation of the initializer.
In that case I'd change ClassFieldDefinitionEvaluation to store fieldName in a new internal slot of initializer. The internal slot is then later processed in Runtime Semantics: EvaluateBody:

  1. Assert: argumentsList is empty.
  2. If IsAnonymousFunctionDefinition(AssignmentExpression) is true, then
    a. Let name be functionObject.[[FieldName]].
    b. Let exprValue be the result of performing NamedEvaluation for |AssignmentExpression| with argument name.
  3. Else,
    a. Let exprRef be the result of evaluating AssignmentExpression.
    b. Let exprValue be ? GetValue(exprRef).
  4. Return Completion { [[Type]]: return, [[Value]]: exprValue, [[Target]]: empty }.

Or without the proposed NamedEvaluation operation:

  1. Assert: argumentsList is empty.
  2. Let exprRef be the result of evaluating AssignmentExpression.
  3. Let exprValue be ? GetValue(exprRef).
  4. If IsAnonymousFunctionDefinition(AssignmentExpression) is true, then
    a. Let name be functionObject.[[FieldName]].
    b. Let hasNameProperty be ? HasOwnProperty(exprValue, "name").
    c. If hasNameProperty is false, perform SetFunctionName(exprValue, name).
  5. Return Completion { [[Type]]: return, [[Value]]: exprValue, [[Target]]: empty }.

(NB: It looks like EvaluateBody in the current class fields proposal is missing the GetValue and Return-Completion handling.)

@anba
Copy link
Contributor Author

anba commented Dec 17, 2018

Would you be interested in making a PR of it in ecma262?

Filed tc39/ecma262#1372

@littledan
Copy link
Member

This issue is fixed, as tc39/ecma262#1372 has landed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants