1352429 - Improve 'in' operator error message

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Florian Scholz (Open Web Docs)]

See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/in_operator_no_object (which needs updating, if we update our message)

Case 1: If you are coming from Python and try this:

"Hello" in "Hello World";

Firefox throws: TypeError: invalid 'in' operand "Hello World"
Chrome throws:  TypeError: Cannot use 'in' operator to search for 'Hello' in Hello World.

I think in Chrome makes it much more clear that searching in strings makes no sense with JS 'in'.

Case 2: If you have a null/undefined variable:

var foo = null;
"bar" in foo;

Firefox throws: TypeError: invalid 'in' operand "foo"
Chrome throws: TypeError: Cannot use 'in' operator to search for 'bar' in null

I think it useful to know that something is null and thus 'in' fails.

According to some telemetry probes JSMSG_IN_NOT_OBJECT is in the top 50 of JS errors thrown.

Comment 1

[Tooru Fujisawa [:arai]]

maybe it's better suggesting .includes() instead for string+string ?

Comment 2

[Masaya Hayashi]

I'm working on this!

Comment 3

[Masaya Hayashi]

Attachment: Improve error message for in operator

In this patch, when you execute following code, you will get an error message saying "cannot use 'in' operator to search for 'hello' in 'hellohello'".

> 'hello' in 'hellohello'

In addition, when you use `in` operator for long string, you will get an error message with omitted string as shown below.

> 'hello'.repeat(100) in 'hello world'.repeat(100)

> cannot use 'in' operator to search for 'hellohello...' in 'hello worl...'

I also added a test(named inOperatorError.js) for these error messages, but I didn't get where to put the test file.
So can someone give me an advice for where to put this test file?

Thank you.

Comment 4

[Tooru Fujisawa [:arai]]

Comment on attachment 8926254 [details] [diff] [review]
Improve error message for in operator

Review of attachment 8926254 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you!

about the directory for test, js/src/tests/ecma_6/Expressions would be fine.

then, there's one more place that does the same error report about in-operator, inside baseline JIT.
https://dxr.mozilla.org/mozilla-central/rev/2535bad09d720e71a982f3f70dd6925f66ab8ec7/js/src/jit/BaselineIC.cpp#1272-1275
can you make this and pre-existing error reporting into a single function and use it in both places, leaving isObject() check in caller side, for performance reason?
definition should be in Interpreter.cpp, and declaration should be in Interpreter.h.

::: js/src/js.msg
@@ +116,4 @@
>  MSG_DEF(JSMSG_BAD_INSTANCEOF_RHS,      1, JSEXN_TYPEERR, "invalid 'instanceof' operand {0}")
>  MSG_DEF(JSMSG_BAD_LEFTSIDE_OF_ASS,     0, JSEXN_REFERENCEERR, "invalid assignment left-hand side")
>  MSG_DEF(JSMSG_BAD_PROTOTYPE,           1, JSEXN_TYPEERR, "'prototype' property of {0} is not an object")
> +MSG_DEF(JSMSG_IN_NOT_OBJECT,           2, JSEXN_TYPEERR, "cannot use 'in' operator to search for '{0}' in '{1}'")

you need to keep previous one, and add new one for this specific case.
there's another place that uses JSMSG_IN_NOT_OBJECT with previous way.

