Improve 'in' operator error message

RESOLVED FIXED in Firefox 58

Status

()

P3
normal
RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: fscholz, Assigned: mcdonalds.only)

Tracking

(Blocks: 1 bug, {dev-doc-complete, triage-deferred})

unspecified
mozilla58
dev-doc-complete, triage-deferred
Points:
---

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

2 years ago
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.
Keywords: triage-deferred
Priority: -- → P3
maybe it's better suggesting .includes() instead for string+string ?
(Assignee)

Comment 2

a year ago
I'm working on this!
(Assignee)

Comment 3

a year ago
Created attachment 8926254 [details] [diff] [review]
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.
Attachment #8926254 - Flags: review?(arai.unmht)
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
Attachment #8926254 - Flags: review?(arai.unmht) → feedback+
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)
Assignee: nobody → mcdonalds.only
Status: NEW → ASSIGNED
Flags: needinfo?(fscholz)
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?
(Reporter)

Comment 7

a year ago
(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).
Flags: needinfo?(fscholz)
(Assignee)

Comment 8

a year ago
Created attachment 8926764 [details] [diff] [review]
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.
Attachment #8926254 - Attachment is obsolete: true
Attachment #8926764 - Flags: review?(arai.unmht)
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
Attachment #8926764 - Flags: review?(arai.unmht) → feedback+
(Assignee)

Comment 10

a year ago
Created attachment 8926894 [details] [diff] [review]
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.
Attachment #8926764 - Attachment is obsolete: true
Attachment #8926894 - Flags: review?(arai.unmht)
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
Attachment #8926894 - Flags: review?(arai.unmht) → review+
(Assignee)

Comment 12

a year ago
Created attachment 8926924 [details] [diff] [review]
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?
Attachment #8926894 - Attachment is obsolete: true
Attachment #8926924 - Flags: review?(arai.unmht)
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
Attachment #8926924 - Flags: review?(arai.unmht) → review+
Keywords: checkin-needed

Comment 14

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/389bd78cf79d
Improve error message for in operator. r=arai
Keywords: checkin-needed

Comment 15

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/389bd78cf79d
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58

Updated

11 months ago
See Also: → bug 1437377
You need to log in before you can comment on or make changes to this bug.