Bug 221093 – Support Ergonomic Brand Checks proposal (`#x in obj`)

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Jordan Harband]

See https://github.com/tc39/proposal-private-fields-in-in - as of yesterday, it is now stage 3. It'd be great to see Webkit ship this feature ASAP!

[Radar WebKit Bug Importer]

<rdar://problem/73987372>

[Jordan Harband]

test262 tests have landed: https://github.com/tc39/test262/pull/2963

it'd be great to see this implemented :-D

[Ross Kirsling]

Created attachment 427961 [details]
Patch

[Ross Kirsling]

Comment on attachment 427961 [details]
Patch

(Marking r? since this is a fully-working patch, but I've asked for backend guidance on Slack.)

[Caio Lima]

Comment on attachment 427961 [details]
Patch

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

Patch LGTM. I left couple of suggestions.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:2823
> +        emitThrowTypeError("Cannot access static private method or accessor");

Thank you for fixing this.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:2111
> +            fixEdge<CellUse>(node->child1());

Since we throw on anything but Object, I think it would be fine use `ObjectUse` here. However, this choice will cause OSR whenever `base` (i.e child1) is not an object. It is also true for non-cell types for the code as it is.

> Source/JavaScriptCore/jit/JITOperations.cpp:484
> +        return JSValue::encode(jsBoolean(false));

I’d prefer return here `JSValue()`

> Source/JavaScriptCore/jit/JITOperations.cpp:506
> +        return JSValue::encode(jsBoolean(false));

ditto

[Ross Kirsling]

(In reply to Caio Lima from comment #5)
> > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:2111
> > +            fixEdge<CellUse>(node->child1());
> 
> Since we throw on anything but Object, I think it would be fine use
> `ObjectUse` here. However, this choice will cause OSR whenever `base` (i.e
> child1) is not an object. It is also true for non-cell types for the code as
> it is.

Hmm, one way or another, I think I'd like to keep this part consistent with InByVal/InById, since this operand behaves identically.

[Ross Kirsling]

Created attachment 428855 [details]
Patch

Addressed comments and made the feature off-by-default per our meeting. Will aim to optimize InByVal itself next and then mimic that functionality for this feature.

[Ross Kirsling]

Created attachment 428857 [details]
Patch

[Caio Lima]

Comment on attachment 428857 [details]
Patch

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

r=me with comments

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3220
> +        }

I’d assert here if it’s a private method or accessor

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:8279
> +            auto bytecode = currentInstruction->as<OpHasPrivateName>();

Could you add a FIXME to point to a bug where we could inline this operation?

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:8285
> +            auto bytecode = currentInstruction->as<OpHasPrivateBrand>();

Ditto

> Source/JavaScriptCore/runtime/JSObjectInlines.h:702
> +    ASSERT(brand.isSymbol());

Could we assert here that it’s also a PrivateSymbol?

> JSTests/stress/private-in.js:58
> +  shouldThrowTypeError(() => SS.isSS(3));

It would be nice test cases where we evaluate a class more than once.

[Ross Kirsling]

Created attachment 429435 [details]
Patch

[EWS]

Caio Lima found in /Volumes/Data/worker/Commit-Queue/build/JSTests/ChangeLog does not appear to be a valid reviewer according to contributors.json.
/Volumes/Data/worker/Commit-Queue/build/JSTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

[Ross Kirsling]

Created attachment 429451 [details]
Patch for landing

[EWS]

Committed r277926 (238057@main): <https://commits.webkit.org/238057@main>

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