Improve the error message produced when a user attempts to access a property of [something that evaluated to] undefined.
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
People
(Reporter: mrrrgn, Assigned: arai, NeedInfo)
References
(Blocks 3 open bugs)
Details
(Keywords: dev-doc-complete, feature, site-compat)
Attachments
(2 files, 3 obsolete files)
Take js> f().fail() typein:7:1 TypeError: f(...) is undefined Stack: @typein:7:1 or js> var a = undefined js> a.fail typein:4:1 TypeError: a is undefined Stack: @typein:4:1 These error messages could do better about explaining what actually went wrong in these situations. I suggest something along the lines of: TypeError: f(...) is undefined, can't access property 'fail' of undefined or maybe TypeError: f(...) evaluates to undefined, undefined has no properties.
Comment 1•8 years ago
|
||
(In reply to Morgan Phillips [:mrrrgn] from comment #0) > These error messages could do better about explaining what actually went > wrong in these situations. I suggest something along the lines of: > > TypeError: f(...) is undefined, can't access property 'fail' of undefined > > or maybe > > TypeError: f(...) evaluates to undefined, undefined has no properties. My 2 cents, I prefer the first one because I really like seeing the property name that it was trying to access in the message
Reporter | ||
Comment 2•8 years ago
|
||
(In reply to Brian Grinstead [:bgrins] from comment #1) > (In reply to Morgan Phillips [:mrrrgn] from comment #0) > > These error messages could do better about explaining what actually went > > wrong in these situations. I suggest something along the lines of: > > > > TypeError: f(...) is undefined, can't access property 'fail' of undefined > > > > or maybe > > > > TypeError: f(...) evaluates to undefined, undefined has no properties. > > My 2 cents, I prefer the first one because I really like seeing the property > name that it was trying to access in the message I'm down with that. +1
Comment 3•6 years ago
|
||
So, the message seems to be located here https://searchfox.org/mozilla-central/rev/8384a6519437f5eefbe522196f9ddf5c8b1d3fb4/js/src/js.msg#43 , which is then used in https://searchfox.org/mozilla-central/rev/8384a6519437f5eefbe522196f9ddf5c8b1d3fb4/js/src/vm/JSContext.cpp#887-894 . This ReportIsNotDefined appears to be called in https://searchfox.org/mozilla-central/rev/8384a6519437f5eefbe522196f9ddf5c8b1d3fb4/js/src/vm/NativeObject.cpp#2288,2294 and https://hg.mozilla.org/mozilla-central/log/tip/js/src/vm/NativeObject.cpp I don't know C++ well so I'm not sure what it would take to make the change we want. Arai, I see you did review in this code lately. How hard would you think it is to change the error message to be like `{0} is undefined, can't access property {1}` ?
Assignee | ||
Comment 4•6 years ago
|
||
The message is JSMSG_UNEXPECTED_TYPE https://searchfox.org/mozilla-central/rev/8384a6519437f5eefbe522196f9ddf5c8b1d3fb4/js/src/js.msg#80 which is used by ReportIsNullOrUndefined https://searchfox.org/mozilla-central/rev/8384a6519437f5eefbe522196f9ddf5c8b1d3fb4/js/src/vm/JSContext.cpp#904 which lacks the higher context what the resulting object is used for, so we need to provide property id from callers like this: https://searchfox.org/mozilla-central/rev/8384a6519437f5eefbe522196f9ddf5c8b1d3fb4/js/src/vm/Interpreter.cpp#4575 but the ReportIsNullOrUndefined is used also in different contexts, here's the reverse call graph: ReportIsNullOrUndefined ToObjectSlow ToObject (many callers) CodeGenerator::visitValueToObject (JIT) ThrowObjectCoercible BaselineCompiler::emit_JSOP_CHECKOBJCOERCIBLE (JIT) CodeGenerator::visitCheckObjCoercible (JIT) ToObjectFromStack (many callers) so, it would require adding a new function alternative to ToObject/ToObjectSlow which tries to convert the given value to an object, and returns the object or an error details without throwing the actual error, and then also adding a new function which handles the error case and report an appropriate error message, based on the higher context including property id. it might be complicated since the related case happens also in JIT code, which means we should pass the property id from JIT code. I'll take a look.
Assignee | ||
Comment 5•6 years ago
|
||
so, for most property/element operations, we can get property id or element value, so we can report them. but some expression (like x[y]++) is converted to using JSOP_CHECKOBJCOERCIBLE which throws the error, and it doesn't have property/element info, so in that case we cannot report helpful error message. anyway, I'll try to prepare patch for property access case.
Comment 6•6 years ago
|
||
> anyway, I'll try to prepare patch for property access case.
that's already a very good improvement ! Thanks for looking into this arai
Assignee | ||
Comment 7•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6dd7d6b430d8eaff818fc0de594ea3a1304fdd2f
Assignee | ||
Comment 8•6 years ago
|
||
* Renamed ReportIsNullOrUndefined to ReportIsNullOrUndefinedForPropertyAccess for clarity, and changed it to take `bool reportScanStack`, instead of `int spindex`, in order to handle `reportScanStack==false` case there too, given that there are more callers * Added ReportIsNullOrUndefinedForPropertyAccess variant which takes property key, and report it in the error message. the existing one is used when it's element access and the element key is not primitive (thus converting it to string has side-effect) * Added AutoByteId as a wrapper for JSAutoByteString+jsid * Added ToObjectFromStackForPropertyAccess which takes property key, and calls ReportIsNullOrUndefinedForPropertyAccess with the property key if possible * ToObjectFromStackForPropertyAccess and ToObjectSlowForPropertyAccess have 3 variants (HandleId, HandlePropertyName, HandleValue) in order to lazily convert the property/element key to jsid only when error happens * Changed ToObjectFromStack callers to use ToObjectFromStackForPropertyAccess if property key or element key is available the change for tests are in the next patch, which will be folded with this patch after review.
Comment 9•6 years ago
|
||
Comment on attachment 8995729 [details] [diff] [review] Show property key in the error message when target object value is null or undefined. Review of attachment 8995729 [details] [diff] [review]: ----------------------------------------------------------------- This looks great. Thanks! I'm sorry for the slow review. ::: js/src/vm/JSContext.cpp @@ +936,5 @@ > } > } > > +// A thin wrapper for JSAutoByteString for jsid. > +class MOZ_RAII AutoByteId { If this can be a function rather than a class, that seems just as good and less code. Like this (?) char* EncodeIdAsLatin1(JSContext* cx, HandleId id, JSAutoByteString& str) { ... return str.encodeLatin1(cx, idstr); } @@ +992,5 @@ > + bytes.get(), js_undefined_str, keyBytes.ptr()); > + } else { > + MOZ_ASSERT(v.isNull()); > + JS_ReportErrorNumberLatin1(cx, GetErrorMessage, nullptr, JSMSG_PROPERTY_FAIL_EXPR, > + bytes.get(), js_null_str, keyBytes.ptr()); Style nit: I think the ternary expression in the original would be clearer here: `(v.isNull() ? js_null_str : js_undefined_str)`. With this patch, the reader has to narrow their eyes and visually diff the two branches. The assertion by itself isn't a good enough reason to duplicate code; I do like assertions, but we already asserted v.isNullOrUndefined() at the top and `v` is a non-mutable handle.
Assignee | ||
Comment 10•6 years ago
|
||
Thank you for reviewing :) Apparently I forgot to post the test patch. here it is.
Updated•6 years ago
|
Assignee | ||
Comment 11•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0c6e521429cfaff0585ec6eaf734e9fcf873f8a Bug 1259822 - Show property key in the error message when target object value is null or undefined. r=jorendorff
Comment 12•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f0c6e521429c
Comment 13•6 years ago
|
||
This is great arai, thanks a lot ! 2 things I noticed: - it seems that we lost the [Learn more] link we had in the console next to TypeErrors - could we have the same thing for ReferenceError (e.g. doing `x.prop` without prior `var x;`) ?
Assignee | ||
Comment 14•6 years ago
|
||
thanks :) (In reply to Nicolas Chevobbe [:nchevobbe] from comment #13) > - could we have the same thing for ReferenceError (e.g. doing `x.prop` > without prior `var x;`) ? given that the opcode for reading "x" doesn't have info about ".prop", it's a bit complicated. if this happens frequently, it would worth trying, but I think it happens only once per each expression, compared to this bug that happens whenever the value becomes null/undefined, depending on the execution.
Comment 15•6 years ago
|
||
This is really great, thanks ! Now I miss a last piece, that's when we try to use an undefined variable as a function. The current error is: XXX is not a function It would be nice that the error message also says it's undefined or null. For example: XXX is undefined, can't call it as a function.
Assignee | ||
Comment 16•6 years ago
|
||
(In reply to Julien Wajsberg [:julienw] from comment #15) > This is really great, thanks ! > > Now I miss a last piece, that's when we try to use an undefined variable as > a function. The current error is: > > XXX is not a function > > It would be nice that the error message also says it's undefined or null. > For example: > > XXX is undefined, can't call it as a function. sounds like related to bug 418573.
Comment 17•6 years ago
|
||
Maybe it is too specific for the general release notes. I will leave the release manager decided. Release Note Request (optional, but appreciated) [Why is this notable]: New helpful feature in the devtools [Affects Firefox for Android]: No [Suggested wording]: Improved error messages in the developer console when dealing with undefined variables [Links (documentation, blog post, etc)]:
Comment 19•6 years ago
|
||
Adding dev-doc-needed needed keyword as it fits https://developer.mozilla.org/fr/docs/Mozilla/Firefox/Releases/63 better than our end-users release notes.
Comment 21•6 years ago
|
||
I see arai created a reference page for the error — thanks! I've added a note to the rel notes, see: https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/63#JavaScript
Comment 22•6 years ago
|
||
Updated•6 years ago
|
Comment 23•6 years ago
|
||
That patch should apply cleanly to mozilla-beta after first backing out b58b7cd29a0b.
Comment 24•6 years ago
|
||
Backed out from mozilla-release (= Gecko 63) and mozilla-beta (= Gecko 64 in beta repo) as requested by Jason: https://hg.mozilla.org/releases/mozilla-beta/rev/0e99081b5322d213fdba77a12ebbf6293f9c2a7f https://hg.mozilla.org/releases/mozilla-beta/rev/db3a5881e0d22fa59c2107d49c0fefa675fe6bd3 (FIREFOX_63b_RELBRANCH) https://hg.mozilla.org/releases/mozilla-release/rev/ea4b5c999c575532506dfe6c702f59c5a268983b Setting needinfo to set status for firefox65 to fixed once it gets added next week.
Comment 25•6 years ago
|
||
Backed out the backout (=relanded) due to bug913749.js failing: Bug 1488417: https://hg.mozilla.org/releases/mozilla-beta/rev/0413c9c3d902bf4653569bd4c0e7e6ba1e239ba8 Bug 1259822: https://hg.mozilla.org/releases/mozilla-beta/rev/35c61888a49d69506cdd330b81885838ccf45f8c Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=205877473&repo=mozilla-beta Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&resultStatus=usercancel%2Crunnable%2Ctestfailed%2Cbusted%2Cexception%2Cretry&group_state=expanded&selectedJob=205877473&revision=f76d65097eb36e929926ebf9ebe86052bbd2f67e Means it fails this change: https://hg.mozilla.org/releases/mozilla-beta/rev/0e99081b5322d213fdba77a12ebbf6293f9c2a7f#l8.1
Comment 26•6 years ago
|
||
(updating the flag as per Bug 1498257)
Updated•6 years ago
|
Comment 27•6 years ago
|
||
Backed out once again for 64: https://hg.mozilla.org/releases/mozilla-release/rev/290d09651022f524b7bcc5a5514dc616f93fe89a
Comment 28•5 years ago
|
||
This was backed out from all releases for various web compat regressions. Reopening per IRC discussion with jorendorff.
Comment 29•5 years ago
|
||
Seems like we need to exactly match Chrome's error message "Cannot read property 'foobar' of undefined" to be able to reland this.
Comment 30•5 years ago
|
||
Maybe we could keep Error.message as it currently is, and fill another, DevTools exposed property with a nicer message? This will allow us to iterate on those message without breaking web compatibility. I'm not sure if this could have an impact in some other place though.
Comment 31•5 years ago
|
||
Assuming that https://github.com/facebookincubator/idx is the only problem here, we might actually have a bit more flexibility in what it allows: https://github.com/facebookincubator/idx/blob/master/packages/idx/src/idx.js#L76-L81
Assignee | ||
Comment 32•5 years ago
|
||
fwiw, I've located yet another pattern of error message detection, which dynamically creates the pattern: https://github.com/ignivamanishbhudhraja/React-Native-Boilerplate/blob/83bc40ca1e549ae4dd12ec23db1b5fddd2d84a56/src/utilities/Idx.js#L62-L112
Comment 33•5 years ago
|
||
(In reply to Tooru Fujisawa [:arai] from comment #32) > fwiw, I've located yet another pattern of error message detection, which > dynamically creates the pattern: > https://github.com/ignivamanishbhudhraja/React-Native-Boilerplate/blob/ > 83bc40ca1e549ae4dd12ec23db1b5fddd2d84a56/src/utilities/Idx.js#L62-L112 Is this actually being used? This repository has almost no stars or activity.
Assignee | ||
Comment 34•5 years ago
|
||
(In reply to Tom Schuster [:evilpie] from comment #33) > (In reply to Tooru Fujisawa [:arai] from comment #32) > > fwiw, I've located yet another pattern of error message detection, which > > dynamically creates the pattern: > > https://github.com/ignivamanishbhudhraja/React-Native-Boilerplate/blob/ > > 83bc40ca1e549ae4dd12ec23db1b5fddd2d84a56/src/utilities/Idx.js#L62-L112 > > Is this actually being used? This repository has almost no stars or activity. sorry, I forgot to mention that I wasn't sure it's the canonical repository for the file. then, it seems to be the older version of idx, from 1 year ago: https://github.com/facebookincubator/idx/blame/51349691549bb2202c66bc320025eb1e8c2513f8/packages/idx/src/idx.js
Updated•5 years ago
|
Comment 35•4 years ago
|
||
Mike, Adam, not shipping this means regressing error messages for developers (like https://twitter.com/SamHulick/status/1204598663525421056/photo/1), so I wanted to re-open this discussion to at least iterate this forward.
We might get this unstuck by limiting exposure:
a) Maybe just ship this to DevEdition and let it bake there until the web catches up. This way developers can be exposed and hopefully can fix their error detection.
b) Only throw better error messages when DevTools is open
What are your thoughts?
Comment 36•4 years ago
|
||
a) Maybe just ship this to DevEdition and let it bake there until the web catches up. This way developers can be exposed and hopefully can fix their error detection.
I think this approach is good -- ship in DevEdition (maybe beta?) + Nightly and not break release.
Comment 37•4 years ago
|
||
(In reply to Mike Taylor [:miketaylr] from comment #36)
I think this approach is good -- ship in DevEdition (maybe beta?) + Nightly and not break release.
If we do this, how could we make sure to not regress a release? If we would also exclude beta it would be more safe. See eg bug 1512401, which I filed only a couple days before a release.
Comment 38•4 years ago
|
||
effectively,
if NOT_RELEASE:
do new thing
else:
do old thing
It won't ride to release, until some future point where we have confidence we can ship it (if ever).
Comment 39•4 years ago
|
||
I proposed Nightly and DevEdition (exclude Beta) to reach developers. This is the audience that we need need to make aware of the error-detection breakage in their projects. Beta should stay intact and close to behavior of release.
If we can land this behind a pref, we can also mitigate any impact we see and roll back off-train. We have other devtools prefs recently that were only enabled on Nightly/DevEdition https://searchfox.org/mozilla-central/source/modules/libpref/init/all.js#4911 : #if defined(MOZ_DEV_EDITION) || defined(NIGHTLY_BUILD)
.
Upside will hopefully be that developers have a place to discover, test and fix error parsing breakage. The trade-off would be that Nightly and DevEdition users will receive increased breakage for daily browsing and this will add overhead in web compat triage to sift out the breakage caused by the improved error messages.
Comment 40•4 years ago
|
||
Have we though about switching to error message from
win.browser.runtime is undefined, can't access property "getManifest" of it
to
can't access property "getManifest", win.browser.runtime is undefined
I think this might have a higher probability of keeping some of those regular expressions working.
Comment 41•4 years ago
|
||
I think this might have a higher probability of keeping some of those regular expressions working.
How confident would we be that this breaks scripts less?
Alternatively, we could report the better error just to the Console and the old one to content?
Comment 42•4 years ago
|
||
(In reply to :Harald Kirschner :digitarald from comment #41)
I think this might have a higher probability of keeping some of those regular expressions working.
How confident would we be that this breaks scripts less?
I looked into some of the previous failures reported here, and it would not help much. For example the problem described in bug 1490772 Comment 5 would still occur.
Alternatively, we could report the better error just to the Console and the old one to content?
I don't know, we would have to add a lot of extra machinery to make this possible.
I think we should try doing this for the Dev Edition and Nightly like you proposed.
Comment 43•4 years ago
|
||
:arai, would you be interested in re-landing this behind a pref that can be enabled for Nightly/DE?
Assignee | ||
Comment 44•4 years ago
|
||
will look into this weekend.
Assignee | ||
Comment 45•4 years ago
|
||
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 46•4 years ago
|
||
Depends on D58103
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Comment 47•4 years ago
|
||
Pushed by arai_a@mac.com: https://hg.mozilla.org/integration/autoland/rev/03939cd2d0ab Part 1: Add pref to enable fix for accessing property of null or undefined. r=jorendorff https://hg.mozilla.org/integration/autoland/rev/e8d220dbe029 Part 2: Show property key in the error message when target object value is null or undefined. r=jorendorff
Comment 48•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/03939cd2d0ab
https://hg.mozilla.org/mozilla-central/rev/e8d220dbe029
Updated•4 years ago
|
Description
•