-
-
Notifications
You must be signed in to change notification settings - Fork 6.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
improve error handling #1152
Comments
Are you talking about runtime errors here too? Or compile-time?
There are new exceptions since 3.0, however they're in the |
i mean run-time, yeah, like |
so do these exceptions propagate out? thing is the user really wants line/col info, so it would be nice if the exceptio would report that also. the parser would simply have to keep track of newlines as it is chugging along. again i would say well worth it. |
but yeah, a first remedy would be exposing them, so we can catch them and retro-provide line information. still think it would be worthwhile collecting line inf oduring parsing. its a simple matter of counting line breaks, and the number of bytes since the last line break...or do you think it would be very complicated? |
Unfortunately, I do know very little about the parsing/lexing aspect of this library. This is a question for the maintainer :) |
too a quick look at the parser. so adding line counting would be quite simply done in the lexer. then current_position would return row/col instead of byte count, or both. that could then be put directly into parse_error (which actually seems to already be exposed via basic_json::parse_error). adding source info to the json value is also simply done when constructing it in the parser. the sax interface would simply take source location as an argument for the events. i will think about how the source info can be made optional, probaby using a second template argument to basic_json. |
ok, was possible, did it on out branch. i will send a pull request sometime where source locations per json value will be off by default (with no penalty) and easy to opt in to |
also tomorrow will push to a public branch and link that here |
so, why wait: |
This change would (strictly speaking) break compatibility with the 3.x.x versions and should not be added before version 4.0.0. This is not necessarily a bad thing - I just think that if we want to improve the diagnosis information, we may want to think bigger. For instance, we may think about using What do you think? |
well, it should be doable that programs usually just compile, but abi may break, and the change would be detectable, as some types change. |
also for the parser error you can just directly generate all that information at the throw site, i.e. in the parser, as you should have it all right there,... |
So what is your proposal? |
Any ideas on this? |
Hmm, i can’t think of info beyond Source location that is needed. I don’t think Lexer state is needed. If parsing fails then we will not get a result anyway. My suggestion would still be just a source info type which is a template parameter. And the possibility to omit it at no cost (probably using empty base class optimization). Question then is what to use as default.
Gruß!
Daniel Oberhoff
… Am 18.07.2018 um 13:31 schrieb Niels Lohmann ***@***.***>:
Any ideas on this?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
I don't understand. Can you make an example? |
I am traveling, but maybe I can code it up rough next week sometime on my clone |
Any news on this? |
i have a working solution here:
why is this needed? i think this is because of the private base class, but honestly i got dizzy reading the concept... |
btw, the base class is needed to properly support opting out, because only this way wil an empty source location storage truly be empty, due to the empty base class optimization (usually in c++ classes cann not be smaller than 1 byte) |
I am currently not sure how to proceed here. I would need a concrete example with concrete expected error messages. |
example: |
i.e. usage errors on a json object now tell me the source location of the object. helps enormously, since json does not have a schema. |
very simple in principle. just preserve source location along with the json objects. |
oops, wrong button, sorry |
basically this servers the use case where json is used as configuration language, and the config file is broken |
This would mean an overhead of a string and at least one integer per JSON value. Right? |
3 integers: byte pos, row, column. by the string i suppose you mean the filename. no, that is not saved. the data maybe from memory also, i.e. a rest resource loaded from http or something, but location info is still useful since the resource can also be viewed in a browser. |
other alternatives are shorter integer types, or ommitting any of the values (i.e. row alone could often be enough) |
This would only work if the value is unchanged between parsing and the error message. What would you do if a value is changed after the fact? |
change would mean re-assignment i guess? or do you allow in-place changes beyond container modifications? for re-assignments the source location would be overwritten too. for container modifications i would just leave the old info there. this might not be academically correct for some definition of correct, but i would consider this a case of perfect vs good. |
I mean json j = parse(...);
j["foo"] = false;
std::string s = j["foo"]; The last line would trigger an exception. For this, we would not have source locations, right? |
yes, but i think that actually makes sense, because there is none, at least not at runtime |
if insteade on the last line you where to index with an unknown key you would in turn get one. |
Howdy. I was poking around just for this, wondering if there was a way to customize the error messages, especially for source line info. I was thinking of hacking this into the parse function myself... once the parse is done, I don't see how source locations are valid. It might be nice if the exception class had a few more fields than 'id' so we could construct our own error messages. For instance, name ( I've spent all of 10 minutes looking at this, but I'd say have lexer.get_position() return a struct with byte, line, and column? |
P.S. If one forgets a comma between objects, e.g.
I get 'unexpected string literal; expected '}' ... should be: expected ',' or '}' |
Customizing the message would bevrelatively easy |
Also FYI for me it was about errors also in usage, i.e. after parsing |
In the code where I'm using this lib I report errors in usage by the path in the json tree, e.g.
might yield something like "error [foo.baz] should be a string" But this is done with a custom front end class. I don't think that functionality belongs in this library, keeping track of that goes beyond the json spec. But, I do agree that reporting parse errors by line and column (or at least having this info available to the client) would be quite useful. I've got this working via a 'position' struct. |
@efp Thanks for reporting the issue in |
@efp the last case you talked about is exactly why i added source location to the json data. while i agree that validation may not belong in the json code (though since there are things like bson json schema may also be of benefit?). but without the sourvce info a frontend cannot tell the user where the error happened in the config file. |
Here's what I did: https://github.com/efp/json/tree/parse_error_lines_cols Let me know if you'd like it as a pull request. |
@efp This looks good! I would be happy to have a PR! |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
we commonly get quite horrible errors from the parser or when converting to user types. also error handling is not even mentioned in the docs.
I would propose making error handling a first class citizen, report lines/columns, give more detailed info in conversion to user types, etc.
I am willing to help, as this library is currently quite integral to our codebase
The text was updated successfully, but these errors were encountered: