[js-ctypes] TypeError messages should be more intelligible

RESOLVED FIXED in mozilla48

Status

()

defect
RESOLVED FIXED
6 years ago
Last year

People

(Reporter: Yoric, Assigned: arai)

Tracking

unspecified
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(14 attachments, 16 obsolete attachments)

87.63 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
35.69 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
23.08 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
7.50 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
11.63 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
11.69 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
28.02 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
52.96 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
8.20 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
18.61 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
16.57 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
6.47 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
14.00 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
6.95 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
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.
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.
Assignee

Updated

5 years ago
Duplicate of this bug: 1123047
Assignee

Updated

5 years ago
Blocks: 551057
Assignee

Comment 3

4 years ago
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`.
Attachment #8554439 - Flags: review?(benjamin)
Assignee

Comment 4

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

Updated

4 years ago
Attachment #8554439 - Flags: review?(benjamin) → review?(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.
Flags: needinfo?(arai.unmht)
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.)
Assignee

Comment 7

4 years ago
(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 :)
Flags: needinfo?(arai.unmht)
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.
Attachment #8554439 - Flags: review?(jorendorff) → review+
Assignee

Comment 9

4 years ago
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?
Flags: needinfo?(jorendorff)
Assignee

Comment 10

4 years ago
> 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.
(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.
Flags: needinfo?(jorendorff)
Assignee

Comment 12

4 years ago
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
Attachment #8579228 - Flags: review?(jorendorff)
Assignee

Comment 13

4 years ago
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?
Attachment #8579230 - Flags: review?(jorendorff)
Assignee

Comment 14

4 years ago
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
Attachment #8579231 - Flags: review?(jorendorff)
Assignee

Comment 15

4 years ago
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.
Attachment #8579232 - Flags: review?(jorendorff)
Assignee

Comment 16

4 years ago
oops, that was old patch, sorry.
Attachment #8579232 - Attachment is obsolete: true
Attachment #8579232 - Flags: review?(jorendorff)
Attachment #8579234 - Flags: review?(jorendorff)
Assignee

Comment 17

4 years ago
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
Attachment #8579235 - Flags: review?(jorendorff)
Assignee

Comment 18

4 years ago
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)
Attachment #8579236 - Flags: review?(jorendorff)
Assignee

Comment 19

4 years ago
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
Attachment #8579238 - Flags: review?(jorendorff)
Assignee

Comment 20

4 years ago
Part 9 makes error related to construction to TypeError.
e.g.
  TypeError: cannot construct from void_t
Attachment #8579239 - Flags: review?(jorendorff)
Assignee

Comment 21

4 years ago
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
Attachment #8579240 - Flags: review?(jorendorff)
Assignee

Comment 22

4 years ago
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)
Attachment #8579242 - Flags: review?(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
Attachment #8579228 - Flags: review?(jorendorff) → review+
Attachment #8579230 - Flags: review?(jorendorff) → review+
Assignee

Comment 24

4 years ago
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 ..."
)
Keywords: leave-open
Assignee

Comment 27

4 years ago
sorry, I overlooked that js-ctypes tests exist outside js/src.
Assignee

Comment 30

4 years ago
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.
Attachment #8594453 - Flags: review?(jorendorff)
Assignee

Comment 31

4 years ago
Attachment #8594454 - Flags: review?(jorendorff)
Assignee

Comment 32

4 years ago
Attachment #8594455 - Flags: review?(jorendorff)
Assignee

Comment 33

4 years ago
Assignee: nobody → arai.unmht
Attachment #8579231 - Attachment is obsolete: true
Attachment #8579231 - Flags: review?(jorendorff)
Attachment #8594457 - Flags: review?(jorendorff)
Assignee

Comment 34

4 years ago
Attachment #8579234 - Attachment is obsolete: true
Attachment #8579234 - Flags: review?(jorendorff)
Attachment #8594458 - Flags: review?(jorendorff)
Assignee

Comment 35

4 years ago
Attachment #8579235 - Attachment is obsolete: true
Attachment #8579235 - Flags: review?(jorendorff)
Attachment #8594459 - Flags: review?(jorendorff)
Assignee

Comment 37

4 years ago
Attachment #8579238 - Attachment is obsolete: true
Attachment #8579238 - Flags: review?(jorendorff)
Attachment #8594461 - Flags: review?(jorendorff)
Assignee

Comment 38

4 years ago
Attachment #8579239 - Attachment is obsolete: true
Attachment #8579239 - Flags: review?(jorendorff)
Attachment #8594462 - Flags: review?(jorendorff)
Assignee

Comment 39

4 years ago
Attachment #8579240 - Attachment is obsolete: true
Attachment #8579240 - Flags: review?(jorendorff)
Attachment #8594463 - Flags: review?(jorendorff)
Assignee

Comment 40

4 years ago
Attachment #8579242 - Attachment is obsolete: true
Attachment #8579242 - Flags: review?(jorendorff)
Attachment #8594464 - Flags: review?(jorendorff)
Attachment #8594453 - Flags: review?(jorendorff) → review+
Attachment #8594454 - Flags: review?(jorendorff) → review+
Attachment #8594455 - Flags: review?(jorendorff) → review+
Assignee

Comment 43

4 years ago
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 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.
Attachment #8594457 - Flags: review?(jorendorff) → review+
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.
Attachment #8594458 - Flags: review?(jorendorff)
Attachment #8594459 - Flags: review?(jorendorff)
Attachment #8594460 - Flags: review?(jorendorff)
Attachment #8594461 - Flags: review?(jorendorff)
Attachment #8594462 - Flags: review?(jorendorff)
Attachment #8594463 - Flags: review?(jorendorff)
Attachment #8594464 - Flags: review?(jorendorff)
Assignee

Comment 48

3 years ago
just rebased
Attachment #8594458 - Attachment is obsolete: true
Attachment #8729225 - Flags: review?(jorendorff)
Assignee

Comment 49

3 years ago
Change from previous one: now property access with symbol value doesn't throw, so removed a testcase for symbol.
Attachment #8594459 - Attachment is obsolete: true
Attachment #8729226 - Flags: review?(jorendorff)
Assignee

Comment 52

3 years ago
Attachment #8594462 - Attachment is obsolete: true
Attachment #8729231 - Flags: review?(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.
Attachment #8729225 - Flags: review?(jorendorff) → review+
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}"
Attachment #8729226 - Flags: review?(jorendorff) → review+
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.
Attachment #8729227 - Flags: review?(jorendorff) → review+
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?
Attachment #8729228 - Flags: review?(jorendorff) → review+
Attachment #8729231 - Flags: review?(jorendorff) → review+
Attachment #8729233 - Flags: review?(jorendorff) → review+
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.
Attachment #8729235 - Flags: review?(jorendorff) → review+
Assignee

Comment 60

3 years ago
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
Assignee

Comment 62

3 years ago
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
Assignee

Updated

3 years ago
Status: NEW → RESOLVED
Closed: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Duplicate of this bug: 551057
You need to log in before you can comment on or make changes to this bug.