Bug 163491 – Use the reject() helper function for conditionally throwing TypeErrors.

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

[Mark Lam]

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.

[Mark Lam]

Created attachment 291728 [details]
proposed patch.

[Darin Adler]

Comment on attachment 291728 [details]
proposed patch.

I don’t understand why a function that throws an error object is named "reject".

[Mark Lam]

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

[Mark Lam]

This patch has passed the JSC and layout tests.

[JF Bastien]

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

[Mark Lam]

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

[JF Bastien]

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

[Filip Pizlo]

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!

[Mark Lam]

Comment on attachment 291728 [details]
proposed patch.

I'll commit this manually.

[Mark Lam]

Thanks for the review.  Landed in r207411: <http://trac.webkit.org/r207411>.

[Mark Lam]

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.