::: js/src/tests/temp/inOperatorError.js
@@ +1,1 @@
> +function checkErr(substr, str, messageSubstr, messageStr) {

please add header at the beginning of this file, like the following (lines 1-5):
https://dxr.mozilla.org/mozilla-central/rev/2535bad09d720e71a982f3f70dd6925f66ab8ec7/js/src/tests/ecma_6/Array/concat-proxy.js#1-5
BUGNUMBER is 1352429 here, summary should be a string that says what should happen.

@@ +1,5 @@
> +function checkErr(substr, str, messageSubstr, messageStr) {
> +    try {
> +        substr in str;
> +    } catch (e) {
> +        assertEq(e.message.includes(messageSubstr), true);

please add `var caught = false` before try block, and `caught = true;` in catch block, and `assertEq(caught, true);` after catch block, to make sure the error is thrown and assertions here is executed.

::: js/src/vm/Interpreter.cpp
@@ +2215,5 @@
>  CASE(JSOP_IN)
>  {
>      HandleValue rref = REGS.stackHandleAt(-1);
> +    HandleValue lref = REGS.stackHandleAt(-2);
> +    if (rref.isString() && lref.isString()) {

whole this error handling can be put inside `if (rref.isObject())` block, so that successful case doesn't have to check isString().

@@ +2216,5 @@
>  {
>      HandleValue rref = REGS.stackHandleAt(-1);
> +    HandleValue lref = REGS.stackHandleAt(-2);
> +    if (rref.isString() && lref.isString()) {
> +        static const size_t MIN_STRING_LENGTH = 10;

the name should be MAX_STRING_LENGTH.

and also I think 10 is too short.
maybe around 16 to 32?

@@ +2217,5 @@
>      HandleValue rref = REGS.stackHandleAt(-1);
> +    HandleValue lref = REGS.stackHandleAt(-2);
> +    if (rref.isString() && lref.isString()) {
> +        static const size_t MIN_STRING_LENGTH = 10;
> +        auto omitString = [](JSContext* cx, HandleString hstr) -> JSString* {

what does "h" in "hstr" mean?
can it be "str" ?

@@ +2240,5 @@
> +            if (!rrstr)
> +                goto error;
> +        }
> +
> +        UniqueChars lbytes(JS_EncodeStringToUTF8(cx, lrstr));

please null-check lbytes here.

@@ +2241,5 @@
> +                goto error;
> +        }
> +
> +        UniqueChars lbytes(JS_EncodeStringToUTF8(cx, lrstr));
> +        UniqueChars rbytes(JS_EncodeStringToUTF8(cx, rrstr));

and here for rbytes

Comment 5

[Tooru Fujisawa [:arai]]

fscholz, how do you think about suggesting String.prototype.includes in the error message?
we could also suggest it in MDN side tho (by adding documentation link later)

Comment 6

[Tooru Fujisawa [:arai]]

Comment on attachment 8926254 [details] [diff] [review]
Improve error message for in operator

Review of attachment 8926254 [details] [diff] [review]:
-----------------------------------------------------------------

forgot about the case 2 in comment #0, are you going to fix ("foo" in null) case as well?

Comment 7

[Florian Scholz (Open Web Docs)]

(In reply to Tooru Fujisawa [:arai] from comment #5)
> fscholz, how do you think about suggesting String.prototype.includes in the
> error message?
> we could also suggest it in MDN side tho (by adding documentation link later)

I think for more compatibility many are still using indexOf, and maybe there are other cases like |"foo" in obj| where includes doesn't make sense?

I think generally the messages should just tell you precisely what is wrong and then the [Learn more] pages tell you how to possibly solve it (with various solutions like includes/indexOf and different cases like case1 and case2 from comment #0).

Comment 8

[Masaya Hayashi]

Attachment: Improve error message for use of in operator

Thank you for your review!
I created a new patch.

I'm not sure that test cases are enough.
Can you give me suggestions for other test cases?

Thank you.

Comment 9

[Tooru Fujisawa [:arai]]

Comment on attachment 8926764 [details] [diff] [review]
Improve error message for use of in operator

Review of attachment 8926764 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks again!

::: js/src/tests/ecma_6/Expressions/inNotObjectError.js
@@ +21,5 @@
> +checkErr('subString', 'this is baseString', 'subString', 'this is baseStri...');
> +checkErr('this is subString', 'this is base', 'this is subStrin...', 'this is base');
> +checkErr('HEAD' + 'subString'.repeat(30000), 'HEAD' + 'base'.repeat(30000), 'HEADsubStringsub...', 'HEADbasebasebase...');
> +
> +assertThrowsInstanceOf(() => { 1 in 'hello' }, TypeError);

please add comment that says these are testing that it doesn't crash

@@ +25,5 @@
> +assertThrowsInstanceOf(() => { 1 in 'hello' }, TypeError);
> +assertThrowsInstanceOf(() => { 'hello' in 1 }, TypeError);
> +assertThrowsInstanceOf(() => { 'hello' in null }, TypeError);
> +assertThrowsInstanceOf(() => { null in 'hello' }, TypeError);
> +assertThrowsInstanceOf(() => { null in null }, TypeError);

it would be nice to test the following as well:
  * 1.1
  * true
  * Symbol.iterator
  * undefined
  * {} in left hand side
  * [] in left hand side
  * /a/ in left hand side
  * some expression, like, variable, property access, that returns some of above

::: js/src/vm/Interpreter.cpp
@@ +1693,5 @@
>      DECLARE_POINTER_ASSIGN_OPS(ReservedRooted, T)
>  };
>  
> +void
> +js::ReportInNotObjectError(JSContext* cx, HandleValue lref, int lindex, HandleValue rref, int rindex)

please wrap this line to 99 chars

@@ +1696,5 @@
> +void
> +js::ReportInNotObjectError(JSContext* cx, HandleValue lref, int lindex, HandleValue rref, int rindex)
> +{
> +    static const size_t MAX_STRING_LENGTH = 16;
> +    auto omitString = [](JSContext* cx, HandleString str) -> JSString* {

now omitString is used only in single place, and I think you can just embed this there, instead of creating another lambda.
so that you don't have to capture this.

@@ +1717,5 @@
> +    };
> +
> +    UniqueChars lbytes = lref.isString()
> +                       ? uniqueCharsFromString(cx, lref)
> +                       : DecompileValueGenerator(cx, lindex, lref, nullptr);

DecompileValueGenerator returns a string with Latin1 encoding, so you need to use Latin1 everywhere in this function, instead of UTF-8
(using UTF-8 in decompiler is planned, in bug 1302358, but haven't yet happened)

please use Latin1 variant of functions (JS_EncodeString, JS_ReportErrorNumberLatin1)

@@ +1725,5 @@
> +                       ? uniqueCharsFromString(cx, rref)
> +                       : DecompileValueGenerator(cx, rindex, rref, nullptr);
> +    if (!rbytes)
> +        return;
> +    JS_ReportErrorNumberUTF8(cx, GetErrorMessage, nullptr, JSMSG_IN_NOT_OBJECT, lbytes.get(), rbytes.get());

please wrap this line to 99 chars

Comment 10

[Masaya Hayashi]

Attachment: Improve error message for use of in operator

Thank you for your review again!
I created another patch.

I excluded the test of '{} on the left hand'.
That's because when you execute `{} in null` in Chrome, you will get an error message saying
> Uncaught SyntaxError: Unexpected token in
, not a TypeError.

Firefox says
> SyntaxError: expected expression, got keyword 'in'
even after applying this patch.

Comment 11

[Tooru Fujisawa [:arai]]

Comment on attachment 8926894 [details] [diff] [review]
Improve error message for use of in operator

Review of attachment 8926894 [details] [diff] [review]:
-----------------------------------------------------------------

thank you again!
great work!

> I excluded the test of '{} on the left hand'.
> That's because when you execute `{} in null` in Chrome, you will get an
> error message saying
> > Uncaught SyntaxError: Unexpected token in
> , not a TypeError.
> 
> Firefox says
> > SyntaxError: expected expression, got keyword 'in'
> even after applying this patch.
you need to enclose {} with (), otherwise "{}" at the beginning of statement is treated as a block, not an object.
anyway, I think these tests are sufficient :)

here's try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=db82e002b33691bc3faf72dde65984ec51ffc370

Comment 12

[Masaya Hayashi]

Attachment: Improve error message for use of in operator

Thank you for executing try-run.
It seems that it failed with problem of style.
It now includes TraceLogging.h after StringBuffer.h.
Can you execute try-run again?

Comment 13

[Tooru Fujisawa [:arai]]

Comment on attachment 8926924 [details] [diff] [review]
Improve error message for use of in operator

Review of attachment 8926924 [details] [diff] [review]:
-----------------------------------------------------------------

good catch :)
here's another try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=880fa27e8361946b93968a89916cb9c18f845b80

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/389bd78cf79d
Improve error message for in operator. r=arai

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/389bd78cf79d

Comment 16

[Florian Scholz (Open Web Docs)]

Updated https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/in_operator_no_object