891107 - [js-ctypes] TypeError messages should be more intelligible

Status: RESOLVED FIXED

(Core :: js-ctypes, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

Currently, the TypeError messages hooked by js-ctypes are rather unintelligible.

From the top of my head:
- passing |undefined| as an argument to a function call results in an error message mentioning "void 0";
- generally, passing an argument that causes a type error to a function should at least mention the function being called and the argument number.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Note to anyone who may be interested in picking up that bug.

Just patching function |TypeError| to handle |undefined| more intelligently would be a good start.

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Show information about value, type, function, and argument number in type conversion error messages in js-ctypes.

Prepared 8 patches (not yet fully replaced all JS_ReportErros to JS_ReportErrorNumber though).

Part 1 improves error message related to type conversion (ImplicitConvert, ExplicitConvert, ConvertArgument, etc), like an example in bug 551057 comment #0:
> can't pass the string "xyzzy" to argument 2 of hypot(double, double)

Basically, js-ctypes' function type notation is different from above C style, so I added some functions to generate dedicated type notation for native functions (BuildCStyleFunctionTypeSource, BuildCStyleTypeSource, and BuildFunctionTypeSource). Then, to do that, I added SLOT_FUNNAME slot to CData, which holds a reference to JSString represents function name.
For non-native functions, original function type notation (ctypes.FunctionType(...).ptr(...)) is used.

Also, added ConversionType enum class and some arguments (funObj, argIndex, arrObj, arrIndex) to determine where the error happens. ConversionType::Argument corresponds to `isArgument == true`.

Comment 4

[Tooru Fujisawa [:arai]]

Forgot to add try run result.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c0ca66d6a5a
(Commit messages are old)

Comment 5

[Jason Orendorff [:jorendorff]]

Hey arai, does this still apply? I think sfink made some changes to these error messages that might help.

Sorry for failing to review this. If you have a chance to take a look, I'd appreciate it. I'll look at it as soon as I see your response here.

Comment 6

[Steve Fink [:sfink] [:s:]]

I changed only the dlopen error message, though I guess it does allow use of prerror.h. From the title of this bug, that doesn't sound relevant here. (And I'm still trying to land it. I just tried again, so hopefully it won't bounce. Bug 1131424.)

Comment 7

[Tooru Fujisawa [:arai]]

(In reply to Jason Orendorff [:jorendorff] from comment #5)
> Hey arai, does this still apply? I think sfink made some changes to these
> error messages that might help.
> 
> Sorry for failing to review this. If you have a chance to take a look, I'd
> appreciate it. I'll look at it as soon as I see your response here.

as sfink wrote, there is no confliction, and the patch Part 1 (and others also) are still applicable :)

Comment 8

[Jason Orendorff [:jorendorff]]

Comment on attachment 8554439 [details] [diff] [review]
Part 1: Show information about value, type, function, and argument number in type conversion error messages in js-ctypes.

Review of attachment 8554439 [details] [diff] [review]:
-----------------------------------------------------------------

I apologize for the slow review here. Thank you for this epic patch. r=me with the comments below addressed.

::: js/src/ctypes/CTypes.cpp
@@ +877,5 @@
>  
> +static const char*
> +EncodeLatin1(JSContext* cx, AutoString& str, JSAutoByteString& bytes)
> +{
> +  return bytes.encodeLatin1(cx, NewUCString(cx, str));

Out-of-memory handling needs some work here.

Every place where EncodeLatin1 is called, the return value must be checked for null.

It's unlikely for an OOM to happen while we're generating a message for some other error, so don't bother doing anything fancy --- if EncodeLatin1 returns null, just return false or null to the caller.

Do the same for all callers of ValueToSourceForError, please.

@@ +881,5 @@
> +  return bytes.encodeLatin1(cx, NewUCString(cx, str));
> +}
> +
> +static const char*
> +ValueToSourceForError(JSContext* cx, HandleValue val, JSAutoByteString& bytes)

This seems generally useful. Please move this function to jsexn.h/cpp or something, where others can find it. js::ValueToSourceForError.

The only part that won't make sense in jsexn.cpp is the bit that calls CType::IsCType. So delete that part, but then add a special ctypes version of the function in this file:

    static const char *
    CTypesToSourceForError(JSContext *cx, HandleValue val, JSAutoByteString &bytes)
    {
      if (val.isObject() && (CType::IsCType(&val.toObject()) || CData::IsCData(&val.toObject))) {
        RootedString str(cx, JS_ValueToSource(cx, val));
        ...
      }
      return ValueToSourceForError(cx, val, bytes);
    }

@@ +915,5 @@
> +    }
> +  } else if (val.isInt32()) {
> +    AppendString(source, "the integer ");
> +  } else if (val.isDouble()) {
> +    AppendString(source, "the double ");

Please instead write:

    } else if (val.isNumber()) {
      AppendString(source, "the number ");

Whether a numeric value is represented as an int32 or a double is just an implementation detail; even error messages shouldn't talk to the user about it.

@@ +922,5 @@
> +  } else if (val.isBoolean()) {
> +    AppendString(source, "the boolean ");
> +  } else {
> +    MOZ_ASSERT(val.isSymbol());
> +    AppendString(source, "the symbol ");

I think "the symbol " here is redundant, since it'll end up saying things like

    the symbol Symbol("abc")
    the symbol Symbol.iterator

It already says "Symbol" in the ToSource for those.

Same goes for "the boolean "; I think it's better to just return "true" or "false".

@@ +993,5 @@
> +  FunctionInfo* fninfo = FunctionType::GetFunctionInfo(typeObj);
> +  BuildCStyleTypeSource(cx, fninfo->mReturnType, source);
> +  AppendString(source, " ");
> +  if (nameStr) {
> +    AppendString(source, nameStr);

Please add
    MOZ_ASSERT(ptrCount == 0);
(to explain to the reader why we don't handle both, not out of concern that it'll go wrong).

@@ +1232,5 @@
> +}
> +
> +static bool
> +ArgumentConvError(JSContext* cx, const char* expected, HandleValue actual,
> +                  const char* funStr, unsigned argIndex)

Please remove the unused `expected` argument.

@@ +1375,5 @@
> +}
> +
> +static bool
> +NonPrimitveError(JSContext* cx, HandleObject typeObj)
> +{

Typo: the third i in "primitive" is missing.

This only happens from the .value getter, so a better error message would be ".value only works on character and numeric types, not `struct Boat`"

@@ +3079,5 @@
>          if (nbytes == (size_t) -1)
>            return false;
>  
>          if (targetLength < nbytes) {
> +          MOZ_ASSERT(!funObj);

I guess the previous assertion, under `case TYPE_array: {`, covers all these cases, but keeping the extra assertions is fine in this case.

@@ +7763,5 @@
>    if (!obj)
>      return false;
>    if (!CDataFinalizer::IsCDataFinalizer(obj)) {
> +    JS_ReportError(cx, "not a CDataFinalizer");
> +    return false;

This used to call TypeError. Why not keep that?

@@ +7811,5 @@
>    if (!obj)
>      return false;
>    if (!CDataFinalizer::IsCDataFinalizer(obj)) {
> +    JS_ReportError(cx, "not a CDataFinalizer");
> +    return false;

Same here.

::: js/src/ctypes/Library.cpp
@@ +347,5 @@
>    if (!result)
>      return false;
>  
> +  if (isFunction)
> +    JS_SetReservedSlot(result, SLOT_FUNNAME, STRING_TO_JSVAL(nameStr));

use StringValue(nameStr) instead of STRING_TO_JSVAL.

Comment 9

[Tooru Fujisawa [:arai]]

Thank you for reviewing!

(In reply to Jason Orendorff [:jorendorff] from comment #8)
> @@ +1375,5 @@
> > +}
> > +
> > +static bool
> > +NonPrimitveError(JSContext* cx, HandleObject typeObj)
> > +{
> 
> Typo: the third i in "primitive" is missing.
> 
> This only happens from the .value getter, so a better error message would be
> ".value only works on character and numeric types, not `struct Boat`"

I think adding "struct " prefix here will make it inconsistent from other types:
  ".value only works on character and numeric types, not `Boat.ptr`"
  ".value only works on character and numeric types, not `Boat.array(10)`"
So just `Boat` would be nice.
  ".value only works on character and numeric types, not `Boat`"
Or perhaps is it better to change BuildTypeSource to add "struct " or something to StructType for `makeShort=true` case?

> @@ +7763,5 @@
> >    if (!obj)
> >      return false;
> >    if (!CDataFinalizer::IsCDataFinalizer(obj)) {
> > +    JS_ReportError(cx, "not a CDataFinalizer");
> > +    return false;
> 
> This used to call TypeError. Why not keep that?

Sorry I should've noted that they are in temporaly state.
They are different from other TypeError errors, since they're not type conversion, but type check of `this` value.
They're fixed in Part 5 with other places which handle type error of `this` value and `callee` value. So, in Part 1 I made them consistent with other places.
Error message will look like "CDataFinalizer.prototype.forget called on incompatible Number".
Is it okay?

Comment 10

[Tooru Fujisawa [:arai]]

> since they're not type conversion
Oops, I was wrong. TypeError is also not a type conversion error. I forgot that I separated ConvError from TypeError.
please ignore that sentence.
Anyway, I think making them consistent with other places might be better.

Comment 11

[Jason Orendorff [:jorendorff]]

(In reply to Tooru Fujisawa [:arai] from comment #9)
> > This only happens from the .value getter, so a better error message would be
> > ".value only works on character and numeric types, not `struct Boat`"
> 
> I think adding "struct " prefix here will make it inconsistent from other
> types:
>   ".value only works on character and numeric types, not `Boat.ptr`"
>   ".value only works on character and numeric types, not `Boat.array(10)`"
> So just `Boat` would be nice.
>   ".value only works on character and numeric types, not `Boat`"

I agree.

> Error message will look like "CDataFinalizer.prototype.forget called on
> incompatible Number".
> Is it okay?

Yes, that's fine.

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Report argument length error as RangeError in js-ctypes.

Thank you again!
Here are remaining parts (added 3 more parts from previous try run).
Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=09729280fb20

Part 2 makes error related to argument length to RangeError, and make messages consistent.
e.g.
  RangeError: StructType takes one or two arguments

Comment 13

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Report argument type error as TypeError in js-ctypes.

Part 3 makes error related to argument type or value to TypeError and RangeError,
including the message added in bug 1143281.
e.g.
  TypeError: first argument of StructType must be a string
  RangeError: argument of Int64.prototype.toString must be an integer at least 2 and no greater than 36

Currently "first " prefix isn't added if the function accepts only one argument, is it okay? or should I add "first " for that case too?

Comment 14

[Tooru Fujisawa [:arai]]

Attachment: Part 4: Show information about value in overflow error messages in js-ctypes.

Part 4 improves error messages related to overflow.
e.g.
  RangeError: the string "0xfffffffffffffffffffffff" does not fit to the type int32_t
  RangeError: struct size does not fit to size_t

Comment 15

[Tooru Fujisawa [:arai]]

Attachment: Part 5: Improve function name in this and callee type error messages in js-ctypes.

Part 5 improves error messages related to `this` and `callee` in native functions, as noted above.
e.g.
  TypeError: StructType.prototype.define called on incompatible object, got the object (new Number(1))
  TypeError: StructType property setter called on non-StructType CData, got ctypes.int32_t(0)
  TypeError: CType.prototype.toString called on incompatible Number
  TypeError: UInt64.prototype.toString called on non-UInt64 CData
  TypeError: CDataFinalizer.prototype.readString called on empty CDataFinalizer

Here, source representation of the `this` value is not shown in the error message for toString/toSource, since it may cause recursive error.

Then, Is it possible that CType::IsCType(obj) in CType::ConstructData fail?
If it's possible from JavaScript, I'd like to make testcases.

Comment 16

[Tooru Fujisawa [:arai]]

Attachment: Part 5: Show function name in this and callee type error messages in js-ctypes.

oops, that was old patch, sorry.

Comment 17

[Tooru Fujisawa [:arai]]

Attachment: Part 6: Show information about range and value in array index error messages in js-ctypes.

Part 6 improves error messages related to array index in ArrayType
e.g.
  TypeError: the number -1 is not a valid array index
  TypeError: Symbol("foo") is not a valid array index
  RangeError: array index 10 must be less than 10

Comment 18

[Tooru Fujisawa [:arai]]

Attachment: Part 7: Show information about value, type, function, and argument number in function related error messages in js-ctypes.

Part 7 improves error messages related to FunctionType construction and invokation
e.g.
  TypeError: the type of argument 1 is not a ctypes type (got the number 10)
  TypeError: return type is not a ctypes type (got the number 1)
  TypeError: cannot construct from FunctionType; use FunctionType.ptr instead
  RangeError: number of arguments does not match declaration of ctypes.FunctionType(ctypes.default_abi, ctypes.int32_t) (expected 0, got 1)
  TypeError: variadic argument 2 must be a CData object (got the number 20)

Comment 19

[Tooru Fujisawa [:arai]]

Attachment: Part 8: Show information about field name and type in struct field related error messages in js-ctypes.

Part 8 improves error messages related to StructType construction and field access
e.g.
  TypeError: struct field descriptors require a valid name and type (got the number 1)
  TypeError: struct field descriptors must contain one property (got the object ({x:1, y:2}) with 2 properties)
  TypeError: the number 1 is not a valid name of struct field descriptors
  TypeError: the number 1 is not a valid value of struct field descriptors for 'x' field
  struct field type must have defined and nonzero size (got ctypes.StructType("b") for 'x' field)
  TypeError: struct fields must have unique names, 'x' field appears twice
  TypeError: 'z' does not name a field

Comment 20

[Tooru Fujisawa [:arai]]

Attachment: Part 9: Report construction error as TypeError in js-ctypes.

Part 9 makes error related to construction to TypeError.
e.g.
  TypeError: cannot construct from void_t

Comment 21

[Tooru Fujisawa [:arai]]

Attachment: Part 10: Show information about value in pointer related error messages in js-ctypes.

Part 10 improves error messages related to PointerType
e.g.
  TypeError: cannot modify pointer of undefined size foo.ptr(ctypes.UInt64("0x0"))
  TypeError: cannot read contents of null pointer ctypes.int32_t.ptr(ctypes.UInt64("0x0"))
  TypeError: base type ctypes.int32_t.ptr(ctypes.UInt64("0x12345")) is not an 8-bit or 16-bit integer or character type

Comment 22

[Tooru Fujisawa [:arai]]

Attachment: Part 11: Show information about type in cast error messages in js-ctypes.

Part 11 improves error messages in ctypes.cast
e.g.
  TypeError: target type foo has undefined size
  TypeError: target type foo has larger size than source type ctypes.int32_t (8 > 4)

Comment 23

[Jason Orendorff [:jorendorff]]

Comment on attachment 8579228 [details] [diff] [review]
Part 2: Report argument length error as RangeError in js-ctypes.

Review of attachment 8579228 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with these minor changes:

Judging by how RangeError is used in ES6, we shouldn't be using it for this. The most likely error if you pass the wrong number of arguments to a JS function is TypeError. Let's use that.

::: js/src/ctypes/CTypes.cpp
@@ +3840,5 @@
>                        HandleObject obj,
>                        const CallArgs& args)
>  {
>    if (args.length() > 1) {
> +    return ArgumentLengthError(cx, "CType constructor", "zero or one", "");

"at most one", ""

@@ +4362,5 @@
>    }
>  
>    // Construct and return a new ArrayType object.
>    if (args.length() > 1) {
> +    return ArgumentLengthError(cx, "CType.prototype.array", "zero or one", "");

Same here.

@@ +4501,5 @@
>  ABI::ToSource(JSContext* cx, unsigned argc, jsval* vp)
>  {
>    CallArgs args = CallArgsFromVp(argc, vp);
>    if (args.length() != 0) {
> +    return ArgumentLengthError(cx, "ABI.prototype.toSource", "zero", "s");

"no", ""

The phrasing "takes no arguments" is a bit better than "takes zero arguments".

The same thing happens maybe 6 other places in this file.

@@ +4947,5 @@
>    // with a length argument, or with an actual JS array.
>    if (CType::IsSizeDefined(obj)) {
>      if (args.length() > 1) {
> +      return ArgumentLengthError(cx, "size defined ArrayType constructor",
> +                                 "zero or one", "");

"at most one" again

Comment 24

[Tooru Fujisawa [:arai]]

Thank you for reviewing :)
I'll land Part1-3 first. In most case Part 1 will solve the debugging difficulty (mostly "expected type pointer, got ..."
)

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/564184f293a6
https://hg.mozilla.org/integration/mozilla-inbound/rev/28bbd9862551
https://hg.mozilla.org/integration/mozilla-inbound/rev/43f5ca4913df

Comment 26

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/139ae480928c
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecdde43f6d6a

Comment 27

[Tooru Fujisawa [:arai]]

sorry, I overlooked that js-ctypes tests exist outside js/src.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Also: https://treeherder.mozilla.org/logviewer.html#?job_id=9001639&repo=mozilla-inbound

Comment 29

[Tooru Fujisawa [:arai]]

one more backout changeset
https://hg.mozilla.org/integration/mozilla-inbound/rev/f56304442f9f

Comment 30

[Tooru Fujisawa [:arai]]

Attachment: Part 1 followup: Update test_jsctypes.js.

updated toolkit/components/ctypes/tests/unit/test_jsctypes.js for each parts, just by changing Error to TypeError or RangeError for each assertion.
for Part 1-3 followup, I'll merge them later.

Comment 31

[Tooru Fujisawa [:arai]]

Attachment: Part 2 followup: Update test_jsctypes.js.

Comment 32

[Tooru Fujisawa [:arai]]

Attachment: Part 3 followup: Update test_jsctypes.js.

Comment 33

[Tooru Fujisawa [:arai]]

Attachment: Part 4: Show information about value in overflow error messages in js-ctypes.

Comment 34

[Tooru Fujisawa [:arai]]

Attachment: Part 5: Show function name in this and callee type error messages in js-ctypes.

Comment 35

[Tooru Fujisawa [:arai]]

Attachment: Part 6: Show information about range and value in array index error messages in js-ctypes.

Comment 36

[Tooru Fujisawa [:arai]]

Attachment: Part 7: Show information about value, type, function, and argument number in function related error messages in js-ctypes.

Comment 37

[Tooru Fujisawa [:arai]]

Attachment: Part 8: Show information about field name and type in struct field related error messages in js-ctypes.

Comment 38

[Tooru Fujisawa [:arai]]

Attachment: Part 9: Report construction error as TypeError in js-ctypes.

Comment 39

[Tooru Fujisawa [:arai]]

Attachment: Part 10: Show information about value in pointer related error messages in js-ctypes.

Comment 40

[Tooru Fujisawa [:arai]]

Attachment: Part 11: Show information about type in cast error messages in js-ctypes.

Comment 41

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b2b956ba0acd
https://hg.mozilla.org/integration/mozilla-inbound/rev/951ec7d134c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/467559ddc08f

Comment 42

[Tooru Fujisawa [:arai]]

backed out for xpcshell-test failure
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bd7ce78571b
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1c096248e7f
https://hg.mozilla.org/integration/mozilla-inbound/rev/20da7a0125b5

Comment 43

[Tooru Fujisawa [:arai]]

all failures were trivial issue that test code didn't follow the change on Error type or message.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8f8f267cee2
I'll try to land them again after tree open.

Comment 44

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8929ee2c2f56
https://hg.mozilla.org/integration/mozilla-inbound/rev/e364d76c8039
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c3f8affe0fd

Comment 45

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8929ee2c2f56
https://hg.mozilla.org/mozilla-central/rev/e364d76c8039
https://hg.mozilla.org/mozilla-central/rev/7c3f8affe0fd

Comment 46

[Jason Orendorff [:jorendorff]]

Comment on attachment 8594457 [details] [diff] [review]
Part 4: Show information about value in overflow error messages in js-ctypes.

Review of attachment 8594457 [details] [diff] [review]:
-----------------------------------------------------------------

Arai, do you still have these patches around? Interested in landing them? I can review today, if so.

Here's one to start on ... sorry for the long wait :( :( I'm slowly getting out from under a huge bugzilla backlog.

::: js/src/ctypes/CTypes.cpp
@@ +4957,4 @@
>        return nullptr;
>      }
> +    if (!SizeTojsval(cx, size, &sizeVal)) {
> +      SizeOverflow(cx, "array size", "double");

Instead of "double", say "JavaScript number", as that is the real limitation here.

@@ +5634,5 @@
>    }
>  
>    RootedValue sizeVal(cx);
> +  if (!SizeTojsval(cx, structSize, &sizeVal)) {
> +    SizeOverflow(cx, "struct size", "double");

"JavaScript number" here too.

@@ +8187,5 @@
> +  bool overflow = false;
> +  if (!jsvalToBigInteger(cx, args[0], true, &i, &overflow)) {
> +    if (overflow) {
> +      return TypeOverflow(cx, "int64", args[0]);
> +    }

Style nit: no curly braces on this if-statement.

@@ +8363,5 @@
> +  bool overflow = false;
> +  if (!jsvalToBigInteger(cx, args[0], true, &u, &overflow)) {
> +    if (overflow) {
> +      return TypeOverflow(cx, "uint64", args[0]);
> +    }

Same here.

::: js/src/ctypes/ctypes.msg
@@ +39,5 @@
>  MSG_DEF(CTYPESMSG_WRONG_ARG_LENGTH,3, JSEXN_TYPEERR, "{0} takes {1} argument{2}")
> +
> +/* overflow */
> +MSG_DEF(CTYPESMSG_SIZE_OVERFLOW, 2, JSEXN_RANGEERR, "{0} does not fit to {1}")
> +MSG_DEF(CTYPESMSG_TYPE_OVERFLOW, 2, JSEXN_RANGEERR, "{0} does not fit to the type {1}")

"fit in", not "fit to"

::: js/src/jit-test/tests/ctypes/size-overflow-struct.js
@@ +1,4 @@
> +load(libdir + 'asserts.js');
> +
> +function test() {
> +  if (getBuildConfiguration().x86) {

getBuildConfiguration()["pointer-byte-size"] tells if you're on a 64-bit platform or not.

Comment 47

[Jason Orendorff [:jorendorff]]

Comment on attachment 8594458 [details] [diff] [review]
Part 5: Show function name in this and callee type error messages in js-ctypes.

Review of attachment 8594458 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing review flags for now. Arai will upload new patches.

Comment 48

[Tooru Fujisawa [:arai]]

Attachment: Part 5: Show function name in this and callee type error messages in js-ctypes.

just rebased

Comment 49

[Tooru Fujisawa [:arai]]

Attachment: Part 6: Show information about range and value in array index error messages in js-ctypes.

Change from previous one: now property access with symbol value doesn't throw, so removed a testcase for symbol.

Comment 50

[Tooru Fujisawa [:arai]]

Attachment: Part 7: Show information about value, type, function, and argument number in function related error messages in js-ctypes.

and rebased others

Comment 51

[Tooru Fujisawa [:arai]]

Attachment: Part 8: Show information about field name and type in struct field related error messages in js-ctypes.

Comment 52

[Tooru Fujisawa [:arai]]

Attachment: Part 9: Report construction error as TypeError in js-ctypes.

Comment 53

[Tooru Fujisawa [:arai]]

Attachment: Part 10: Show information about value in pointer related error messages in js-ctypes.

Comment 54

[Tooru Fujisawa [:arai]]

Attachment: Part 11: Show information about type in cast error messages in js-ctypes.

Comment 55

[Jason Orendorff [:jorendorff]]

Comment on attachment 8729225 [details] [diff] [review]
Part 5: Show function name in this and callee type error messages in js-ctypes.

Review of attachment 8729225 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ctypes/CTypes.cpp
@@ +7456,5 @@
>                                 "no", "s");
>    }
>  
> +  bool isEmpty = false;
> +  JSObject* obj = CDataFinalizer::GetCData(cx, JS_THIS_OBJECT(cx, vp), &isEmpty);

Wow. Filed bug 1255800 to remove JS_THIS_OBJECT.

@@ +7736,5 @@
>    return valData.toObjectOrNull();
>  }
>  
>  JSObject*
> +CDataFinalizer::GetCData(JSContext* cx, JSObject* obj, bool* isEmpty)

Custom error handling has been a running problem for the JS engine.

The rule about error handling is, each method should either always or never report an error on failure (null or false return). That way it's obvious when something is wrong. What you've written here looks almost totally correct (ConvertToJS can OOM), but it'll be harder for the next maintainer to come along and correctly update things because the contract is unclear.

I think CDataFinalizer::GetCData and GetValue should always report. Instead of isEmpty, pass in a const char *methodName, and include that in the error message if it's non-null.

Comment 56

[Jason Orendorff [:jorendorff]]

Comment on attachment 8729226 [details] [diff] [review]
Part 6: Show information about range and value in array index error messages in js-ctypes.

Review of attachment 8729226 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ctypes/ctypes.msg
@@ +24,5 @@
>  /* array */
>  MSG_DEF(CTYPESMSG_ARRAY_MISMATCH,4, JSEXN_TYPEERR, "length of {0} does not match to the length of the type {1} (expected {2}, got {3})")
>  MSG_DEF(CTYPESMSG_ARRAY_OVERFLOW,4, JSEXN_TYPEERR, "length of {0} does not fit in the length of the type {1} (expected {2} or lower, got {3})")
> +MSG_DEF(CTYPESMSG_INVALID_INDEX, 1, JSEXN_TYPEERR, "{0} is not a valid array index")
> +MSG_DEF(CTYPESMSG_INVALID_RANGE, 2, JSEXN_RANGEERR, "array index {0} must be less than {1}")

how about: "array index {0} is out of bounds for array of length {1}"

Comment 57

[Jason Orendorff [:jorendorff]]

Comment on attachment 8729227 [details] [diff] [review]
Part 7: Show information about value, type, function, and argument number in function related error messages in js-ctypes.

Review of attachment 8729227 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ctypes/CTypes.cpp
@@ +1001,5 @@
>  }
>  
>  static void
> +BuildFunctionTypeSource(JSContext* cx, HandleObject funObj, AutoString& source,
> +                        bool noData = false)

This doesn't seem to be used anywhere.

Comment 58

[Jason Orendorff [:jorendorff]]

Comment on attachment 8729228 [details] [diff] [review]
Part 8: Show information about field name and type in struct field related error messages in js-ctypes.

Review of attachment 8729228 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ctypes/CTypes.cpp
@@ +6211,5 @@
>    FieldInfoHash::Ptr ptr = GetFieldInfo(obj)->lookup(name);
>    if (ptr)
>      return &ptr->value();
>  
> +  FieldMissingError(cx, name);

It would be nice to include the type name in this error message.

    TypeError: struct 'a' does not have a field named 'z'

Not required, though, if you just want to land this!

::: js/src/ctypes/ctypes.msg
@@ +32,5 @@
> +MSG_DEF(CTYPESMSG_FIELD_DESC_COUNT,2, JSEXN_TYPEERR, "struct field descriptors must contain one property (got {0} with {1} properties)")
> +MSG_DEF(CTYPESMSG_FIELD_DESC_NAME,1, JSEXN_TYPEERR, "{0} is not a valid name of struct field descriptors")
> +MSG_DEF(CTYPESMSG_FIELD_DESC_SIZE,2, JSEXN_TYPEERR, "struct field type must have defined and nonzero size (got {0} for '{1}' field)")
> +MSG_DEF(CTYPESMSG_FIELD_DESC_TYPE,1, JSEXN_TYPEERR, "struct field descriptors require a valid name and type (got {0})")
> +MSG_DEF(CTYPESMSG_FIELD_DESC_VALUE,2, JSEXN_TYPEERR, "{0} is not a valid value of struct field descriptors for '{1}' field")

The word "value" seems odd - you mean "not a valid type", right?

Comment 59

[Jason Orendorff [:jorendorff]]

Comment on attachment 8729235 [details] [diff] [review]
Part 11: Show information about type in cast error messages in js-ctypes.

Review of attachment 8729235 [details] [diff] [review]:
-----------------------------------------------------------------

Wonderful. Thanks so much for doing this. I apologize for the delay.

Comment 60

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b2624519692fb70a1c09d37a7f6a8c1bf1a9f7b0
Bug 891107 - Part 4: Show information about value in overflow error messages in js-ctypes. r=jorendorff

https://hg.mozilla.org/integration/mozilla-inbound/rev/13e045fff28d32c498821dc0d3f094140f1f1a36
Bug 891107 - Part 5: Show function name in this and callee type error messages in js-ctypes. r=jorendorff

https://hg.mozilla.org/integration/mozilla-inbound/rev/8fe18f2b8aa9240e032a935f9af3d99e180a9f84
Bug 891107 - Part 6: Show information about range and value in array index error messages in js-ctypes. r=jorendorff

https://hg.mozilla.org/integration/mozilla-inbound/rev/ebd8c98e232911cfb61cf416d8466979a257b0e1
Bug 891107 - Part 7: Show information about value, type, function, and argument number in function related error messages in js-ctypes. r=jorendorff

https://hg.mozilla.org/integration/mozilla-inbound/rev/c47e7505bd94cec93f10756473bfa9b415ada737
Bug 891107 - Part 8: Show information about field name and type in struct field related error messages in js-ctypes. r=jorendorff

https://hg.mozilla.org/integration/mozilla-inbound/rev/f223b115ad37d91bb62907afab41646531622006
Bug 891107 - Part 9: Report construction error as TypeError in js-ctypes. r=jorendorff

https://hg.mozilla.org/integration/mozilla-inbound/rev/07d1058b745e603593394ce9465e91669256e4d4
Bug 891107 - Part 10: Show information about value in pointer related error messages in js-ctypes. r=jorendorff

https://hg.mozilla.org/integration/mozilla-inbound/rev/c1bb74893286a8a73eb76700ea65bd5470b77da8
Bug 891107 - Part 11: Show information about type in cast error messages in js-ctypes. r=jorendorff

Comment 61

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3acdbde976fad82c908f901e7653500659ec3030
Backed out changeset c1bb74893286 (bug 891107)

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea30fdf3bca31dbf290476e3311cadd2fe43f739
Backed out changeset 07d1058b745e (bug 891107)

https://hg.mozilla.org/integration/mozilla-inbound/rev/eca08d1ba01688b9eaa8319b25fefe88d4cbef4f
Backed out changeset f223b115ad37 (bug 891107)

https://hg.mozilla.org/integration/mozilla-inbound/rev/62e9ab66609476c433dfef2656e98d680b5f0adf
Backed out changeset c47e7505bd94 (bug 891107)

https://hg.mozilla.org/integration/mozilla-inbound/rev/65e48fb8295b00503a957b3ea12f8791a4d45e6e
Backed out changeset ebd8c98e2329 (bug 891107)

https://hg.mozilla.org/integration/mozilla-inbound/rev/69104d84a78149ff790633ba4dd8af1220494267
Backed out changeset 8fe18f2b8aa9 (bug 891107)

https://hg.mozilla.org/integration/mozilla-inbound/rev/750308ebc10ab2aa10740899df8d26a22a846170
Backed out changeset 13e045fff28d (bug 891107)

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d5cb3a1603c0511aaff722751c28717b15806e3
Backed out changeset b2624519692f (bug 891107)

Comment 62

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f74c36a3e7a7d75e1261bdb4903b9fd44fcec93
Bug 891107 - Part 4: Show information about value in overflow error messages in js-ctypes. r=jorendorff

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc5171ce2a4b96c9f8b4a8f3c3a2c7074cd440c9
Bug 891107 - Part 5: Show function name in this and callee type error messages in js-ctypes. r=jorendorff

https://hg.mozilla.org/integration/mozilla-inbound/rev/daa58aad68585df2ae33b2432d17a5296cb150da
Bug 891107 - Part 6: Show information about range and value in array index error messages in js-ctypes. r=jorendorff

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e8e3d17142899118207b146ed5b994127722ef3
Bug 891107 - Part 7: Show information about value, type, function, and argument number in function related error messages in js-ctypes. r=jorendorff

https://hg.mozilla.org/integration/mozilla-inbound/rev/d96ab48369288689e7452ba168e1d3bf54673421
Bug 891107 - Part 8: Show information about field name and type in struct field related error messages in js-ctypes. r=jorendorff

https://hg.mozilla.org/integration/mozilla-inbound/rev/a5ea955d64f2174ebd018fb1c9ce36486a9910c3
Bug 891107 - Part 9: Report construction error as TypeError in js-ctypes. r=jorendorff

https://hg.mozilla.org/integration/mozilla-inbound/rev/000e3f54a3d139bc2f3987a257f78bd74e475bd6
Bug 891107 - Part 10: Show information about value in pointer related error messages in js-ctypes. r=jorendorff

https://hg.mozilla.org/integration/mozilla-inbound/rev/dec8c7173f68246a405439a30d057180e15067b4
Bug 891107 - Part 11: Show information about type in cast error messages in js-ctypes. r=jorendorff

Comment 63

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9f74c36a3e7a
https://hg.mozilla.org/mozilla-central/rev/fc5171ce2a4b
https://hg.mozilla.org/mozilla-central/rev/daa58aad6858
https://hg.mozilla.org/mozilla-central/rev/1e8e3d171428
https://hg.mozilla.org/mozilla-central/rev/d96ab4836928
https://hg.mozilla.org/mozilla-central/rev/a5ea955d64f2
https://hg.mozilla.org/mozilla-central/rev/000e3f54a3d1
https://hg.mozilla.org/mozilla-central/rev/dec8c7173f68