Bug 209363 – hasObservableSideEffectsForRegExpSplit doesn't check for @@match override

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Ross Kirsling]

hasObservableSideEffectsForRegExpSplit doesn't check for @@match override

[Ross Kirsling]

Created attachment 394136 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/60707365>

[Michael Saboff]

Comment on attachment 394136 [details]
Patch

r=me. Looks good.

[Ross Kirsling]

(In reply to Michael Saboff from comment #3)
> Comment on attachment 394136 [details]
> Patch
> 
> r=me. Looks good.

Thanks, Michael!

[EWS]

Committed r258801: <https://trac.webkit.org/changeset/258801>

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

[Yusuke Suzuki]

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?

[Ross Kirsling]

(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.

[Yusuke Suzuki]

(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.

[Ross Kirsling]

(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.

[Ross Kirsling]

(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>

[Saam Barati]

(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.

[Yusuke Suzuki]

(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.

[Ross Kirsling]

(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?

[Yusuke Suzuki]

Discussed with Ross in slack, we will open a bug to fix the above issue.

[Ross Kirsling]

(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.