Improve the error message produced when a user attempts to access a property of [something that evaluated to] undefined.

REOPENED
Assigned to

Status

()

defect
REOPENED
3 years ago
5 days ago

People

(Reporter: mrrrgn, Assigned: jorendorff)

Tracking

(Blocks 1 bug, {dev-doc-complete, feature, site-compat})

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(relnote-firefox -, firefox63 wontfix, firefox64 wontfix, firefox65 wontfix, firefox66 affected)

Details

Attachments

(3 attachments)

Reporter

Description

3 years ago
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.
Reporter

Updated

3 years ago
Blocks: 1257918
(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

3 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
See Also: → 1476666
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}` ?
Flags: needinfo?(arai.unmht)
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.
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.
Flags: needinfo?(arai.unmht)
> anyway, I'll try to prepare patch for property access case.

that's already a very good improvement ! Thanks for looking into this arai
* 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.
Attachment #8995729 - Flags: review?(jorendorff)
Assignee

Comment 9

10 months 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.
Attachment #8995729 - Flags: review?(jorendorff) → review+
Thank you for reviewing :)

Apparently I forgot to post the test patch.
here it is.
Attachment #9001793 - Flags: review?(jorendorff)
Assignee

Updated

10 months ago
Attachment #9001793 - Flags: review?(jorendorff) → review+
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

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f0c6e521429c
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
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;`) ?

Updated

10 months ago
Depends on: 1486140
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.
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.
(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.
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)]:
relnote-firefox: --- → ?
Keywords: feature
Duplicate of this bug: 1407903
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.
Depends on: 1490772
Duplicate of this bug: 1346627
Depends on: 1498257
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
Assignee

Updated

8 months ago
Assignee: arai.unmht → jorendorff
Assignee

Comment 23

8 months ago
That patch should apply cleanly to mozilla-beta after first backing out b58b7cd29a0b.
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.
Flags: needinfo?(aryx.bugmail)
This was backed out from all releases for various web compat regressions. Reopening per IRC discussion with jorendorff.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla63 → ---
Seems like we need to exactly match Chrome's error message "Cannot read property 'foobar' of undefined" to be able to reland this.
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.
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
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
(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.
(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
You need to log in before you can comment on or make changes to this bug.