kaitai_struct icon indicating copy to clipboard operation
kaitai_struct copied to clipboard

It is very hard to debug `ValidationNotEqualError` without offsets

Open Mingun opened this issue 1 year ago • 4 comments

In JS current implementation of ValidationNotEqualError (and other errors) does not contain an offset, which makes debug of KSY a nightmare. However, JS generator in Web-IDE shows, that constructor gets much more parameters, that it handles:

Cassette.prototype._read = function() {
  this._debug._unnamed0 = { start: this._io.pos, ioOffset: this._io.byteOffset };
  this._unnamed0 = this._io.readBytes(1);
  this._debug._unnamed0.end = this._io.pos;
  if (!((KaitaiStream.byteArrayCompare(this._unnamed0, [30]) == 0))) {
    throw new KaitaiStream.ValidationNotEqualError([30], this._unnamed0, this._io, "/types/status/types/supply_counts/types/ci01/types/cassette/seq/0");
  }
  ...
}

https://github.com/kaitai-io/kaitai_struct_javascript_runtime/blob/03c8bf24e357cff62b6269d106f87650c3c0c578/KaitaiStream.js#L791-L797

The error message in Web-IDE is very unhelpful:

Parse error (ValidationNotEqualError): not equal, expected [30], but got [48] <...stack...>

  1. first, there no offset where the error is occurred
  2. second, it shows decimal values, whereas HEX editor shows HEX values, so it is hard to match one with other
  3. third, it is unclear in which field error is happened

Mingun avatar Aug 02 '22 07:08 Mingun

This has been on my radar for a while, but for adding common location info to JS exceptions (i.e. io to pull the byte offset from, and srcPath - the YAML path to the field definition in the .ksy), some code reuse pattern should be employed (otherwise there would be a bunch of duplication and we don't want that). It would make sense to use inheritance as in Java so that it's easier to catch all types of validation errors in JS as well (as if (e instanceof ValidationFailedError) ...), but the issue is that the JavaScript runtime should still work on older JS versions than ES6, so you can't really use ES6 classes for that. And trying to do emulate inheritance in pre-ES6 versions is a hassle.

It would be nice to port JavaScript runtime to TypeScript, i.e. resolve conflicts, review and merge https://github.com/kaitai-io/kaitai_struct_javascript_runtime/pull/25, but I haven't found the time to do it yet. Another problem with the JS runtime written in TS can be that its distribution will be more complicated, because the JS code needs to be built from source using the TypeScript compiler (I see that rollup.js is used for that, it seems solid at first glance but we have to look out for problems nonetheless). Nothing that can't be done, but it still looks like a chunk of work.


3. third, it is unclear in which field error is happened

I know it's probably not exactly what you hoped for, but the stack trace actually contains the line number where the error was thrown (the following is from Google Chrome):

Parse error (ValidationNotEqualError): not equal, expected [80,75], but got [17,0]
Call stack: Error
    at new KaitaiStream.ValidationNotEqualError (https://ide.kaitai.io/lib/_npm/kaitai-struct/KaitaiStream.js:796:17)
    at Zip.PkSection.PkSection._read (eval at initCode (https://ide.kaitai.io/js/v1/kaitaiWorker.js:89:9), <anonymous>:567:15)
    at Zip._read (eval at initCode (https://ide.kaitai.io/js/v1/kaitaiWorker.js:89:9), :145:19)
    at reparse (https://ide.kaitai.io/js/v1/kaitaiWorker.js:96:17)
    at myself.onmessage (https://ide.kaitai.io/js/v1/kaitaiWorker.js:117:47)

Web IDE screenshot of the "JS code (debug)" showing the line where the validation error was thrown

generalmimon avatar Aug 07 '22 19:08 generalmimon

Yes, I actually use stack trace and debug code for now to understand, what is wrong, but that is very hard work. Anyway, it still say nothing about actual position in a parsed stream, I can only guess about it.

It would make sense to use inheritance as in Java so that it's easier to catch all types of validation errors in JS as well (as if (e instanceof ValidationFailedError) ...), but the issue is that the JavaScript runtime should still work on older JS versions than ES6, so you can't really use ES6 classes for that.

While this is a good idea, I don't see how it is block the current problem. The information about offset and a path can be

  • added to the message property of an exception
  • written to a custom field of exception from which it can be retrieved just by inspecting this property. If property does not exist, it means that error is not a KaitaiStruct error

Mingun avatar Aug 08 '22 07:08 Mingun

@Mingun:

While this is a good idea, I don't see how it is block the current problem. The information about offset and a path can be

  • added to the message property of an exception

Well, yes, but without any pattern to reuse code, you'll have to copy it to a few places, and any changes of the common error message format (for example) become a nightmare.

A poor man's way to reuse code could be something similar to what I did in Go (because classical inheritance is not available here):

kaitai/error.go:36-43

func (l locationInfo) msgWithLocation(msg string) string {
	var pos interface{}
	pos, err := l.io.Pos()
	if err != nil {
		pos = "N/A"
	}
	return fmt.Sprintf("%s: at pos %v: %s", l.srcPath, pos, msg)
}

kaitai/error.go:52-54

func validationFailedMsg(msg string) string {
	return "validation failed: " + msg
}

kaitai/error.go:79-85

func (e ValidationNotEqualError) Error() string {
	return e.msgWithLocation(
		validationFailedMsg(
			fmt.Sprintf("not equal, expected %v, but got %v", e.expected, e.actual),
		),
	)
}

That could indeed be implemented in JavaScript quite easily, if we're talking about temporary "low-hanging fruit" workarounds. But I would prefer inheritance in the future.

generalmimon avatar Aug 08 '22 11:08 generalmimon

Honestly, I don't see how the variant

var MyError = function(...) {
  init(this, ...);
}

differs from the variant

class MyError {
  constructor(...) {
    super(...);
  }
}

and why you think that this is kind of "workaround". In both cases it is just call of some function, with some sugar in the second case.

Mingun avatar Aug 08 '22 15:08 Mingun