Bug 206431 – [JSC] Add support for private class fields

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Caitlin Potter (:caitp)]

A separate bug for the 2nd part of https://bugs.webkit.org/show_bug.cgi?id=174212.

This implements private instance (non-static) class fields, using the framework created for public class fields. Currently, this contains a single new opcode (op_get_by_val_direct), which handles Symbol cells with PrivateSymbolImpls specially. In addition, a new flag is added to PutByValDirect, in order to distinguish between definition and assignment of private names.

[Caitlin Potter (:caitp)]

Created attachment 388066 [details]
Patch v6

[Ross Kirsling]

Comment on attachment 388066 [details]
Patch v6

View in context: https://bugs.webkit.org/attachment.cgi?id=388066&action=review

A handful of minor things from me.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1857
> +            // FIXME: only do this if there is an eval() within a nested scope --- otherwise it isn't needed.

Do we need a bug for this?

> Source/JavaScriptCore/parser/Parser.cpp:5016
> +                if (Options::useClassFields() && match(PRIVATENAME)) {

If we're ensuring that PRIVATENAME is only lexed with useClassFields, I don't think we should need to guard instances of `match(PRIVATENAME)`.

> Source/JavaScriptCore/parser/Parser.h:1244
> +            ASSERT(i < m_scopeStack.size());

I think this assert should be moved before the while.

> Source/JavaScriptCore/parser/Parser.h:2036
> +    if (isEvalNode<ParsedNode>() && variablesUnderTDZ) {
> +        if (variablesUnderTDZ->privateNamesSize()) {

nit: Doesn't seem like these need to be two separate ifs.

> Source/JavaScriptCore/parser/VariableEnvironment.h:165
> +    typedef WTF::IteratorRange<PrivateNames::iterator> PrivateNamesRange;

nit: `using` is preferred over `typedef` in new code.
(Except when you're adding to an existing list of 'em, which is probably fine...)

> Source/JavaScriptCore/parser/VariableEnvironment.h:190
> +        ASSERT(privateNamesSize() > 0);

Do you think this is better than an early out (kind of like what you've got over in SymbolTable)? Seems unfortunate that you have to guard all the callsites.

> Source/JavaScriptCore/parser/VariableEnvironment.h:261
> +        auto it = m_rareData->m_privateNames.find(impl);
> +        if (it == m_rareData->m_privateNames.end())
> +            it = m_rareData->m_privateNames.add(impl, PrivateNameEntry(0)).iterator;
> +        return it->value;

WTF::HashMap::add says "Does nothing if the key is already present." so I don't think you need to perform the `find` here?

> Source/JavaScriptCore/runtime/JSScope.cpp:343
> +        if (symbolTable->hasPrivateNames()) {
> +            for (auto name : symbolTable->privateNames())

Seems like this one should work without the guard? Unless it's known to be faster...

[Caitlin Potter (:caitp)]

Comment on attachment 388066 [details]
Patch v6

View in context: https://bugs.webkit.org/attachment.cgi?id=388066&action=review

I've addressed the parsing comments and some of the runtime comments, the fixes should be in the next patch --- but I'm not 100% sure about some of the runtime fixes, particularly getting rid of the guards in various methods.

If some questions are answered about that, I'm happy to fix it.

>> Source/JavaScriptCore/parser/Parser.cpp:5016
>> +                if (Options::useClassFields() && match(PRIVATENAME)) {
> 
> If we're ensuring that PRIVATENAME is only lexed with useClassFields, I don't think we should need to guard instances of `match(PRIVATENAME)`.

Done.

There are some other similar cases, like the above

```
if (Options::useClassFields())
    semanticFailIfTrue(match(PRIVATENAME), "Cannot access private identifiers in an optional chain");
```

Do you think it's worth changing those too?

>> Source/JavaScriptCore/parser/Parser.h:1244
>> +            ASSERT(i < m_scopeStack.size());
> 
> I think this assert should be moved before the while.

Done

>> Source/JavaScriptCore/parser/Parser.h:2036
>> +        if (variablesUnderTDZ->privateNamesSize()) {
> 
> nit: Doesn't seem like these need to be two separate ifs.

Done

>> Source/JavaScriptCore/parser/VariableEnvironment.h:165
>> +    typedef WTF::IteratorRange<PrivateNames::iterator> PrivateNamesRange;
> 
> nit: `using` is preferred over `typedef` in new code.
> (Except when you're adding to an existing list of 'em, which is probably fine...)

Done

>> Source/JavaScriptCore/parser/VariableEnvironment.h:190
>> +        ASSERT(privateNamesSize() > 0);
> 
> Do you think this is better than an early out (kind of like what you've got over in SymbolTable)? Seems unfortunate that you have to guard all the callsites.

The reason for the guard is that we need to have `m_rareData` to get valid iterators for the WTF::IteratorRange. The reason for this is because of assertions which fire in HashTableIterator::checkValidity(). It doesnt seem to be a problem for release builds, but for development builds we seem to need this to avoid assertion failures if an IteratorRange is created from invalid iterators.

There may be a way around this, but I'm not certain what it is yet.

>> Source/JavaScriptCore/parser/VariableEnvironment.h:261
>> +        return it->value;
> 
> WTF::HashMap::add says "Does nothing if the key is already present." so I don't think you need to perform the `find` here?

Done

[Ross Kirsling]

(In reply to Caitlin Potter (:caitp) from comment #3)
> >> Source/JavaScriptCore/parser/Parser.cpp:5016
> >> +                if (Options::useClassFields() && match(PRIVATENAME)) {
> > 
> > If we're ensuring that PRIVATENAME is only lexed with useClassFields, I don't think we should need to guard instances of `match(PRIVATENAME)`.
> 
> Done.
> 
> There are some other similar cases, like the above
> 
> ```
> if (Options::useClassFields())
>     semanticFailIfTrue(match(PRIVATENAME), "Cannot access private
> identifiers in an optional chain");
> ```
> 
> Do you think it's worth changing those too?

Yeah, I'd vote for doing so.

> >> Source/JavaScriptCore/parser/VariableEnvironment.h:190
> >> +        ASSERT(privateNamesSize() > 0);
> > 
> > Do you think this is better than an early out (kind of like what you've got over in SymbolTable)? Seems unfortunate that you have to guard all the callsites.
> 
> The reason for the guard is that we need to have `m_rareData` to get valid
> iterators for the WTF::IteratorRange. The reason for this is because of
> assertions which fire in HashTableIterator::checkValidity(). It doesnt seem
> to be a problem for release builds, but for development builds we seem to
> need this to avoid assertion failures if an IteratorRange is created from
> invalid iterators.
> 
> There may be a way around this, but I'm not certain what it is yet.

Sure -- I guess I was hoping `PrivateNamesRange(nullptr, nullptr)` would just work, like with PrivateNameIteratorRange, but if it's not that simple, then no worries.

[Caitlin Potter (:caitp)]

Created attachment 388545 [details]
Patch v7

[Caitlin Potter (:caitp)]

Comment on attachment 388066 [details]
Patch v6

View in context: https://bugs.webkit.org/attachment.cgi?id=388066&action=review

Ready for the next round, imho

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:1857
>> +            // FIXME: only do this if there is an eval() within a nested scope --- otherwise it isn't needed.
> 
> Do we need a bug for this?

Done

>> Source/JavaScriptCore/runtime/JSScope.cpp:343
>> +            for (auto name : symbolTable->privateNames())
> 
> Seems like this one should work without the guard? Unless it's known to be faster...

It doesn't, it's the same HashTable iterator as in the other case :( I've simplified both instances of this and added a comment to explain why the guarded access is needed

[Ross Kirsling]

(In reply to Caitlin Potter (:caitp) from comment #6)
> >> Source/JavaScriptCore/runtime/JSScope.cpp:343
> >> +            for (auto name : symbolTable->privateNames())
> > 
> > Seems like this one should work without the guard? Unless it's known to be faster...
> 
> It doesn't, it's the same HashTable iterator as in the other case :( I've
> simplified both instances of this and added a comment to explain why the
> guarded access is needed

Thanks!

[Darin Adler]

Comment on attachment 388545 [details]
Patch v7

View in context: https://bugs.webkit.org/attachment.cgi?id=388545&action=review

> Source/JavaScriptCore/jit/JITOperations.cpp:865
> +        if (found == (bytecode.m_flags & PutByValThrowIfExists)) {

Windows warns here:

JavaScriptCore\jit/JITOperations.cpp(865,64): warning C4805: '==': unsafe mix of type 'bool' and type 'int' in operation

Could fix by adding !! prefix or != 0 suffix to the second half of the expression.

[Caio Lima]

Comment on attachment 388545 [details]
Patch v7

View in context: https://bugs.webkit.org/attachment.cgi?id=388545&action=review

I left some comments there.

> Source/JavaScriptCore/ChangeLog:24
> +        To illustrate, typical private field access will look like:

It would be nice have an explanation why we need to have this private symbol lookup on CL, otherwise we could simply use any `get_by_id` variant. It could be a short text explaining that different evaluations of class can't access private fields of each other.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:868
> +        generator.emitProfileType(privateName.get(), var, m_position, JSTextPosition(-1, m_position.offset, m_ident.length()));

I don't understand why we need `emitProfileType` here. I'm having a hard time to see how this would be reported to the user, and how this would be relevant.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:896
> +        generator.emitProfileType(privateName.get(), var, m_position, JSTextPosition(-1, m_position.offset, m_ident.length()));

ditto.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2032
> +        generator.emitProfileType(privateName.get(), var, m_position, JSTextPosition(-1, m_position.offset + ident.length(), -1));

ditto.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4441
> +        generator.emitProfileType(privateName.get(), var, m_position, JSTextPosition(-1, m_position.offset + m_ident->length(), -1));

ditto.

> Source/JavaScriptCore/jit/JITOperations.cpp:682
> +{

We should add test cases for the PrivateName path

> Source/JavaScriptCore/jit/JITOperations.cpp:719
> +void JIT_OPERATION operationPutByIdPutPrivateStrictOptimize(JSGlobalObject* globalObject, StructureStubInfo* stubInfo, EncodedJSValue encodedValue, EncodedJSValue encodedBase, UniquedStringImpl* uid)

Ditto.

> Source/JavaScriptCore/parser/Parser.h:488
> +    void copyPrivateNameUses(Scope& other)

See comments on `VariableEnvironment.h`

> Source/JavaScriptCore/parser/Parser.h:1238
> +    bool copyPrivateNameUsesToOuterScope()

I think it is better name this as `copyUndeclaredPrivateNameUsesToOuterScope`.

> Source/JavaScriptCore/parser/Parser.h:1249
> +        current->copyPrivateNameUses(m_scopeStack[i]);

See comments on `VariableEnvironment.h`

> Source/JavaScriptCore/parser/Parser.h:2034
> +    if (isEvalNode<ParsedNode>() && variablesUnderTDZ && variablesUnderTDZ->privateNamesSize()) {

Maybe it is a good time to rename `variablesUnderTDZ` to `privateNamesAndVariablesUnderTDZ`?

> Source/JavaScriptCore/parser/VariableEnvironment.h:223
> +    ALWAYS_INLINE void copyPrivateNameUses(VariableEnvironment& outer) const {

I think this would be better to understand if called `copyUndeclaredPrivateNames`, given we don't copy declared private names.

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1168
> +        });

nit: fix spacing.

[Caitlin Potter (:caitp)]

Created attachment 389815 [details]
Patch v8

[Caitlin Potter (:caitp)]

Comment on attachment 388545 [details]
Patch v7

View in context: https://bugs.webkit.org/attachment.cgi?id=388545&action=review

I've tried to address each of these.

>> Source/JavaScriptCore/ChangeLog:24
>> +        To illustrate, typical private field access will look like:
> 
> It would be nice have an explanation why we need to have this private symbol lookup on CL, otherwise we could simply use any `get_by_id` variant. It could be a short text explaining that different evaluations of class can't access private fields of each other.

```
Private field names are loaded from scope before their use. This prevents multiple evaluations
of the same class source from accessing each other's private fields, because the values of the
symbols loaded from the class scope would be distinct. This is required by the proposal text,
and is the key reason why we use ByVal lookups rather than ById lookups.
```

hope that covers it 👍

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:868
>> +        generator.emitProfileType(privateName.get(), var, m_position, JSTextPosition(-1, m_position.offset, m_ident.length()));
> 
> I don't understand why we need `emitProfileType` here. I'm having a hard time to see how this would be reported to the user, and how this would be relevant.

Based on the comments from part1, it's not. I guess there were more instances of this added in part 2. Cleaning them up now.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:896
>> +        generator.emitProfileType(privateName.get(), var, m_position, JSTextPosition(-1, m_position.offset, m_ident.length()));
> 
> ditto.

gone

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2032
>> +        generator.emitProfileType(privateName.get(), var, m_position, JSTextPosition(-1, m_position.offset + ident.length(), -1));
> 
> ditto.

done

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:4441
>> +        generator.emitProfileType(privateName.get(), var, m_position, JSTextPosition(-1, m_position.offset + m_ident->length(), -1));
> 
> ditto.

gone

>> Source/JavaScriptCore/jit/JITOperations.cpp:865
>> +        if (found == (bytecode.m_flags & PutByValThrowIfExists)) {
> 
> Windows warns here:
> 
> JavaScriptCore\jit/JITOperations.cpp(865,64): warning C4805: '==': unsafe mix of type 'bool' and type 'int' in operation
> 
> Could fix by adding !! prefix or != 0 suffix to the second half of the expression.

Done

>> Source/JavaScriptCore/parser/Parser.h:488
>> +    void copyPrivateNameUses(Scope& other)
> 
> See comments on `VariableEnvironment.h`

Done

>> Source/JavaScriptCore/parser/Parser.h:1238
>> +    bool copyPrivateNameUsesToOuterScope()
> 
> I think it is better name this as `copyUndeclaredPrivateNameUsesToOuterScope`.

Fair enough, done

>> Source/JavaScriptCore/parser/Parser.h:1249
>> +        current->copyPrivateNameUses(m_scopeStack[i]);
> 
> See comments on `VariableEnvironment.h`

Done

>> Source/JavaScriptCore/parser/Parser.h:2034
>> +    if (isEvalNode<ParsedNode>() && variablesUnderTDZ && variablesUnderTDZ->privateNamesSize()) {
> 
> Maybe it is a good time to rename `variablesUnderTDZ` to `privateNamesAndVariablesUnderTDZ`?

I'm not sure if the extra verbosity really adds much, especially since the private names are really just variables anyways. If everyone else feels it's worth it, then by all means.

>> Source/JavaScriptCore/parser/VariableEnvironment.h:223
>> +    ALWAYS_INLINE void copyPrivateNameUses(VariableEnvironment& outer) const {
> 
> I think this would be better to understand if called `copyUndeclaredPrivateNames`, given we don't copy declared private names.

fair enough, done

>> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1168
>> +        });
> 
> nit: fix spacing.

I know check-webkit-style complains, but this is consistent with the style of the rest of the file, so I don't think changing it is a good idea.

[Caio Lima]

Comment on attachment 389815 [details]
Patch v8

View in context: https://bugs.webkit.org/attachment.cgi?id=389815&action=review

> Source/JavaScriptCore/ChangeLog:27
> +        and is the key reason why we use ByVal lookups rather than ById lookups.

@Saam and @Yusuke, We are interested in receive feedback from this approach of loading PrivateNames from class scope. We are following the same approach to store private methods/accessors and it would be good to know if we can keep working in that direction on those patches.

> Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:139
> +    ECMAMode ecmaMode, PutKind putKind, PutByValFlags flags)

We need to change 32-bits as well. Right now it is causing compilation error on ARMv7 EWS.

[Caio Lima]

Comment on attachment 389815 [details]
Patch v8

View in context: https://bugs.webkit.org/attachment.cgi?id=389815&action=review

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1563
> +        osrExitPoint(size, metadata, return)

We are missing proper support for OSR return location of `op_get_by_val_direct`, but I think this can be included in a followup when we add support for DFG.

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1666
> +        getterSetterOSRExitReturnPoint(op_%opcodeName%, size)

ditto.

[Caitlin Potter (:caitp)]

Created attachment 391161 [details]
Patch v9

[Caitlin Potter (:caitp)]

Comment on attachment 389815 [details]
Patch v8

View in context: https://bugs.webkit.org/attachment.cgi?id=389815&action=review

>> Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:139
>> +    ECMAMode ecmaMode, PutKind putKind, PutByValFlags flags)
> 
> We need to change 32-bits as well. Right now it is causing compilation error on ARMv7 EWS.

Done, ARMv7 EWS is green now

[Ross Kirsling]

Comment on attachment 391161 [details]
Patch v9

View in context: https://bugs.webkit.org/attachment.cgi?id=391161&action=review

This already has my blessing, but here a couple final nits, of the nittiest variety. :P

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:731
> +        RegisterID* symbol = generator.emitCall(generator.finalDestination(nullptr, createPrivateSymbol.get()), createPrivateSymbol.get(), NoExpectedFunction, arguments, position(), position(), position(), DebuggableCall::No);

nit: Here and below, I think new code should use RefPtr<RegisterID> instead of RegisterID* for locals.

> Source/JavaScriptCore/parser/Lexer.cpp:1112
> +        bool isIdentifierStart = isPrivateName ? m_buffer16.size() == 1: !m_buffer16.size();

style nit: missing space before colon

[Caitlin Potter (:caitp)]

Created attachment 391542 [details]
Patch v10

[Yusuke Suzuki]

Will look in weekend.

[Caitlin Potter (:caitp)]

Created attachment 392723 [details]
Patch v11

periodic rebase

[Caitlin Potter (:caitp)]

Created attachment 393027 [details]
Patch v12

Added a new feature flag (JSC::Options::usePrivateClassFields)

[Caitlin Potter (:caitp)]

Created attachment 393054 [details]
Patch v13

[Caitlin Potter (:caitp)]

Created attachment 394108 [details]
Patch v14

Periodic rebase

[Caitlin Potter (:caitp)]

Created attachment 397021 [details]
Patch v15

[Saam Barati]

Comment on attachment 397021 [details]
Patch v15

View in context: https://bugs.webkit.org/attachment.cgi?id=397021&action=review

> Source/JavaScriptCore/ChangeLog:20
> +        subtle, but revolve around whether or not an exception is thrown during certain accesses.

maybe it's best to enumerate those cases here so folks reading the changelog can understand them

> Source/JavaScriptCore/ChangeLog:25
> +        Private field names are loaded from scope before their use. This prevents multiple evaluations
> +        of the same class source from accessing each other's private fields, because the values of the
> +        symbols loaded from the class scope would be distinct. This is required by the proposal text,
> +        and is the key reason why we use ByVal lookups rather than ById lookups.

Maybe also worth saying that we create the symbols during class creation

> Source/JavaScriptCore/ChangeLog:32
> +        resolve_scope      <scope=>, <currentScope>, "#x", GlobalProperty, 0
> +        get_from_scope     <symbol=>, <scope>, "#x", 1050624<DoNotThrowIfNotFound|GlobalProperty|NotInitialization>, 0, 0
> +        get_by_val_direct  <value=>, <receiver --- probably 'this'>, <symbol>

this is nice

> Source/JavaScriptCore/bytecode/PutByValFlags.h:3
> + * Copyright (C) 2019 Igalia S.L.

a lot of these should be 2020 now. Sorry for taking so long to review :-(

> Source/JavaScriptCore/bytecode/PutByValFlags.h:33
> +enum class PrivateFieldAccess : uint8_t {

name nit: This feels like the name should be something like PrivateFieldAccessKind

> Source/JavaScriptCore/bytecode/PutByValFlags.h:36
> +    Create,

I prefer this name over "add". I also like "define". See other comments on the "Add" naming.

> Source/JavaScriptCore/bytecode/PutKind.h:30
> +enum PutKind { Direct, NotDirect, DirectAddPrivate, DirectPutPrivate };

nit: I'm a fan of having the shared part of these names come before the non shared part, so I'd do:
DirectPrivateAdd and DirectPrivatePut

Also, see another comment I made about "add" feels like the wrong name.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2714
> +RegisterID* BytecodeGenerator::emitPrivateFieldAdd(RegisterID* base, RegisterID* property, RegisterID* value)

nit on naming: I feel like "add" is the less idiomatic way of doing this. Maybe "define" somehow is better to incorporate?

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:557
> +    for (; p; p = p->m_next) {

I'm a bit confused, if we're defining these properties first, what do we do in the loop that has this comment:
"Iterate over the remaining properties in the list."

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:882
> +        RefPtr<RegisterID> scope = generator.emitResolveScope(generator.newTemporary(), var);

no need for "newTemporary" here. Also, this use is slightly wrong if we relied on it, as we have no +1 ref to it.
(Also, weirdly, that function doesn't actually seem to store into dst...)

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2140
> +        RefPtr<RegisterID> scope = generator.emitResolveScope(generator.newTemporary(), var);

same comment here on newTemporary. I'm guessing this shows up in a few other places in this patch too.

> Source/JavaScriptCore/dfg/DFGCapabilities.cpp:300
> +    case op_get_by_val_direct: // FIXME: op_get_by_val_direct will need DFG/FTL support to ship class fields.

link to a bug here

Also, does the new version of PutByValDirect work in DFG/FTL?

> Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:156
> +    if (UNLIKELY(m_private != PrivateFieldAccess::None)) {

not a fan of this UNLIKELY here since once private fields are used who knows if this is unlikely or not

> Source/JavaScriptCore/jit/JITOperations.cpp:728
> +    bool found = baseObject->isProxy() ? JSObject::getOwnPropertySlot(baseObject, globalObject, ident, slot)
> +        : baseValue.getOwnPropertySlot(globalObject, ident, slot);

what's up with this proxy check? My guess is you don't want to call the Proxy hook, right? But this also skips DOMwindow.

Also worth thinking about and testing, what happens with "with" and walking the prototype chain w/ one of these symbols? Will we accidentally leak it to a proxy that way?

> Source/JavaScriptCore/jit/JITOperations.cpp:732
> +    if (putKind == DirectAddPrivate && found)
> +        throwException(globalObject, scope, createRedefinedPrivateNameError(globalObject));

What JS code reaches this?

> Source/JavaScriptCore/jit/JITOperations.cpp:738
> +    Structure* structure = 0;

nullptr

> Source/JavaScriptCore/jit/JITOperations.cpp:760
> +    RETURN_IF_EXCEPTION(scope, void());

can you just release the scope before calling putPrivateField?

> Source/JavaScriptCore/jit/JITOperations.cpp:775
> +    putPrivateField(vm, globalObject, callFrame, baseObject, identifier, value, DirectAddPrivate, [=](VM& vm, CodeBlock* codeBlock, Structure* structure, PutPropertySlot& putSlot, const Identifier& ident) {

I'd use a "&" lambda for consistency w/ below

> Source/JavaScriptCore/jit/JITOperations.cpp:779
> +        if (accessType != static_cast<AccessType>(stubInfo->accessType))
> +            return;

can we actually call a setter here? Feels like maybe this should be an assert?

> Source/JavaScriptCore/jit/JITOperations.cpp:784
> +    RETURN_IF_EXCEPTION(scope, void());

can you just release the scope before calling putPrivateField?

> Source/JavaScriptCore/jit/JITOperations.cpp:798
> +    RETURN_IF_EXCEPTION(scope, void());

can you just release the scope before calling putPrivateField?

> Source/JavaScriptCore/jit/JITOperations.cpp:822
> +    RETURN_IF_EXCEPTION(scope, void());

can you just release the scope before calling putPrivateField?

> Source/JavaScriptCore/jit/JITOperations.cpp:922
> +    if (property.isPrivateName()) {

Could this be conflated with a privateName from a builtin?

> Source/JavaScriptCore/jit/JITOperations.cpp:926
> +        PropertySlot slot(baseObject, PropertySlot::InternalMethodType::VMInquiry);

VMInquiry feels wrong here. Is this also to skip over proxy?

I feel like the invariant here is that private fields must end up in normal property storage. If my understanding is correct, maybe just add a helper for get/set private field on JSObject?

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1155
> +            if (found == bool(bytecode.m_flags.isPrivateFieldAdd()))

bool(...) not needed

> Source/JavaScriptCore/parser/Parser.cpp:3057
> +        semanticFailIfFalse(copyUndeclaredPrivateNamesToOuterScope(), "Cannot reference undeclared private names");

nit: maybe we can provide at least one of the names for a better error message? (Could be a follow-up)

> Source/JavaScriptCore/parser/Parser.cpp:4828
> +bool Parser<LexerType>::ensurePrivateNameUsed(const Identifier* ident)

name nit: this feels like the slightly wrong name given a boolean returns if it's valid or not, and the name contains "ensure" as a verb

Maybe just "usePrivateName"?

> Source/JavaScriptCore/parser/Parser.cpp:4836
> +    ScopeRef current = currentPrivateNameScope(&found);
> +    if (!found)
> +        return false;

nit: maybe just have this be "hasPrivateNameScope"?

> Source/JavaScriptCore/parser/Parser.h:887
> +    bool m_isPrivateNameScope;

do we really need this if we have m_isClassScope? AFAICT, they're always equal to each other.

> Source/JavaScriptCore/parser/Parser.h:1232
> +    ScopeRef currentPrivateNameScope(bool *found = 0)

"bool *found" => "bool* found"
"= 0" => "= nullptr"

> Source/JavaScriptCore/parser/VariableEnvironment.h:108
> +    enum Traits : uint16_t {

why uint16_t?

> Source/JavaScriptCore/parser/VariableEnvironment.h:172
> +        auto& meta = getOrAddPrivateName(identifier.get());

what is meta?

> Source/JavaScriptCore/parser/VariableEnvironment.h:185
> +            find(identifier)->value.setIsCaptured();

couldn't this be an assert that it's captured already? Maybe I'm missing something?

> Source/JavaScriptCore/parser/VariableEnvironment.h:209
> +    ALWAYS_INLINE void copyPrivateNames(VariableEnvironment& other) const

name nit: maybe "copyPrivateNamesTo"?

> Source/JavaScriptCore/parser/VariableEnvironment.h:223
> +    ALWAYS_INLINE void copyUndeclaredPrivateNames(VariableEnvironment& outer) const {

style nit: brace goes on following line
name nit: maybe also suffix of "To" here to method name?

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:316
> +static EncodedJSValue JSC_HOST_CALL createPrivateSymbol(JSGlobalObject* globalObject, CallFrame*)
> +{
> +    VM& vm = globalObject->vm();
> +    return JSValue::encode(Symbol::create(vm, PrivateSymbolImpl::createNullSymbol().get()));
> +}

Should we make this a bytecode instead of a host call? Maybe we can make it even define all the things in one go instead of repeated bytecode "invocations"?

(A follow-up is ok for that.)

That should speed up class creation w/ private fields by some amount.

Ideally, we'd inline this into the compiler, but it might be tricky to inline PrivateSymbolImpl::createNullSymbol, but I haven't checked exactly what that's doing.

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1205
> +            init.set(JSFunction::create(init.vm, jsCast<JSGlobalObject*>(init.owner), 0, String(), createPrivateSymbol));
> +        });

style nit: spacing is off here

> JSTests/stress/put-by-val-direct-putprivate.js:1
> +// TODO: //@ requireOptions("--usePrivateClassFields=1")

?

[Saam Barati]

Comment on attachment 397021 [details]
Patch v15

View in context: https://bugs.webkit.org/attachment.cgi?id=397021&action=review

> Source/JavaScriptCore/ChangeLog:20
> +        subtle, but revolve around whether or not an exception is thrown during certain accesses.

maybe it's best to enumerate those cases here so folks reading the changelog can understand them

> Source/JavaScriptCore/ChangeLog:25
> +        Private field names are loaded from scope before their use. This prevents multiple evaluations
> +        of the same class source from accessing each other's private fields, because the values of the
> +        symbols loaded from the class scope would be distinct. This is required by the proposal text,
> +        and is the key reason why we use ByVal lookups rather than ById lookups.

Maybe also worth saying that we create the symbols during class creation

> Source/JavaScriptCore/ChangeLog:32
> +        resolve_scope      <scope=>, <currentScope>, "#x", GlobalProperty, 0
> +        get_from_scope     <symbol=>, <scope>, "#x", 1050624<DoNotThrowIfNotFound|GlobalProperty|NotInitialization>, 0, 0
> +        get_by_val_direct  <value=>, <receiver --- probably 'this'>, <symbol>

this is nice

> Source/JavaScriptCore/bytecode/PutByValFlags.h:3
> + * Copyright (C) 2019 Igalia S.L.

a lot of these should be 2020 now. Sorry for taking so long to review :-(

> Source/JavaScriptCore/bytecode/PutByValFlags.h:33
> +enum class PrivateFieldAccess : uint8_t {

name nit: This feels like the name should be something like PrivateFieldAccessKind

> Source/JavaScriptCore/bytecode/PutByValFlags.h:36
> +    Create,

I prefer this name over "add". I also like "define". See other comments on the "Add" naming.

> Source/JavaScriptCore/bytecode/PutKind.h:30
> +enum PutKind { Direct, NotDirect, DirectAddPrivate, DirectPutPrivate };

nit: I'm a fan of having the shared part of these names come before the non shared part, so I'd do:
DirectPrivateAdd and DirectPrivatePut

Also, see another comment I made about "add" feels like the wrong name.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2714
> +RegisterID* BytecodeGenerator::emitPrivateFieldAdd(RegisterID* base, RegisterID* property, RegisterID* value)

nit on naming: I feel like "add" is the less idiomatic way of doing this. Maybe "define" somehow is better to incorporate?

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:557
> +    for (; p; p = p->m_next) {

I'm a bit confused, if we're defining these properties first, what do we do in the loop that has this comment:
"Iterate over the remaining properties in the list."

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:882
> +        RefPtr<RegisterID> scope = generator.emitResolveScope(generator.newTemporary(), var);

no need for "newTemporary" here. Also, this use is slightly wrong if we relied on it, as we have no +1 ref to it.
(Also, weirdly, that function doesn't actually seem to store into dst...)

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2140
> +        RefPtr<RegisterID> scope = generator.emitResolveScope(generator.newTemporary(), var);

same comment here on newTemporary. I'm guessing this shows up in a few other places in this patch too.

> Source/JavaScriptCore/dfg/DFGCapabilities.cpp:300
> +    case op_get_by_val_direct: // FIXME: op_get_by_val_direct will need DFG/FTL support to ship class fields.

link to a bug here

Also, does the new version of PutByValDirect work in DFG/FTL?

> Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:156
> +    if (UNLIKELY(m_private != PrivateFieldAccess::None)) {

not a fan of this UNLIKELY here since once private fields are used who knows if this is unlikely or not

> Source/JavaScriptCore/jit/JITOperations.cpp:728
> +    bool found = baseObject->isProxy() ? JSObject::getOwnPropertySlot(baseObject, globalObject, ident, slot)
> +        : baseValue.getOwnPropertySlot(globalObject, ident, slot);

what's up with this proxy check? My guess is you don't want to call the Proxy hook, right? But this also skips DOMwindow.

Also worth thinking about and testing, what happens with "with" and walking the prototype chain w/ one of these symbols? Will we accidentally leak it to a proxy that way?

> Source/JavaScriptCore/jit/JITOperations.cpp:732
> +    if (putKind == DirectAddPrivate && found)
> +        throwException(globalObject, scope, createRedefinedPrivateNameError(globalObject));

What JS code reaches this?

> Source/JavaScriptCore/jit/JITOperations.cpp:738
> +    Structure* structure = 0;

nullptr

> Source/JavaScriptCore/jit/JITOperations.cpp:760
> +    RETURN_IF_EXCEPTION(scope, void());

can you just release the scope before calling putPrivateField?

> Source/JavaScriptCore/jit/JITOperations.cpp:775
> +    putPrivateField(vm, globalObject, callFrame, baseObject, identifier, value, DirectAddPrivate, [=](VM& vm, CodeBlock* codeBlock, Structure* structure, PutPropertySlot& putSlot, const Identifier& ident) {

I'd use a "&" lambda for consistency w/ below

> Source/JavaScriptCore/jit/JITOperations.cpp:779
> +        if (accessType != static_cast<AccessType>(stubInfo->accessType))
> +            return;

can we actually call a setter here? Feels like maybe this should be an assert?

> Source/JavaScriptCore/jit/JITOperations.cpp:784
> +    RETURN_IF_EXCEPTION(scope, void());

can you just release the scope before calling putPrivateField?

> Source/JavaScriptCore/jit/JITOperations.cpp:798
> +    RETURN_IF_EXCEPTION(scope, void());

can you just release the scope before calling putPrivateField?

> Source/JavaScriptCore/jit/JITOperations.cpp:822
> +    RETURN_IF_EXCEPTION(scope, void());

can you just release the scope before calling putPrivateField?

> Source/JavaScriptCore/jit/JITOperations.cpp:922
> +    if (property.isPrivateName()) {

Could this be conflated with a privateName from a builtin?

> Source/JavaScriptCore/jit/JITOperations.cpp:926
> +        PropertySlot slot(baseObject, PropertySlot::InternalMethodType::VMInquiry);

VMInquiry feels wrong here. Is this also to skip over proxy?

I feel like the invariant here is that private fields must end up in normal property storage. If my understanding is correct, maybe just add a helper for get/set private field on JSObject?

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1155
> +            if (found == bool(bytecode.m_flags.isPrivateFieldAdd()))

bool(...) not needed

> Source/JavaScriptCore/parser/Parser.cpp:3057
> +        semanticFailIfFalse(copyUndeclaredPrivateNamesToOuterScope(), "Cannot reference undeclared private names");

nit: maybe we can provide at least one of the names for a better error message? (Could be a follow-up)

> Source/JavaScriptCore/parser/Parser.cpp:4828
> +bool Parser<LexerType>::ensurePrivateNameUsed(const Identifier* ident)

name nit: this feels like the slightly wrong name given a boolean returns if it's valid or not, and the name contains "ensure" as a verb

Maybe just "usePrivateName"?

> Source/JavaScriptCore/parser/Parser.cpp:4836
> +    ScopeRef current = currentPrivateNameScope(&found);
> +    if (!found)
> +        return false;

nit: maybe just have this be "hasPrivateNameScope"?

> Source/JavaScriptCore/parser/Parser.h:887
> +    bool m_isPrivateNameScope;

do we really need this if we have m_isClassScope? AFAICT, they're always equal to each other.

> Source/JavaScriptCore/parser/Parser.h:1232
> +    ScopeRef currentPrivateNameScope(bool *found = 0)

"bool *found" => "bool* found"
"= 0" => "= nullptr"

> Source/JavaScriptCore/parser/VariableEnvironment.h:108
> +    enum Traits : uint16_t {

why uint16_t?

> Source/JavaScriptCore/parser/VariableEnvironment.h:172
> +        auto& meta = getOrAddPrivateName(identifier.get());

what is meta?

> Source/JavaScriptCore/parser/VariableEnvironment.h:185
> +            find(identifier)->value.setIsCaptured();

couldn't this be an assert that it's captured already? Maybe I'm missing something?

> Source/JavaScriptCore/parser/VariableEnvironment.h:209
> +    ALWAYS_INLINE void copyPrivateNames(VariableEnvironment& other) const

name nit: maybe "copyPrivateNamesTo"?

> Source/JavaScriptCore/parser/VariableEnvironment.h:223
> +    ALWAYS_INLINE void copyUndeclaredPrivateNames(VariableEnvironment& outer) const {

style nit: brace goes on following line
name nit: maybe also suffix of "To" here to method name?

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:316
> +static EncodedJSValue JSC_HOST_CALL createPrivateSymbol(JSGlobalObject* globalObject, CallFrame*)
> +{
> +    VM& vm = globalObject->vm();
> +    return JSValue::encode(Symbol::create(vm, PrivateSymbolImpl::createNullSymbol().get()));
> +}

Should we make this a bytecode instead of a host call? Maybe we can make it even define all the things in one go instead of repeated bytecode "invocations"?

(A follow-up is ok for that.)

That should speed up class creation w/ private fields by some amount.

Ideally, we'd inline this into the compiler, but it might be tricky to inline PrivateSymbolImpl::createNullSymbol, but I haven't checked exactly what that's doing.

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1205
> +            init.set(JSFunction::create(init.vm, jsCast<JSGlobalObject*>(init.owner), 0, String(), createPrivateSymbol));
> +        });

style nit: spacing is off here

> JSTests/stress/put-by-val-direct-putprivate.js:1
> +// TODO: //@ requireOptions("--usePrivateClassFields=1")

?

[Saam Barati]

Comment on attachment 397021 [details]
Patch v15

View in context: https://bugs.webkit.org/attachment.cgi?id=397021&action=review

> Source/JavaScriptCore/ChangeLog:20
> +        subtle, but revolve around whether or not an exception is thrown during certain accesses.

maybe it's best to enumerate those cases here so folks reading the changelog can understand them

> Source/JavaScriptCore/ChangeLog:25
> +        Private field names are loaded from scope before their use. This prevents multiple evaluations
> +        of the same class source from accessing each other's private fields, because the values of the
> +        symbols loaded from the class scope would be distinct. This is required by the proposal text,
> +        and is the key reason why we use ByVal lookups rather than ById lookups.

Maybe also worth saying that we create the symbols during class creation

> Source/JavaScriptCore/ChangeLog:32
> +        resolve_scope      <scope=>, <currentScope>, "#x", GlobalProperty, 0
> +        get_from_scope     <symbol=>, <scope>, "#x", 1050624<DoNotThrowIfNotFound|GlobalProperty|NotInitialization>, 0, 0
> +        get_by_val_direct  <value=>, <receiver --- probably 'this'>, <symbol>

this is nice

> Source/JavaScriptCore/bytecode/PutByValFlags.h:3
> + * Copyright (C) 2019 Igalia S.L.

a lot of these should be 2020 now. Sorry for taking so long to review :-(

> Source/JavaScriptCore/bytecode/PutByValFlags.h:33
> +enum class PrivateFieldAccess : uint8_t {

name nit: This feels like the name should be something like PrivateFieldAccessKind

> Source/JavaScriptCore/bytecode/PutByValFlags.h:36
> +    Create,

I prefer this name over "add". I also like "define". See other comments on the "Add" naming.

> Source/JavaScriptCore/bytecode/PutKind.h:30
> +enum PutKind { Direct, NotDirect, DirectAddPrivate, DirectPutPrivate };

nit: I'm a fan of having the shared part of these names come before the non shared part, so I'd do:
DirectPrivateAdd and DirectPrivatePut

Also, see another comment I made about "add" feels like the wrong name.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2714
> +RegisterID* BytecodeGenerator::emitPrivateFieldAdd(RegisterID* base, RegisterID* property, RegisterID* value)

nit on naming: I feel like "add" is the less idiomatic way of doing this. Maybe "define" somehow is better to incorporate?

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:557
> +    for (; p; p = p->m_next) {

I'm a bit confused, if we're defining these properties first, what do we do in the loop that has this comment:
"Iterate over the remaining properties in the list."

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:882
> +        RefPtr<RegisterID> scope = generator.emitResolveScope(generator.newTemporary(), var);

no need for "newTemporary" here. Also, this use is slightly wrong if we relied on it, as we have no +1 ref to it.
(Also, weirdly, that function doesn't actually seem to store into dst...)

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2140
> +        RefPtr<RegisterID> scope = generator.emitResolveScope(generator.newTemporary(), var);

same comment here on newTemporary. I'm guessing this shows up in a few other places in this patch too.

> Source/JavaScriptCore/dfg/DFGCapabilities.cpp:300
> +    case op_get_by_val_direct: // FIXME: op_get_by_val_direct will need DFG/FTL support to ship class fields.

link to a bug here

Also, does the new version of PutByValDirect work in DFG/FTL?

> Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:156
> +    if (UNLIKELY(m_private != PrivateFieldAccess::None)) {

not a fan of this UNLIKELY here since once private fields are used who knows if this is unlikely or not

> Source/JavaScriptCore/jit/JITOperations.cpp:728
> +    bool found = baseObject->isProxy() ? JSObject::getOwnPropertySlot(baseObject, globalObject, ident, slot)
> +        : baseValue.getOwnPropertySlot(globalObject, ident, slot);

what's up with this proxy check? My guess is you don't want to call the Proxy hook, right? But this also skips DOMwindow.

Also worth thinking about and testing, what happens with "with" and walking the prototype chain w/ one of these symbols? Will we accidentally leak it to a proxy that way?

> Source/JavaScriptCore/jit/JITOperations.cpp:732
> +    if (putKind == DirectAddPrivate && found)
> +        throwException(globalObject, scope, createRedefinedPrivateNameError(globalObject));

What JS code reaches this?

> Source/JavaScriptCore/jit/JITOperations.cpp:738
> +    Structure* structure = 0;

nullptr

> Source/JavaScriptCore/jit/JITOperations.cpp:760
> +    RETURN_IF_EXCEPTION(scope, void());

can you just release the scope before calling putPrivateField?

> Source/JavaScriptCore/jit/JITOperations.cpp:775
> +    putPrivateField(vm, globalObject, callFrame, baseObject, identifier, value, DirectAddPrivate, [=](VM& vm, CodeBlock* codeBlock, Structure* structure, PutPropertySlot& putSlot, const Identifier& ident) {

I'd use a "&" lambda for consistency w/ below

> Source/JavaScriptCore/jit/JITOperations.cpp:779
> +        if (accessType != static_cast<AccessType>(stubInfo->accessType))
> +            return;

can we actually call a setter here? Feels like maybe this should be an assert?

> Source/JavaScriptCore/jit/JITOperations.cpp:784
> +    RETURN_IF_EXCEPTION(scope, void());

can you just release the scope before calling putPrivateField?

> Source/JavaScriptCore/jit/JITOperations.cpp:798
> +    RETURN_IF_EXCEPTION(scope, void());

can you just release the scope before calling putPrivateField?

> Source/JavaScriptCore/jit/JITOperations.cpp:822
> +    RETURN_IF_EXCEPTION(scope, void());

can you just release the scope before calling putPrivateField?

> Source/JavaScriptCore/jit/JITOperations.cpp:922
> +    if (property.isPrivateName()) {

Could this be conflated with a privateName from a builtin?

> Source/JavaScriptCore/jit/JITOperations.cpp:926
> +        PropertySlot slot(baseObject, PropertySlot::InternalMethodType::VMInquiry);

VMInquiry feels wrong here. Is this also to skip over proxy?

I feel like the invariant here is that private fields must end up in normal property storage. If my understanding is correct, maybe just add a helper for get/set private field on JSObject?

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1155
> +            if (found == bool(bytecode.m_flags.isPrivateFieldAdd()))

bool(...) not needed

> Source/JavaScriptCore/parser/Parser.cpp:3057
> +        semanticFailIfFalse(copyUndeclaredPrivateNamesToOuterScope(), "Cannot reference undeclared private names");

nit: maybe we can provide at least one of the names for a better error message? (Could be a follow-up)

> Source/JavaScriptCore/parser/Parser.cpp:4828
> +bool Parser<LexerType>::ensurePrivateNameUsed(const Identifier* ident)

name nit: this feels like the slightly wrong name given a boolean returns if it's valid or not, and the name contains "ensure" as a verb

Maybe just "usePrivateName"?

> Source/JavaScriptCore/parser/Parser.cpp:4836
> +    ScopeRef current = currentPrivateNameScope(&found);
> +    if (!found)
> +        return false;

nit: maybe just have this be "hasPrivateNameScope"?

> Source/JavaScriptCore/parser/Parser.h:887
> +    bool m_isPrivateNameScope;

do we really need this if we have m_isClassScope? AFAICT, they're always equal to each other.

> Source/JavaScriptCore/parser/Parser.h:1232
> +    ScopeRef currentPrivateNameScope(bool *found = 0)

"bool *found" => "bool* found"
"= 0" => "= nullptr"

> Source/JavaScriptCore/parser/VariableEnvironment.h:108
> +    enum Traits : uint16_t {

why uint16_t?

> Source/JavaScriptCore/parser/VariableEnvironment.h:172
> +        auto& meta = getOrAddPrivateName(identifier.get());

what is meta?

> Source/JavaScriptCore/parser/VariableEnvironment.h:185
> +            find(identifier)->value.setIsCaptured();

couldn't this be an assert that it's captured already? Maybe I'm missing something?

> Source/JavaScriptCore/parser/VariableEnvironment.h:209
> +    ALWAYS_INLINE void copyPrivateNames(VariableEnvironment& other) const

name nit: maybe "copyPrivateNamesTo"?

> Source/JavaScriptCore/parser/VariableEnvironment.h:223
> +    ALWAYS_INLINE void copyUndeclaredPrivateNames(VariableEnvironment& outer) const {

style nit: brace goes on following line
name nit: maybe also suffix of "To" here to method name?

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:316
> +static EncodedJSValue JSC_HOST_CALL createPrivateSymbol(JSGlobalObject* globalObject, CallFrame*)
> +{
> +    VM& vm = globalObject->vm();
> +    return JSValue::encode(Symbol::create(vm, PrivateSymbolImpl::createNullSymbol().get()));
> +}

Should we make this a bytecode instead of a host call? Maybe we can make it even define all the things in one go instead of repeated bytecode "invocations"?

(A follow-up is ok for that.)

That should speed up class creation w/ private fields by some amount.

Ideally, we'd inline this into the compiler, but it might be tricky to inline PrivateSymbolImpl::createNullSymbol, but I haven't checked exactly what that's doing.

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1205
> +            init.set(JSFunction::create(init.vm, jsCast<JSGlobalObject*>(init.owner), 0, String(), createPrivateSymbol));
> +        });

style nit: spacing is off here

> JSTests/stress/put-by-val-direct-putprivate.js:1
> +// TODO: //@ requireOptions("--usePrivateClassFields=1")

?

[Saam Barati]

Comment on attachment 397021 [details]
Patch v15

View in context: https://bugs.webkit.org/attachment.cgi?id=397021&action=review

> Source/JavaScriptCore/ChangeLog:20
> +        subtle, but revolve around whether or not an exception is thrown during certain accesses.

maybe it's best to enumerate those cases here so folks reading the changelog can understand them

> Source/JavaScriptCore/ChangeLog:25
> +        Private field names are loaded from scope before their use. This prevents multiple evaluations
> +        of the same class source from accessing each other's private fields, because the values of the
> +        symbols loaded from the class scope would be distinct. This is required by the proposal text,
> +        and is the key reason why we use ByVal lookups rather than ById lookups.

Maybe also worth saying that we create the symbols during class creation

> Source/JavaScriptCore/ChangeLog:32
> +        resolve_scope      <scope=>, <currentScope>, "#x", GlobalProperty, 0
> +        get_from_scope     <symbol=>, <scope>, "#x", 1050624<DoNotThrowIfNotFound|GlobalProperty|NotInitialization>, 0, 0
> +        get_by_val_direct  <value=>, <receiver --- probably 'this'>, <symbol>

this is nice

> Source/JavaScriptCore/bytecode/PutByValFlags.h:3
> + * Copyright (C) 2019 Igalia S.L.

a lot of these should be 2020 now. Sorry for taking so long to review :-(

> Source/JavaScriptCore/bytecode/PutByValFlags.h:33
> +enum class PrivateFieldAccess : uint8_t {

name nit: This feels like the name should be something like PrivateFieldAccessKind

> Source/JavaScriptCore/bytecode/PutByValFlags.h:36
> +    Create,

I prefer this name over "add". I also like "define". See other comments on the "Add" naming.

> Source/JavaScriptCore/bytecode/PutKind.h:30
> +enum PutKind { Direct, NotDirect, DirectAddPrivate, DirectPutPrivate };

nit: I'm a fan of having the shared part of these names come before the non shared part, so I'd do:
DirectPrivateAdd and DirectPrivatePut

Also, see another comment I made about "add" feels like the wrong name.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2714
> +RegisterID* BytecodeGenerator::emitPrivateFieldAdd(RegisterID* base, RegisterID* property, RegisterID* value)

nit on naming: I feel like "add" is the less idiomatic way of doing this. Maybe "define" somehow is better to incorporate?

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:557
> +    for (; p; p = p->m_next) {

I'm a bit confused, if we're defining these properties first, what do we do in the loop that has this comment:
"Iterate over the remaining properties in the list."

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:882
> +        RefPtr<RegisterID> scope = generator.emitResolveScope(generator.newTemporary(), var);

no need for "newTemporary" here. Also, this use is slightly wrong if we relied on it, as we have no +1 ref to it.
(Also, weirdly, that function doesn't actually seem to store into dst...)

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2140
> +        RefPtr<RegisterID> scope = generator.emitResolveScope(generator.newTemporary(), var);

same comment here on newTemporary. I'm guessing this shows up in a few other places in this patch too.

> Source/JavaScriptCore/dfg/DFGCapabilities.cpp:300
> +    case op_get_by_val_direct: // FIXME: op_get_by_val_direct will need DFG/FTL support to ship class fields.

link to a bug here

Also, does the new version of PutByValDirect work in DFG/FTL?

> Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:156
> +    if (UNLIKELY(m_private != PrivateFieldAccess::None)) {

not a fan of this UNLIKELY here since once private fields are used who knows if this is unlikely or not

> Source/JavaScriptCore/jit/JITOperations.cpp:728
> +    bool found = baseObject->isProxy() ? JSObject::getOwnPropertySlot(baseObject, globalObject, ident, slot)
> +        : baseValue.getOwnPropertySlot(globalObject, ident, slot);

what's up with this proxy check? My guess is you don't want to call the Proxy hook, right? But this also skips DOMwindow.

Also worth thinking about and testing, what happens with "with" and walking the prototype chain w/ one of these symbols? Will we accidentally leak it to a proxy that way?

> Source/JavaScriptCore/jit/JITOperations.cpp:732
> +    if (putKind == DirectAddPrivate && found)
> +        throwException(globalObject, scope, createRedefinedPrivateNameError(globalObject));

What JS code reaches this?

> Source/JavaScriptCore/jit/JITOperations.cpp:738
> +    Structure* structure = 0;

nullptr

> Source/JavaScriptCore/jit/JITOperations.cpp:760
> +    RETURN_IF_EXCEPTION(scope, void());

can you just release the scope before calling putPrivateField?

> Source/JavaScriptCore/jit/JITOperations.cpp:775
> +    putPrivateField(vm, globalObject, callFrame, baseObject, identifier, value, DirectAddPrivate, [=](VM& vm, CodeBlock* codeBlock, Structure* structure, PutPropertySlot& putSlot, const Identifier& ident) {

I'd use a "&" lambda for consistency w/ below

> Source/JavaScriptCore/jit/JITOperations.cpp:779
> +        if (accessType != static_cast<AccessType>(stubInfo->accessType))
> +            return;

can we actually call a setter here? Feels like maybe this should be an assert?

> Source/JavaScriptCore/jit/JITOperations.cpp:784
> +    RETURN_IF_EXCEPTION(scope, void());

can you just release the scope before calling putPrivateField?

> Source/JavaScriptCore/jit/JITOperations.cpp:798
> +    RETURN_IF_EXCEPTION(scope, void());

can you just release the scope before calling putPrivateField?

> Source/JavaScriptCore/jit/JITOperations.cpp:822
> +    RETURN_IF_EXCEPTION(scope, void());

can you just release the scope before calling putPrivateField?

> Source/JavaScriptCore/jit/JITOperations.cpp:922
> +    if (property.isPrivateName()) {

Could this be conflated with a privateName from a builtin?

> Source/JavaScriptCore/jit/JITOperations.cpp:926
> +        PropertySlot slot(baseObject, PropertySlot::InternalMethodType::VMInquiry);

VMInquiry feels wrong here. Is this also to skip over proxy?

I feel like the invariant here is that private fields must end up in normal property storage. If my understanding is correct, maybe just add a helper for get/set private field on JSObject?

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1155
> +            if (found == bool(bytecode.m_flags.isPrivateFieldAdd()))

bool(...) not needed

> Source/JavaScriptCore/parser/Parser.cpp:3057
> +        semanticFailIfFalse(copyUndeclaredPrivateNamesToOuterScope(), "Cannot reference undeclared private names");

nit: maybe we can provide at least one of the names for a better error message? (Could be a follow-up)

> Source/JavaScriptCore/parser/Parser.cpp:4828
> +bool Parser<LexerType>::ensurePrivateNameUsed(const Identifier* ident)

name nit: this feels like the slightly wrong name given a boolean returns if it's valid or not, and the name contains "ensure" as a verb

Maybe just "usePrivateName"?

> Source/JavaScriptCore/parser/Parser.cpp:4836
> +    ScopeRef current = currentPrivateNameScope(&found);
> +    if (!found)
> +        return false;

nit: maybe just have this be "hasPrivateNameScope"?

> Source/JavaScriptCore/parser/Parser.h:887
> +    bool m_isPrivateNameScope;

do we really need this if we have m_isClassScope? AFAICT, they're always equal to each other.

> Source/JavaScriptCore/parser/Parser.h:1232
> +    ScopeRef currentPrivateNameScope(bool *found = 0)

"bool *found" => "bool* found"
"= 0" => "= nullptr"

> Source/JavaScriptCore/parser/VariableEnvironment.h:108
> +    enum Traits : uint16_t {

why uint16_t?

> Source/JavaScriptCore/parser/VariableEnvironment.h:172
> +        auto& meta = getOrAddPrivateName(identifier.get());

what is meta?

> Source/JavaScriptCore/parser/VariableEnvironment.h:185
> +            find(identifier)->value.setIsCaptured();

couldn't this be an assert that it's captured already? Maybe I'm missing something?

> Source/JavaScriptCore/parser/VariableEnvironment.h:209
> +    ALWAYS_INLINE void copyPrivateNames(VariableEnvironment& other) const

name nit: maybe "copyPrivateNamesTo"?

> Source/JavaScriptCore/parser/VariableEnvironment.h:223
> +    ALWAYS_INLINE void copyUndeclaredPrivateNames(VariableEnvironment& outer) const {

style nit: brace goes on following line
name nit: maybe also suffix of "To" here to method name?

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:316
> +static EncodedJSValue JSC_HOST_CALL createPrivateSymbol(JSGlobalObject* globalObject, CallFrame*)
> +{
> +    VM& vm = globalObject->vm();
> +    return JSValue::encode(Symbol::create(vm, PrivateSymbolImpl::createNullSymbol().get()));
> +}

Should we make this a bytecode instead of a host call? Maybe we can make it even define all the things in one go instead of repeated bytecode "invocations"?

(A follow-up is ok for that.)

That should speed up class creation w/ private fields by some amount.

Ideally, we'd inline this into the compiler, but it might be tricky to inline PrivateSymbolImpl::createNullSymbol, but I haven't checked exactly what that's doing.

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1205
> +            init.set(JSFunction::create(init.vm, jsCast<JSGlobalObject*>(init.owner), 0, String(), createPrivateSymbol));
> +        });

style nit: spacing is off here

> JSTests/stress/put-by-val-direct-putprivate.js:1
> +// TODO: //@ requireOptions("--usePrivateClassFields=1")

?

[Saam Barati]

Ughhh, sorry for the repeated duplicate comments. Not sure how that happened. They're all the same.

[Caitlin Potter (:caitp)]

Comment on attachment 397021 [details]
Patch v15

View in context: https://bugs.webkit.org/attachment.cgi?id=397021&action=review

Thanks for the look, I've gone over a bunch of these to answer some questions --- I'll start working on technical changes and nits and making sure all the testing that I think we have is actually in the branch this week.

>>>>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:557
>>>>> +    for (; p; p = p->m_next) {
>>>> 
>>>> I'm a bit confused, if we're defining these properties first, what do we do in the loop that has this comment:
>>>> "Iterate over the remaining properties in the list."
>>> 
>>> I'm a bit confused, if we're defining these properties first, what do we do in the loop that has this comment:
>>> "Iterate over the remaining properties in the list."
>> 
>> I'm a bit confused, if we're defining these properties first, what do we do in the loop that has this comment:
>> "Iterate over the remaining properties in the list."
> 
> I'm a bit confused, if we're defining these properties first, what do we do in the loop that has this comment:
> "Iterate over the remaining properties in the list."

It should probably have a better comment.

This is essentially:

```
propertyNames.forEach(name => {
  if (name.isPrivate())
    lexicalScope[name.toString() /* "#foo" or whatever */] = intrinsicCreatePrivateSymbol();
});
```

This allows them to be referenced by computed property expressions. There's a test262 test which verifies this.

>>>>> Source/JavaScriptCore/dfg/DFGCapabilities.cpp:300
>>>>> +    case op_get_by_val_direct: // FIXME: op_get_by_val_direct will need DFG/FTL support to ship class fields.
>>>> 
>>>> link to a bug here
>>>> 
>>>> Also, does the new version of PutByValDirect work in DFG/FTL?
>>> 
>>> link to a bug here
>>> 
>>> Also, does the new version of PutByValDirect work in DFG/FTL?
>> 
>> link to a bug here
>> 
>> Also, does the new version of PutByValDirect work in DFG/FTL?
> 
> link to a bug here
> 
> Also, does the new version of PutByValDirect work in DFG/FTL?

At least to some extent, it does. I thought we had more stress tests in this branch, but there are so many branches at this point it's hard to keep track. I'll see about bringing in more of them this week.

>>>>> Source/JavaScriptCore/jit/JITOperations.cpp:728
>>>>> +        : baseValue.getOwnPropertySlot(globalObject, ident, slot);
>>>> 
>>>> what's up with this proxy check? My guess is you don't want to call the Proxy hook, right? But this also skips DOMwindow.
>>>> 
>>>> Also worth thinking about and testing, what happens with "with" and walking the prototype chain w/ one of these symbols? Will we accidentally leak it to a proxy that way?
>>> 
>>> what's up with this proxy check? My guess is you don't want to call the Proxy hook, right? But this also skips DOMwindow.
>>> 
>>> Also worth thinking about and testing, what happens with "with" and walking the prototype chain w/ one of these symbols? Will we accidentally leak it to a proxy that way?
>> 
>> what's up with this proxy check? My guess is you don't want to call the Proxy hook, right? But this also skips DOMwindow.
>> 
>> Also worth thinking about and testing, what happens with "with" and walking the prototype chain w/ one of these symbols? Will we accidentally leak it to a proxy that way?
> 
> what's up with this proxy check? My guess is you don't want to call the Proxy hook, right? But this also skips DOMwindow.
> 
> Also worth thinking about and testing, what happens with "with" and walking the prototype chain w/ one of these symbols? Will we accidentally leak it to a proxy that way?

In the current spec, skipping DOMWindow shouldn't be relevant as it will never be a possible private field recipient (I think myself and Caio have discussed this possibility in past months).
An alternative that was considered was to add a method to find if something is specifically a ProxyObject or not, and base the test on that --- we're of course happy to do that if preferred.

I'm not clear on what you mean by leaking the symbol using `with` statements, because private fields are never resolve nodes, so there should be no way to get one from a proxy. Is that the sort of test you're referring to, or something I am missing?

>>>>> Source/JavaScriptCore/jit/JITOperations.cpp:732
>>>>> +        throwException(globalObject, scope, createRedefinedPrivateNameError(globalObject));
>>>> 
>>>> What JS code reaches this?
>>> 
>>> What JS code reaches this?
>> 
>> What JS code reaches this?
> 
> What JS code reaches this?

For instance:

```
class Base {
  constructor(a) { return a ? a : new Proxy(this, {}); }
}

let i = 0;
class Child {
  #x = i++;
  constructor(a) { super(a); }
}

let c = new Child(new Child());
```

We're trying to create the private #x (with the same UniuedStringImpl) twice, on the same object. I believe this case is covered in one of the tests, if not locally, then in test262.

>>>>> Source/JavaScriptCore/jit/JITOperations.cpp:922
>>>>> +    if (property.isPrivateName()) {
>>>> 
>>>> Could this be conflated with a privateName from a builtin?
>>> 
>>> Could this be conflated with a privateName from a builtin?
>> 
>> Could this be conflated with a privateName from a builtin?
> 
> Could this be conflated with a privateName from a builtin?

Yes. We can work around this by going by the PrivateFieldAccessKind instead

>>>>> Source/JavaScriptCore/parser/Parser.h:887
>>>>> +    bool m_isPrivateNameScope;
>>>> 
>>>> do we really need this if we have m_isClassScope? AFAICT, they're always equal to each other.
>>> 
>>> do we really need this if we have m_isClassScope? AFAICT, they're always equal to each other.
>> 
>> do we really need this if we have m_isClassScope? AFAICT, they're always equal to each other.
> 
> do we really need this if we have m_isClassScope? AFAICT, they're always equal to each other.

works either way for me. I like being clear that it's a private name scope (and theoretically, could be a non-class scope, in some future version of the language), but it's fine to worry about that later.

>>>>> Source/JavaScriptCore/parser/VariableEnvironment.h:108
>>>>> +    enum Traits : uint16_t {
>>>> 
>>>> why uint16_t?
>>> 
>>> why uint16_t?
>> 
>> why uint16_t?
> 
> why uint16_t?

cargo culted from VariableEnvironmentEntry

>>>>> Source/JavaScriptCore/parser/VariableEnvironment.h:172
>>>>> +        auto& meta = getOrAddPrivateName(identifier.get());
>>>> 
>>>> what is meta?
>>> 
>>> what is meta?
>> 
>> what is meta?
> 
> what is meta?

It's a PrivateNameEntry, or metadata about a private name. We also work with a VariableEnvironmentEntry here, so I guess using a distinct name made sense.

>>>>> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:316
>>>>> +}
>>>> 
>>>> Should we make this a bytecode instead of a host call? Maybe we can make it even define all the things in one go instead of repeated bytecode "invocations"?
>>>> 
>>>> (A follow-up is ok for that.)
>>>> 
>>>> That should speed up class creation w/ private fields by some amount.
>>>> 
>>>> Ideally, we'd inline this into the compiler, but it might be tricky to inline PrivateSymbolImpl::createNullSymbol, but I haven't checked exactly what that's doing.
>>> 
>>> Should we make this a bytecode instead of a host call? Maybe we can make it even define all the things in one go instead of repeated bytecode "invocations"?
>>> 
>>> (A follow-up is ok for that.)
>>> 
>>> That should speed up class creation w/ private fields by some amount.
>>> 
>>> Ideally, we'd inline this into the compiler, but it might be tricky to inline PrivateSymbolImpl::createNullSymbol, but I haven't checked exactly what that's doing.
>> 
>> Should we make this a bytecode instead of a host call? Maybe we can make it even define all the things in one go instead of repeated bytecode "invocations"?
>> 
>> (A follow-up is ok for that.)
>> 
>> That should speed up class creation w/ private fields by some amount.
>> 
>> Ideally, we'd inline this into the compiler, but it might be tricky to inline PrivateSymbolImpl::createNullSymbol, but I haven't checked exactly what that's doing.
> 
> Should we make this a bytecode instead of a host call? Maybe we can make it even define all the things in one go instead of repeated bytecode "invocations"?
> 
> (A follow-up is ok for that.)
> 
> That should speed up class creation w/ private fields by some amount.
> 
> Ideally, we'd inline this into the compiler, but it might be tricky to inline PrivateSymbolImpl::createNullSymbol, but I haven't checked exactly what that's doing.

Agreed

>>>>> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1205
>>>>> +        });
>>>> 
>>>> style nit: spacing is off here
>>> 
>>> style nit: spacing is off here
>> 
>> style nit: spacing is off here
> 
> style nit: spacing is off here

I know check-webkit-style thinks so, but to me it looks to be formatted exactly the same as the rest of the file.

check-webkit-style is happy when it looks like this:

```
    // PrivateSymbols / PrivateNames
    m_linkTimeConstants[static_cast<unsigned>(LinkTimeConstant::createPrivateSymbol)].initLater([] (const Initializer<JSCell>& init) {
        init.set(JSFunction::create(init.vm, jsCast<JSGlobalObject*>(init.owner), 0, String(), createPrivateSymbol));
        });
```

but it doesn't look very good

>>>>> JSTests/stress/put-by-val-direct-putprivate.js:1
>>>>> +// TODO: //@ requireOptions("--usePrivateClassFields=1")
>>>> 
>>>> ?
>>> 
>>> ?
>> 
>> ?
> 
> ?

I haven't looked at this in the past few months, but at some point this was failing some DFG/FTL variants. It may still be, I'll check tomorrow.

[Alexey Shvayka]

(In reply to Caitlin Potter (:caitp) from comment #29)
> In the current spec, skipping DOMWindow shouldn't be relevant as it will
> never be a possible private field recipient (I think myself and Caio have
> discussed this possibility in past months).
> An alternative that was considered was to add a method to find if something
> is specifically a ProxyObject or not, and base the test on that --- we're of
> course happy to do that if preferred.

ProxyObject::getOwnPropertySlot() returns `false` for private names before doing anything observable. Am I missing something or we can just drop the checks?

Also, do we need to account for `baseValue` being a DebuggerScope? If so, DebuggerScope::getOwnPropertySlot() might require a tweak for private names.

[Caitlin Potter (:caitp)]

(In reply to Alexey Shvayka from comment #30)
> (In reply to Caitlin Potter (:caitp) from comment #29)
> > In the current spec, skipping DOMWindow shouldn't be relevant as it will
> > never be a possible private field recipient (I think myself and Caio have
> > discussed this possibility in past months).
> > An alternative that was considered was to add a method to find if something
> > is specifically a ProxyObject or not, and base the test on that --- we're of
> > course happy to do that if preferred.
> 
> ProxyObject::getOwnPropertySlot() returns `false` for private names before
> doing anything observable. Am I missing something or we can just drop the
> checks?

In the case of private fields, a Proxy can “have” private fields, attached directly to the Proxy. Because of that behaviour in the internal method, we avoid using it and use JSObject’s instead.

> Also, do we need to account for `baseValue` being a DebuggerScope? If so,
> DebuggerScope::getOwnPropertySlot() might require a tweak for private names.

I’m not sure exactly how we’ll want this to look in the inspector, but probably will involve some of this.

[Caitlin Potter (:caitp)]

Created attachment 397121 [details]
Patch v16

[Caitlin Potter (:caitp)]

Comment on attachment 397021 [details]
Patch v15

View in context: https://bugs.webkit.org/attachment.cgi?id=397021&action=review

I've addressed a bunch of these comments, a summary can be found in https://github.com/caitp/webkit/commit/f45d251b43f7f2e65e0350ab5af26b455146551e

Still working on coming up with more JIT stress tests.

>> Source/JavaScriptCore/bytecode/PutByValFlags.h:3
>> + * Copyright (C) 2019 Igalia S.L.
> 
> a lot of these should be 2020 now. Sorry for taking so long to review :-(

Done

>> Source/JavaScriptCore/bytecode/PutByValFlags.h:33
>> +enum class PrivateFieldAccess : uint8_t {
> 
> name nit: This feels like the name should be something like PrivateFieldAccessKind

sgtm, done

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2714
>> +RegisterID* BytecodeGenerator::emitPrivateFieldAdd(RegisterID* base, RegisterID* property, RegisterID* value)
> 
> nit on naming: I feel like "add" is the less idiomatic way of doing this. Maybe "define" somehow is better to incorporate?

I went with the spec algorithm naming convention, but yeah, sgtm. `emitDefinePrivateField` for now, maybe?

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:557
>> +    for (; p; p = p->m_next) {
> 
> I'm a bit confused, if we're defining these properties first, what do we do in the loop that has this comment:
> "Iterate over the remaining properties in the list."

I've moved these declarations to a separate function, so this shouldn't affect cases like ObjectLiterals anymore, and the function name should be less confusing about what it's doing.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:882
>> +        RefPtr<RegisterID> scope = generator.emitResolveScope(generator.newTemporary(), var);
> 
> no need for "newTemporary" here. Also, this use is slightly wrong if we relied on it, as we have no +1 ref to it.
> (Also, weirdly, that function doesn't actually seem to store into dst...)

I've replaced `emitResolveScope(generator.newTemporary(),` with `emitResolveScope(nullptr,` which hopefully covers most of these comments.

>> Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:156
>> +    if (UNLIKELY(m_private != PrivateFieldAccess::None)) {
> 
> not a fan of this UNLIKELY here since once private fields are used who knows if this is unlikely or not

I've removed it

>> Source/JavaScriptCore/jit/JITOperations.cpp:760
>> +    RETURN_IF_EXCEPTION(scope, void());
> 
> can you just release the scope before calling putPrivateField?

I guess the rule for that is if it's called by C++ code or not? It looks like it works fine, so done.

Does this mean I can get rid of the ThrowScope for these JITOperations? They don't seem to be needed.

>> Source/JavaScriptCore/jit/JITOperations.cpp:775
>> +    putPrivateField(vm, globalObject, callFrame, baseObject, identifier, value, DirectAddPrivate, [=](VM& vm, CodeBlock* codeBlock, Structure* structure, PutPropertySlot& putSlot, const Identifier& ident) {
> 
> I'd use a "&" lambda for consistency w/ below

Done

>> Source/JavaScriptCore/jit/JITOperations.cpp:779
>> +            return;
> 
> can we actually call a setter here? Feels like maybe this should be an assert?

Is it actually possible for StubInfo::accessType to change here? i must be missing how. But, changed to an ASSERT.

>>> Source/JavaScriptCore/jit/JITOperations.cpp:922
>>> +    if (property.isPrivateName()) {
>> 
>> Could this be conflated with a privateName from a builtin?
> 
> Yes. We can work around this by going by the PrivateFieldAccessKind instead

Actually, it looks like this code isn't reachable anymore at all, so I've just deleted it.

>> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1155
>> +            if (found == bool(bytecode.m_flags.isPrivateFieldAdd()))
> 
> bool(...) not needed

done

>> Source/JavaScriptCore/parser/Parser.cpp:3057
>> +        semanticFailIfFalse(copyUndeclaredPrivateNamesToOuterScope(), "Cannot reference undeclared private names");
> 
> nit: maybe we can provide at least one of the names for a better error message? (Could be a follow-up)

I agree, keeping it simple for now though

>> Source/JavaScriptCore/parser/Parser.cpp:4828
>> +bool Parser<LexerType>::ensurePrivateNameUsed(const Identifier* ident)
> 
> name nit: this feels like the slightly wrong name given a boolean returns if it's valid or not, and the name contains "ensure" as a verb
> 
> Maybe just "usePrivateName"?

Done

>> Source/JavaScriptCore/parser/Parser.cpp:4836
>> +        return false;
> 
> nit: maybe just have this be "hasPrivateNameScope"?

How do we feel about something like `Optional<ScopeRef> privateNameScope()` and `ScopeRef expectPrivateNameScope()`? The latter would ASSERT, the former would return nullopt if the scope is not a PrivateNameScope.

I'm open. In this case, just querying whether there is a scope or not isn't enough, since we need to affect the found scope if it's present.

>> Source/JavaScriptCore/parser/VariableEnvironment.h:185
>> +            find(identifier)->value.setIsCaptured();
> 
> couldn't this be an assert that it's captured already? Maybe I'm missing something?

class-fields-stress-instance.js fails if I add that assertion here

[Caitlin Potter (:caitp)]

Created attachment 397231 [details]
Patch v17

Fixing 32bit builds (hopefully)

[Caio Lima]

Comment on attachment 397021 [details]
Patch v15

View in context: https://bugs.webkit.org/attachment.cgi?id=397021&action=review

>>>>>> Source/JavaScriptCore/jit/JITOperations.cpp:779
>>>>>> +            return;
>>>>> 
>>>>> can we actually call a setter here? Feels like maybe this should be an assert?
>>>> 
>>>> can we actually call a setter here? Feels like maybe this should be an assert?
>>> 
>>> can we actually call a setter here? Feels like maybe this should be an assert?
>> 
>> can we actually call a setter here? Feels like maybe this should be an assert?
> 
> Is it actually possible for StubInfo::accessType to change here? i must be missing how. But, changed to an ASSERT.

I don't think it is possible to call setter when doing `putPrivateField`. This operation observable spec-wise.

>>>>>> Source/JavaScriptCore/parser/VariableEnvironment.h:185
>>>>>> +            find(identifier)->value.setIsCaptured();
>>>>> 
>>>>> couldn't this be an assert that it's captured already? Maybe I'm missing something?
>>>> 
>>>> couldn't this be an assert that it's captured already? Maybe I'm missing something?
>>> 
>>> couldn't this be an assert that it's captured already? Maybe I'm missing something?
>> 
>> couldn't this be an assert that it's captured already? Maybe I'm missing something?
> 
> class-fields-stress-instance.js fails if I add that assertion here

Do you mind share which JS code makes this assertion fail?

[Caio Lima]

Comment on attachment 397231 [details]
Patch v17

View in context: https://bugs.webkit.org/attachment.cgi?id=397231&action=review

> Source/JavaScriptCore/parser/Parser.cpp:3056
> +        // Fail if there are no parent private name scopes and any used-but-undeclared private names.

Could you create a bug and add a FIXME here to improve this error message?

[Caitlin Potter (:caitp)]

Created attachment 398086 [details]
Patch v18

Added a bunch of new stress tests

[Caitlin Potter (:caitp)]

Comment on attachment 397021 [details]
Patch v15

View in context: https://bugs.webkit.org/attachment.cgi?id=397021&action=review

>> Source/JavaScriptCore/ChangeLog:20
>> +        subtle, but revolve around whether or not an exception is thrown during certain accesses.
>
> maybe it's best to enumerate those cases here so folks reading the changelog can understand them

Done (this will be in Patch v19)

>> Source/JavaScriptCore/ChangeLog:25
>> +        and is the key reason why we use ByVal lookups rather than ById lookups.
> 
> Maybe also worth saying that we create the symbols during class creation

Done (this will be in Patch v19)

[Saam Barati]

Comment on attachment 397021 [details]
Patch v15

View in context: https://bugs.webkit.org/attachment.cgi?id=397021&action=review

>>>>>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2714
>>>>>> +RegisterID* BytecodeGenerator::emitPrivateFieldAdd(RegisterID* base, RegisterID* property, RegisterID* value)
>>>>> 
>>>>> nit on naming: I feel like "add" is the less idiomatic way of doing this. Maybe "define" somehow is better to incorporate?
>>>> 
>>>> nit on naming: I feel like "add" is the less idiomatic way of doing this. Maybe "define" somehow is better to incorporate?
>>> 
>>> nit on naming: I feel like "add" is the less idiomatic way of doing this. Maybe "define" somehow is better to incorporate?
>> 
>> nit on naming: I feel like "add" is the less idiomatic way of doing this. Maybe "define" somehow is better to incorporate?
> 
> I went with the spec algorithm naming convention, but yeah, sgtm. `emitDefinePrivateField` for now, maybe?

Gotcha. I didn't know it's the spec naming. I'll leave it up to you what to use, but I'd just try to be consistent everywhere.

>>>>>>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:557
>>>>>>> +    for (; p; p = p->m_next) {
>>>>>> 
>>>>>> I'm a bit confused, if we're defining these properties first, what do we do in the loop that has this comment:
>>>>>> "Iterate over the remaining properties in the list."
>>>>> 
>>>>> I'm a bit confused, if we're defining these properties first, what do we do in the loop that has this comment:
>>>>> "Iterate over the remaining properties in the list."
>>>> 
>>>> I'm a bit confused, if we're defining these properties first, what do we do in the loop that has this comment:
>>>> "Iterate over the remaining properties in the list."
>>> 
>>> I'm a bit confused, if we're defining these properties first, what do we do in the loop that has this comment:
>>> "Iterate over the remaining properties in the list."
>> 
>> It should probably have a better comment.
>> 
>> This is essentially:
>> 
>> ```
>> propertyNames.forEach(name => {
>>   if (name.isPrivate())
>>     lexicalScope[name.toString() /* "#foo" or whatever */] = intrinsicCreatePrivateSymbol();
>> });
>> ```
>> 
>> This allows them to be referenced by computed property expressions. There's a test262 test which verifies this.
> 
> I've moved these declarations to a separate function, so this shouldn't affect cases like ObjectLiterals anymore, and the function name should be less confusing about what it's doing.

👍

>>>>>> Source/JavaScriptCore/jit/JITOperations.cpp:728
>>>>>> +        : baseValue.getOwnPropertySlot(globalObject, ident, slot);
>>>>> 
>>>>> what's up with this proxy check? My guess is you don't want to call the Proxy hook, right? But this also skips DOMwindow.
>>>>> 
>>>>> Also worth thinking about and testing, what happens with "with" and walking the prototype chain w/ one of these symbols? Will we accidentally leak it to a proxy that way?
>>>> 
>>>> what's up with this proxy check? My guess is you don't want to call the Proxy hook, right? But this also skips DOMwindow.
>>>> 
>>>> Also worth thinking about and testing, what happens with "with" and walking the prototype chain w/ one of these symbols? Will we accidentally leak it to a proxy that way?
>>> 
>>> what's up with this proxy check? My guess is you don't want to call the Proxy hook, right? But this also skips DOMwindow.
>>> 
>>> Also worth thinking about and testing, what happens with "with" and walking the prototype chain w/ one of these symbols? Will we accidentally leak it to a proxy that way?
>> 
>> what's up with this proxy check? My guess is you don't want to call the Proxy hook, right? But this also skips DOMwindow.
>> 
>> Also worth thinking about and testing, what happens with "with" and walking the prototype chain w/ one of these symbols? Will we accidentally leak it to a proxy that way?
> 
> In the current spec, skipping DOMWindow shouldn't be relevant as it will never be a possible private field recipient (I think myself and Caio have discussed this possibility in past months).
> An alternative that was considered was to add a method to find if something is specifically a ProxyObject or not, and base the test on that --- we're of course happy to do that if preferred.
> 
> I'm not clear on what you mean by leaking the symbol using `with` statements, because private fields are never resolve nodes, so there should be no way to get one from a proxy. Is that the sort of test you're referring to, or something I am missing?

You're not missing anything. That's a good point that they're never resolve nodes.

For some reason, I just expected that "#foo" was valid

>>>>>> Source/JavaScriptCore/jit/JITOperations.cpp:732
>>>>>> +        throwException(globalObject, scope, createRedefinedPrivateNameError(globalObject));
>>>>> 
>>>>> What JS code reaches this?
>>>> 
>>>> What JS code reaches this?
>>> 
>>> What JS code reaches this?
>> 
>> What JS code reaches this?
> 
> For instance:
> 
> ```
> class Base {
>   constructor(a) { return a ? a : new Proxy(this, {}); }
> }
> 
> let i = 0;
> class Child {
>   #x = i++;
>   constructor(a) { super(a); }
> }
> 
> let c = new Child(new Child());
> ```
> 
> We're trying to create the private #x (with the same UniuedStringImpl) twice, on the same object. I believe this case is covered in one of the tests, if not locally, then in test262.

👍

>>>>>> Source/JavaScriptCore/jit/JITOperations.cpp:760
>>>>>> +    RETURN_IF_EXCEPTION(scope, void());
>>>>> 
>>>>> can you just release the scope before calling putPrivateField?
>>>> 
>>>> can you just release the scope before calling putPrivateField?
>>> 
>>> can you just release the scope before calling putPrivateField?
>> 
>> can you just release the scope before calling putPrivateField?
> 
> I guess the rule for that is if it's called by C++ code or not? It looks like it works fine, so done.
> 
> Does this mean I can get rid of the ThrowScope for these JITOperations? They don't seem to be needed.

This one doesn't seem to need the throw scope.

>>>>>>> Source/JavaScriptCore/jit/JITOperations.cpp:779
>>>>>>> +            return;
>>>>>> 
>>>>>> can we actually call a setter here? Feels like maybe this should be an assert?
>>>>> 
>>>>> can we actually call a setter here? Feels like maybe this should be an assert?
>>>> 
>>>> can we actually call a setter here? Feels like maybe this should be an assert?
>>> 
>>> can we actually call a setter here? Feels like maybe this should be an assert?
>> 
>> Is it actually possible for StubInfo::accessType to change here? i must be missing how. But, changed to an ASSERT.
> 
> I don't think it is possible to call setter when doing `putPrivateField`. This operation observable spec-wise.

I think it's only possible via a setter call (or something that has arbitrary side effects), which shouldn't be possible here.

>>>>>> Source/JavaScriptCore/parser/Parser.cpp:4836
>>>>>> +        return false;
>>>>> 
>>>>> nit: maybe just have this be "hasPrivateNameScope"?
>>>> 
>>>> nit: maybe just have this be "hasPrivateNameScope"?
>>> 
>>> nit: maybe just have this be "hasPrivateNameScope"?
>> 
>> nit: maybe just have this be "hasPrivateNameScope"?
> 
> How do we feel about something like `Optional<ScopeRef> privateNameScope()` and `ScopeRef expectPrivateNameScope()`? The latter would ASSERT, the former would return nullopt if the scope is not a PrivateNameScope.
> 
> I'm open. In this case, just querying whether there is a scope or not isn't enough, since we need to affect the found scope if it's present.

I think having an Optional version is fine. I'd maybe have the default one assert "privateNameScope" and add a qualifier for the Optional one, like "findPrivateNameScope" or perhaps some better name.

>>>>>> Source/JavaScriptCore/parser/Parser.h:887
>>>>>> +    bool m_isPrivateNameScope;
>>>>> 
>>>>> do we really need this if we have m_isClassScope? AFAICT, they're always equal to each other.
>>>> 
>>>> do we really need this if we have m_isClassScope? AFAICT, they're always equal to each other.
>>> 
>>> do we really need this if we have m_isClassScope? AFAICT, they're always equal to each other.
>> 
>> do we really need this if we have m_isClassScope? AFAICT, they're always equal to each other.
> 
> works either way for me. I like being clear that it's a private name scope (and theoretically, could be a non-class scope, in some future version of the language), but it's fine to worry about that later.

I think it's good to keep the abstraction, but maybe we could do it via just the method call (isPrivateNameScope()) instead of the field.

>>>>>> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1205
>>>>>> +        });
>>>>> 
>>>>> style nit: spacing is off here
>>>> 
>>>> style nit: spacing is off here
>>> 
>>> style nit: spacing is off here
>> 
>> style nit: spacing is off here
> 
> I know check-webkit-style thinks so, but to me it looks to be formatted exactly the same as the rest of the file.
> 
> check-webkit-style is happy when it looks like this:
> 
> ```
>     // PrivateSymbols / PrivateNames
>     m_linkTimeConstants[static_cast<unsigned>(LinkTimeConstant::createPrivateSymbol)].initLater([] (const Initializer<JSCell>& init) {
>         init.set(JSFunction::create(init.vm, jsCast<JSGlobalObject*>(init.owner), 0, String(), createPrivateSymbol));
>         });
> ```
> 
> but it doesn't look very good

Gotcha. I didn't realize they were also mis-formatted. I'd vote for just using the better formatting.

[Caitlin Potter (:caitp)]

Created attachment 398691 [details]
Patch v19

[Caitlin Potter (:caitp)]

Comment on attachment 397021 [details]
Patch v15

I've added the helpers in JSObject.h that were discussed offline, added new tests, renamed some stuff. Hopefully this is looking pretty good for the initial patch, so that we can start working on landing all of the followups.

View in context: https://bugs.webkit.org/attachment.cgi?id=397021&action=review

>>>> Source/JavaScriptCore/jit/JITOperations.cpp:760
>>>> +    RETURN_IF_EXCEPTION(scope, void());
>>> 
>>> can you just release the scope before calling putPrivateField?
>> 
>> I guess the rule for that is if it's called by C++ code or not? It looks like it works fine, so done.
>> 
>> Does this mean I can get rid of the ThrowScope for these JITOperations? They don't seem to be needed.
> 
> This one doesn't seem to need the throw scope.

It seems to be safe to remove them for all the added JITOperations here, so I've done that

>> Source/JavaScriptCore/jit/JITOperations.cpp:926
>> +        PropertySlot slot(baseObject, PropertySlot::InternalMethodType::VMInquiry);
> 
> VMInquiry feels wrong here. Is this also to skip over proxy?
> 
> I feel like the invariant here is that private fields must end up in normal property storage. If my understanding is correct, maybe just add a helper for get/set private field on JSObject?

I've added helpers in JSObject: "getPrivateFieldSlot" which fills in a PropertySlot and returns true or false depending on if the field is found or not, without throwing any exceptions,
"getPrivateField" which performs "getPrivateFieldSlot", but throws a TypeError if the field is not present, "putPrivateField" which performs "getPrivateFieldSlot", throws if the field does not exist,
and then delegates to "putDirect" to overwrite the field. Lastly, "definePrivateField", which acts similar to "putPrivateField", but throws if the field already exists. Any code which had been doing
these things before, has been changed to invoke those helpers instead.

>>>> Source/JavaScriptCore/parser/Parser.cpp:4836
>>>> +        return false;
>>>> 
>>> 
>>> nit: maybe just have this be "hasPrivateNameScope"?
>> 
>> How do we feel about something like `Optional<ScopeRef> privateNameScope()` and `ScopeRef expectPrivateNameScope()`? The latter would ASSERT, the former would return nullopt if the scope is not a PrivateNameScope.
>> 
>> I'm open. In this case, just querying whether there is a scope or not isn't enough, since we need to affect the found scope if it's present.
> 
> I think having an Optional version is fine. I'd maybe have the default one assert "privateNameScope" and add a qualifier for the Optional one, like "findPrivateNameScope" or perhaps some better name.

I like "findPrivateNameScope", done

>>> Source/JavaScriptCore/parser/Parser.h:887
>>> +    bool m_isPrivateNameScope;
>> 
>> works either way for me. I like being clear that it's a private name scope (and theoretically, could be a non-class scope, in some future version of the language), but it's fine to worry about that later.
> 
> I think it's good to keep the abstraction, but maybe we could do it via just the method call (isPrivateNameScope()) instead of the field.

Done. However, it is potentially incorrect/misleading in the case of direct eval, but I don't think it should break anything. At this point, I don't think it's worth changing it back, we can always fix it later on if needed. It shouldn't break anything.

>>> JSTests/stress/put-by-val-direct-putprivate.js:1
>>> +// TODO: //@ requireOptions("--usePrivateClassFields=1")
>> 
>> ?
> 
> I haven't looked at this in the past few months, but at some point this was failing some DFG/FTL variants. It may still be, I'll check tomorrow.

We've investigated this a bit further, eager tier-up for private field access is broken (does not follow correct semantics for private field accesses) at the moment, as it depends too much on profiling data. I think we can do better at this, but it will be a bit messy to do it in this patch, so I'd prefer to hold off on this for now.

I have updated the tests relating to this to have a clearer explanation of what they're doing and why they only run a couple variants without eager tier-up.

[Caitlin Potter (:caitp)]

Created attachment 398693 [details]
Patch v19

Oops, not sure what happened there.

[Caitlin Potter (:caitp)]

Created attachment 400357 [details]
Patch v20

This version changes get_by_val_direct to get_private_name, adds get_by_id style LLInt caching in metadata, and removes parts of the opcode that aren't used (non-Symbol keyed operands). The rest of the patch is the same as it has been since last week, as I've been told it had been under review

[Caitlin Potter (:caitp)]

Created attachment 400458 [details]
Patch v21

Fix cloop & cached property access, in theory

[Caitlin Potter (:caitp)]

Created attachment 400459 [details]
Patch v21

[Caitlin Potter (:caitp)]

Created attachment 400469 [details]
Patch v21

[Caitlin Potter (:caitp)]

Created attachment 400495 [details]
Patch v21

[Caitlin Potter (:caitp)]

Created attachment 400579 [details]
Patch v21.6

[Caio Lima]

Comment on attachment 400579 [details]
Patch v21.6

View in context: https://bugs.webkit.org/attachment.cgi?id=400579&action=review

I'm setting r- because I've found a bug into Baseline JIT for PutByValDirect changes. It is also necessary to include DFG changes to take in consideration `PrivateFieldAccessKind` of `put_by_val_direct`. The patch as it is will compile a `put_by_val_direct <receiver, probably 'this'>, <symbol>, <value>, <PutByValPrivateName>`  as it is `put_by_val_direct <receiver, probably 'this'>, <symbol>, <value>, <None>`.

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:305
>          base = bytecode.m_base;

We are missing here changes to consider new added flags for Private fields.

[Caio Lima]

(In reply to Caio Lima from comment #49)
> Comment on attachment 400579 [details]
> Patch v21.6
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=400579&action=review
> 
> > Source/JavaScriptCore/jit/JITPropertyAccess.cpp:305
> >          base = bytecode.m_base;
> 
> We are missing here changes to consider new added flags for Private fields.

Here is an example of a program that is evaluated wrongly:

```
let shouldStore = false;

function foo() {
  class C {
    #field = this.init();
    #foo;

    init() {
      if (shouldStore)
        this.#foo = 12;
    }
  }

  return new C();
}
noInline(foo);

for (let i = 0; i < 10000; i++) {
  if (i > 1000) {
    shouldStore = true;
    shouldThrow(TypeError, () => {
      foo();
    });
  } else {
    foo();
  }
}
```

I recommend setting `--useConcurrentJIT=false` to reproduce this failure deterministically.

[Saam Barati]

Comment on attachment 400579 [details]
Patch v21.6

View in context: https://bugs.webkit.org/attachment.cgi?id=400579&action=review

r=me

I think it's more beneficial at this point that we land this, turned off behind the runtime flag, so we can improve upon it in ToT, than continue to review it in its entirety as we are now. It's a lot easier to make things better incrementally instead of repeatedly rebasing such a huge patch. This provides a good foundation for us to work off of.

I think you should fix Caio's bug first before landing. It seems IC related. I'd be totally fine with just turning off ICs for private fields in this patch, and doing IC work in a follow-up patch(es), since there is still a lot of IC work to do.

I also have a few comments in this patch, but they can be fixed later if they have bugs open to address them.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:553
> +            RefPtr<RegisterID> createPrivateSymbol = generator.moveLinkTimeConstant(nullptr, LinkTimeConstant::createPrivateSymbol);

nit: let's just do this move once. We can hoist the variable out of the loop and just check if it's non-null.

> Source/JavaScriptCore/dfg/DFGCapabilities.cpp:301
> +    case op_get_private_name: // FIXME: op_get_private_name will need DFG/FTL support to ship class fields.

let's link to a bug here

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1088
> +    PropertySlot slot(baseValue, PropertySlot::InternalMethodType::GetOwnProperty);

nit: you use HasOwnProperty vs GeteOwnProperty in various calls to getPrivateField. I think VMInquiry is probably more correct, since we're not actually doing any of the different spec methods here. We're just using PropertySlot to aid us in ICs.

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:1114
> +            if (structure->propertyAccessesAreCacheable() && !structure->needImpurePropertyWatchpoint()) {

for private names, I don't see why this would be needed. They're not something that can be overridden.

It might make sense to do all the IC work in a follow-up, so the rules can be seen both here, and in the JITs, when they're implemented to do ICs

the "structure->propertyAccessesAreCacheable" is probably more conservative than we need to be here. I think the only thing we care about is not being an uncacheable dictionary.

Private fields are notable in that they're not behavior that can be augmented by the runtime like normal properties.

> Source/JavaScriptCore/parser/Nodes.h:835
> +        bool isPrivateName() const { return m_type == DotType::PrivateName; }

nit: I think using isPrivateField is less ambiguous since isPrivateName is used slightly differently in other contexts.

> Source/JavaScriptCore/parser/VariableEnvironment.cpp:39
> +    m_map = other.m_map;
> +    m_isEverythingCaptured = other.m_isEverythingCaptured;
> +    m_rareData.reset();
> +    if (other.m_rareData)
> +        m_rareData = WTF::makeUnique<VariableEnvironment::RareData>(*other.m_rareData);
> +    return *this;

nit:
```
            VariableEnvironment env(other);
            swap(env);
            return *this;
```
is the no thinking version of this function that also works if you assign into yourself.

> Source/JavaScriptCore/parser/VariableEnvironment.h:259
> +        return m_rareData->m_privateNames.add(impl, PrivateNameEntry(0)).iterator->value;

nit: you can drop the 0 argument from PrivateNameEntry(0)

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:297
> +static EncodedJSValue JSC_HOST_CALL createPrivateSymbol(JSGlobalObject* globalObject, CallFrame*)

let's open a bugzilla & fixme here where we turn this into a bytecode.

> Source/JavaScriptCore/runtime/JSObject.h:1455
> +ALWAYS_INLINE bool JSObject::getPrivateFieldSlot(JSObject* object, JSGlobalObject* globalObject, PropertyName propertyName, PropertySlot& slot)

let's put this and functions below in JSObjectInlines.h instead of here

> JSTests/stress/get-private-field.js:5
> +// GetByValDirect should throw when the receiver does not have the requested private property

nit: get_private_name or GetPrivateName

> JSTests/stress/put-by-val-direct-putprivate.js:1
> +// TODO: //@ requireOptions("--usePrivateClassFields=1") -- Currently, eager JIT is not supported for private field access.

nit: TODO => FIXME

do we have a bugzilla to address this?

[Caitlin Potter (:caitp)]

Created attachment 401081 [details]
Patch for landing

Patch for landing

[EWS]

[e-mail] does not have reviewer permissions according to https://trac.webkit.org/browser/webkit/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

Rejecting attachment 401081 [details] from commit queue.

[Caio Lima]

(In reply to EWS from comment #53)
> [e-mail] does not have reviewer permissions according to
> https://trac.webkit.org/browser/webkit/trunk/Tools/Scripts/webkitpy/common/
> config/contributors.json.
> 
> Rejecting attachment 401081 [details] from commit queue.

It is a joke, right?

[EWS]

[e-mail] does not have reviewer permissions according to https://trac.webkit.org/browser/webkit/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

Rejecting attachment 401081 [details] from commit queue.

[Caio Lima]

(In reply to Caio Lima from comment #54)
> (In reply to EWS from comment #53)
> > [e-mail] does not have reviewer permissions according to
> > https://trac.webkit.org/browser/webkit/trunk/Tools/Scripts/webkitpy/common/
> > config/contributors.json.
> > 
> > Rejecting attachment 401081 [details] from commit queue.
> 
> It is a joke, right?

It was indeed not a joke! I though you meant that [e-mail] does not have commiter permissions. Sorry for doubting you =O.

[EWS]

Committed r262613: <https://trac.webkit.org/changeset/262613>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 401081 [details].

[Radar WebKit Bug Importer]

<rdar://problem/64033223>

[Radar WebKit Bug Importer]

<rdar://problem/64033227>

[Filip Pizlo]

Comment on attachment 401081 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=401081&action=review

> Source/JavaScriptCore/dfg/DFGCapabilities.cpp:301
> +    case op_get_private_name: // FIXME: op_get_private_name will need DFG/FTL support to ship class fields. (https://bugs.webkit.org/show_bug.cgi?id=212781)

I don't think we should add features to JSC if those features don't come with at least a basic DFG and FTL implementation.  We worked super hard to get the DFG and FTL to 100% language coverage relative to the rest of the engine, and this is a huge regression in that department.  I get that from your standpoint it's probably fine because you're adding support for a language feature we didn't support previously.  But the risk of adding stuff like this to DFGCapabilities is that we'll just forget about it.  The worst thing that can happen is that people will optimize for the fact that get_private_name only makes it up to baseline, and then adding DFG/FTL support for it will be a perf regression, and you'll have a really bad time trying to land the DFG/FTL support patch. 

It should be a goal to remove the DFGCapabilities.cpp file and the FTLCapabilities.cpp one.

Since this is behind a flag anyway, it seem like it would have been possible to keep working on this patch (so that it does add the DFG and FTL support as part of it) and only land it once that is done.

I recommend that we roll this patch out of trunk.  I'm OK with it being in a branch.  I'm also super OK with you continuing to post WIP versions of this patch to this bug while you add basic DFG/FTL support.  WIP patches on bugzilla until ready is my preference for this kind of work.

It's OK for the initial DFG/FTL support in this patch to be super basic, for example just calling the same snippet code generator that the baseline calls, and modeling it as a clobber-the-world event.

[Yusuke Suzuki]

*** Bug 183793 has been marked as a duplicate of this bug. ***