hasObservableSideEffectsForRegExpSplit doesn't check for @@match override
Created attachment 394136 [details] Patch
<rdar://problem/60707365>
Comment on attachment 394136 [details] Patch r=me. Looks good.
(In reply to Michael Saboff from comment #3) > Comment on attachment 394136 [details] > Patch > > r=me. Looks good. Thanks, Michael!
Committed r258801: <https://trac.webkit.org/changeset/258801> All reviewed patches have been landed. Closing bug and clearing flags on attachment 394136 [details].
Comment on attachment 394136 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394136&action=review > Source/JavaScriptCore/builtins/RegExpPrototype.js:467 > + if (regexp.@@match !== @regExpPrototypeSymbolMatch) > + return true; Why don't we use @tryGetById for this access?
(In reply to Yusuke Suzuki from comment #6) > Comment on attachment 394136 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=394136&action=review > > > Source/JavaScriptCore/builtins/RegExpPrototype.js:467 > > + if (regexp.@@match !== @regExpPrototypeSymbolMatch) > > + return true; > > Why don't we use @tryGetById for this access? I was just mimicking this: https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/builtins/StringPrototype.js#L222 https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/builtins/StringPrototype.js#L249 I didn't realize @tryGetById worked with symbols, but I can make an update if you prefer.
(In reply to Ross Kirsling from comment #7) > (In reply to Yusuke Suzuki from comment #6) > > Comment on attachment 394136 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=394136&action=review > > > > > Source/JavaScriptCore/builtins/RegExpPrototype.js:467 > > > + if (regexp.@@match !== @regExpPrototypeSymbolMatch) > > > + return true; > > > > Why don't we use @tryGetById for this access? > > I was just mimicking this: > https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/builtins/ > StringPrototype.js#L222 > https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/builtins/ > StringPrototype.js#L249 > > I didn't realize @tryGetById worked with symbols, but I can make an update > if you prefer. While @tryGetById is not observable to users, usual get can be observable by setting a getter here. So if this check is not observable to users, we need to use @tryGetById. For example, String#replace is accessing @@replace explicitly in an observable way in https://tc39.es/ecma262/#sec-string.prototype.replace. So, xxx.@@replace is fine. On the other hand, I think hasObservableSideEffectsForRegExpSplit should not be observable to users. So we need to use @tryGetById.
(In reply to Yusuke Suzuki from comment #8) > While @tryGetById is not observable to users, usual get can be observable by > setting a getter here. > So if this check is not observable to users, we need to use @tryGetById. > > For example, String#replace is accessing @@replace explicitly in an > observable way in https://tc39.es/ecma262/#sec-string.prototype.replace. So, > xxx.@@replace is fine. > > On the other hand, I think hasObservableSideEffectsForRegExpSplit should not > be observable to users. > So we need to use @tryGetById. Oh! You're quite right, thanks for that explanation. Will fix.
(In reply to Ross Kirsling from comment #9) > Oh! You're quite right, thanks for that explanation. Will fix. Committed r258865: <https://trac.webkit.org/changeset/258865>
(In reply to Ross Kirsling from comment #10) > (In reply to Ross Kirsling from comment #9) > > Oh! You're quite right, thanks for that explanation. Will fix. > > Committed r258865: <https://trac.webkit.org/changeset/258865> In your fix, it feels like the else in your ternary should assert it's looking at a symbol.
(In reply to Saam Barati from comment #11) > (In reply to Ross Kirsling from comment #10) > > (In reply to Ross Kirsling from comment #9) > > > Oh! You're quite right, thanks for that explanation. Will fix. > > > > Committed r258865: <https://trac.webkit.org/changeset/258865> > > In your fix, it feels like the else in your ternary should assert it's > looking at a symbol. Yeah, I think this is not good. Super easiest way to fix this problem is having @tryGetByIdWellKnownSymbol(..., "match"), and emit try_get_by_id with well-known symbol.
(In reply to Yusuke Suzuki from comment #12) > (In reply to Saam Barati from comment #11) > > (In reply to Ross Kirsling from comment #10) > > > (In reply to Ross Kirsling from comment #9) > > > > Oh! You're quite right, thanks for that explanation. Will fix. > > > > > > Committed r258865: <https://trac.webkit.org/changeset/258865> > > > > In your fix, it feels like the else in your ternary should assert it's > > looking at a symbol. > > Yeah, I think this is not good. Super easiest way to fix this problem is > having @tryGetByIdWellKnownSymbol(..., "match"), and emit try_get_by_id with > well-known symbol. Is this better than asserting identifier().isPrivateName() for the ResolveNode case?
Discussed with Ross in slack, we will open a bug to fix the above issue.
(In reply to Yusuke Suzuki from comment #14) > Discussed with Ross in slack, we will open a bug to fix the above issue. Opened bug 209524.