Closed
Bug 1352429
Opened 7 years ago
Closed 7 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•7 years ago
|
Keywords: triage-deferred
Priority: -- → P3
Comment 1•7 years ago
|
||
maybe it's better suggesting .includes() instead for string+string ?
Assignee | ||
Comment 2•7 years ago
|
||
I'm working on this!
Assignee | ||
Comment 3•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 years ago
|
Keywords: checkin-needed
Comment 14•7 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/389bd78cf79d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Reporter | ||
Comment 16•6 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
•