Closed
Bug 1352429
Opened 8 years ago
Closed 8 years ago
Improve 'in' operator error message
Categories
(Core :: JavaScript Engine, enhancement, P3)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: fs, Assigned: mcdonalds.only)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(Keywords: dev-doc-complete, triage-deferred)
Attachments
(1 file, 3 obsolete files)
6.57 KB,
patch
|
arai
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•8 years ago
|
Keywords: triage-deferred
Priority: -- → P3
Comment 1•8 years ago
|
||
maybe it's better suggesting .includes() instead for string+string ?
Assignee | ||
Comment 2•8 years ago
|
||
I'm working on this!
Assignee | ||
Comment 3•8 years ago
|
||
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 4•8 years ago
|
||
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+
Comment 5•8 years ago
|
||
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 6•8 years ago
|
||
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•8 years 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•8 years ago
|
||
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 9•8 years ago
|
||
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•8 years ago
|
||
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 11•8 years ago
|
||
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•8 years ago
|
||
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 13•8 years ago
|
||
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+
Updated•8 years ago
|
Keywords: checkin-needed
Comment 14•8 years 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•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Reporter | ||
Comment 16•8 years ago
|
||
Updated https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/in_operator_no_object
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•