1352429 - Improve 'in' operator error message

Status: RESOLVED FIXED

Description

[Florian Scholz [:fscholz] (MDN) [on vacation, back March 18]]

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]

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 [:fscholz] (MDN) [on vacation, back March 18]]

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

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]

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]

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 [:fscholz] (MDN) [on vacation, back March 18]]

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