Bug 163491 - Use the reject() helper function for conditionally throwing TypeErrors.
Summary: Use the reject() helper function for conditionally throwing TypeErrors.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords:
Depends on:
Blocks: 163446
  Show dependency treegraph
 
Reported: 2016-10-15 15:13 PDT by Mark Lam
Modified: 2016-10-17 10:04 PDT (History)
11 users (show)

See Also:


Attachments
proposed patch. (74.57 KB, patch)
2016-10-15 15:25 PDT, Mark Lam
fpizlo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2016-10-15 15:13:13 PDT
In some places where we may conditionally throw a TypeError (e.g. when in strict mode), we already use the reject() helper function to conditionally throw the TypeError.  Doing so makes the code mode compact.  This patch applies this idiom in all places that throws TypeError where appropriate.
Comment 1 Mark Lam 2016-10-15 15:25:41 PDT
Created attachment 291728 [details]
proposed patch.
Comment 2 Darin Adler 2016-10-15 15:49:19 PDT
Comment on attachment 291728 [details]
proposed patch.

I don’t understand why a function that throws an error object is named "reject".
Comment 3 Mark Lam 2016-10-15 16:04:48 PDT
(In reply to comment #2)
> Comment on attachment 291728 [details]
> proposed patch.
> 
> I don’t understand why a function that throws an error object is named
> "reject".

I didn't coin the name.

The name came from https://trac.webkit.org/changeset/104488.  The comment attached to the addition of the reject function states "Helper for the DefineOwnProperty? algorithm."  The code which uses it in JSArray::defineOwnNumericProperty() (from https://trac.webkit.org/changeset/104488/trunk/Source/JavaScriptCore/runtime/JSArray.cpp) has this comment:
    // 3. If current is undefined and extensible is false, then Reject. 

My guess is that Reject was a term used in the ECMAScript spec back then.  I no longer see the term used in that way in the current spec.

What the function really does is: failAndThrowTypeErrorIfNeeded().  I'm not sure that that's the best name for it either.  Would you be ok with me moving forward with this patch as is, and renaming the function to a better name in a subsequent patch?
Comment 4 Mark Lam 2016-10-15 16:16:40 PDT
This patch has passed the JSC and layout tests.
Comment 5 JF Bastien 2016-10-15 16:36:20 PDT
Comment on attachment 291728 [details]
proposed patch.

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

Why change many of the calls to explicitly use ASCIILiteral instead of doing it implicitly? The code was inconsistent before, is the implicit one not WebKit style? (follow up: why is that a supported thing)

> Source/JavaScriptCore/runtime/JSArray.cpp:217
> +            return reject(exec, scope, throwException, ASCIILiteral("Attempting to define numeric property on array with non-writable length property."));

Why is this string inline? Others above are changed to a predefined thing. Some others bellow are also inline char*.
Comment 6 Mark Lam 2016-10-15 16:43:47 PDT
(In reply to comment #5)
> Comment on attachment 291728 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291728&action=review
> 
> Why change many of the calls to explicitly use ASCIILiteral instead of doing
> it implicitly? The code was inconsistent before, is the implicit one not
> WebKit style? (follow up: why is that a supported thing)

This is a performance thing.  Here's why:
1. The ASCIILiteral(const char* characters) constructor is explicit.
2. The String(const char* characters) constructor can be implicit.
3. The String(const char* characters) constructor will malloc and copy over the string.
   The String(ASCIILiteral characters) constructor will NOT malloc and copy the string.
4. throwTypeError() takes either a ASCIILiteral or a const String&.

Hence, explicitly using the ASCIILiteral saves us unnecessary mallocs and copies.

> > Source/JavaScriptCore/runtime/JSArray.cpp:217
> > +            return reject(exec, scope, throwException, ASCIILiteral("Attempting to define numeric property on array with non-writable length property."));
> 
> Why is this string inline? Others above are changed to a predefined thing.
> Some others bellow are also inline char*.

Because there's only one instance of this string AFAIK.  Hence, it's not worth the effort to declare it elsewhere.
Comment 7 JF Bastien 2016-10-15 17:42:33 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > Comment on attachment 291728 [details]
> > proposed patch.
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=291728&action=review
> > 
> > Why change many of the calls to explicitly use ASCIILiteral instead of doing
> > it implicitly? The code was inconsistent before, is the implicit one not
> > WebKit style? (follow up: why is that a supported thing)
> 
> This is a performance thing.  Here's why:
> 1. The ASCIILiteral(const char* characters) constructor is explicit.
> 2. The String(const char* characters) constructor can be implicit.
> 3. The String(const char* characters) constructor will malloc and copy over
> the string.
>    The String(ASCIILiteral characters) constructor will NOT malloc and copy
> the string.
> 4. throwTypeError() takes either a ASCIILiteral or a const String&.
> 
> Hence, explicitly using the ASCIILiteral saves us unnecessary mallocs and
> copies.

Gotcha, thanks for explaining. It's a bit surprising that there's a perf gotcha hiding in the API, but I'm guessing the String version of these is also useful so it can't just get removed (and an explicit char* -> String is a PITA so it shouldn't be changed either).

 
> > > Source/JavaScriptCore/runtime/JSArray.cpp:217
> > > +            return reject(exec, scope, throwException, ASCIILiteral("Attempting to define numeric property on array with non-writable length property."));
> > 
> > Why is this string inline? Others above are changed to a predefined thing.
> > Some others bellow are also inline char*.
> 
> Because there's only one instance of this string AFAIK.  Hence, it's not
> worth the effort to declare it elsewhere.

Ah, makes sense. Ideally the non-unique ones get COMDAT'd away as well, but that requires LTO, so manual COMDAT is sadly / probably safer.


Agreed with the comment on "reject" (only just saw it), probably worth changing separately. Happy to do so if you want.


lgtm with all the power my non-reviewer self has :-)
Comment 8 Filip Pizlo 2016-10-17 09:25:42 PDT
Comment on attachment 291728 [details]
proposed patch.

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

Nice.

>>>> Source/JavaScriptCore/runtime/JSArray.cpp:217
>>>> +            return reject(exec, scope, throwException, ASCIILiteral("Attempting to define numeric property on array with non-writable length property."));
>>> 
>>> Why is this string inline? Others above are changed to a predefined thing. Some others bellow are also inline char*.
>> 
>> Because there's only one instance of this string AFAIK.  Hence, it's not worth the effort to declare it elsewhere.
> 
> Ah, makes sense. Ideally the non-unique ones get COMDAT'd away as well, but that requires LTO, so manual COMDAT is sadly / probably safer.
> 
> 
> Agreed with the comment on "reject" (only just saw it), probably worth changing separately. Happy to do so if you want.
> 
> 
> lgtm with all the power my non-reviewer self has :-)

I agree with the policy that used-once strings should be inline.

> Source/JavaScriptCore/runtime/JSFunction.cpp:548
> +    if (descriptor.configurablePresent() && descriptor.configurable())
> +        return reject(exec, scope, throwException, ASCIILiteral(UnconfigurablePropertyChangeConfigurabilityError));
> +    if (descriptor.enumerablePresent() && descriptor.enumerable())
> +        return reject(exec, scope, throwException, ASCIILiteral(UnconfigurablePropertyChangeEnumerabilityError));
> +    if (descriptor.isAccessorDescriptor())
> +        return reject(exec, scope, throwException, ASCIILiteral(UnconfigurablePropertyChangeAccessMechanismError));
> +    if (descriptor.writablePresent() && descriptor.writable())
> +        return reject(exec, scope, throwException, ASCIILiteral(UnconfigurablePropertyChangeWritabilityError));
> +    if (!valueCheck)
> +        return reject(exec, scope, throwException, ASCIILiteral(ReadonlyPropertyChangeError));

Wow, this is so much cleaner!
Comment 9 Mark Lam 2016-10-17 09:46:33 PDT
Comment on attachment 291728 [details]
proposed patch.

I'll commit this manually.
Comment 10 Mark Lam 2016-10-17 10:02:33 PDT
Thanks for the review.  Landed in r207411: <http://trac.webkit.org/r207411>.
Comment 11 Mark Lam 2016-10-17 10:04:54 PDT
FYI, the bug to rename reject() (since Darin pointed out that reject is a weak name for it) is https://bugs.webkit.org/show_bug.cgi?id=163549.