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!
<rdar://problem/73987372>
test262 tests have landed: https://github.com/tc39/test262/pull/2963 it'd be great to see this implemented :-D
Created attachment 427961 [details] Patch
Comment on attachment 427961 [details] Patch (Marking r? since this is a fully-working patch, but I've asked for backend guidance on Slack.)
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
(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.
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.
Created attachment 428857 [details] Patch
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.
Created attachment 429435 [details] Patch
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).
Created attachment 429451 [details] Patch for landing
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].