Skip to content
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

proposal: encoding/json: export the offset method of the Decoder #23331

Closed
mspiegel opened this issue Jan 4, 2018 · 6 comments
Closed

proposal: encoding/json: export the offset method of the Decoder #23331

mspiegel opened this issue Jan 4, 2018 · 6 comments
Labels
FrozenDueToAge Proposal WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone

Comments

@mspiegel
Copy link

mspiegel commented Jan 4, 2018

I was wondering if there is support for exporting the offset method of the encoding/json Decoder? The offset would be useful in cases where an error is reported during decoding. The json.SyntaxError and json.UnmarshalTypeError types export an Offset field but for other error types it's currently not possible to reconstruct the offset. This is most notable when I'm providing an implementation for the Unmarshaler interface. When decoder.Decode() returns an error generated by an UnmarshalJSON implementation, it's not possible to reconstruct the offset. If the offset method was public then I could use that information to show where the error occurred. There is a slight wrinkle in that the Decoder offset is set to the end of a token when an error occurs. But I don't think it's an issue. If I store the length of the token in the error that is returned from the UnmarshalJSON method that I am implementing then I can subtract and find the start of the token. That design doesn't require any changes to the Decoder. Is there any interest in exporting this method?

@bradfitz bradfitz changed the title encoding/json: export the offset method of the Decoder proposal: encoding/json: export the offset method of the Decoder Jan 4, 2018
@gopherbot gopherbot added this to the Proposal milestone Jan 4, 2018
@ianlancetaylor
Copy link
Contributor

Dup of #11858?

@mspiegel
Copy link
Author

mspiegel commented Jan 4, 2018

I think the issues are related but not duplicates. Implementations of the UnmarshalJSON() method can return any type of error. #11858 looks to be specific to when UnmarshalJSON() returns a UnmarshalTypeError instance. I think this proposal could be seen as as encompassing that issue. Since solving this issue would solve that issue, but not necessarily vice versa.

@rsc
Copy link
Contributor

rsc commented Jan 22, 2018

Exporting the method is a change to the Decoder. It constrains all future implementations. It's unclear that the current semantics are useful (you talk about subtracting to get a useful value). It would be nice if we could stop modifying encoding/json at all.

What is the kind of program where this arises? The justification in the top comment is very hypothetical. More specifics would help us understand how worthwhile this is.

@mspiegel
Copy link
Author

Sure I can provide more specifics. checks-out is an approval management bot that integrates with GitHub. It is derived from Brad Rydzewski's lgtm project that is no longer active. The configuration file is stored in the git repository in HJSON format. The HJSON specification is defined as syntactic sugar for JSON. The HJSON go library is also implemented as syntactic sugar around the encoding/json library. This works well in practice, because line numbers are unchanged between the JSON <--> HJSON transformations.

In a few places in the checks-out configuration file, we've defined a tiny DSL to specify the approval management configuration. The DSL is easy to learn and it avoids having the user specify in JSON the nested data structures that are derived from the DSL. We implement the UnmarshalJSON method to convert the DSL string into the corresponding struct. Currently, we can report a helpful error message when the user has a typo in the DSL. But we can't provide where error occurred in the configuration file.

Isn't the offset already indirectly a part of the encoding/json exported API? The Offset is an exported field of the exported SyntaxError and UnmarshalTypeError types. So if the json library internals are to change and remain 1.x compatible, the Offset fields of SyntaxError and UnmarshalTypeError still need to be calculated.

If the Offset method becomes exported, then adding a sentence in the documentation that the offset returned is the end offset of the last token that was processed. It's OK for the caller to be responsible for finding the begin offset of the last token. The proposal is intended to minimize the modifications needed to the json library.

@ianlancetaylor
Copy link
Contributor

I'm sorry, I don't wholly understand the example. You say that you can report a helpful error message in the DSL. I guess there is another case where you want to report an error message, and where you need an offset. What is that case? Which data are you parsing at that point? What kind of errors do you want to report?

Is the HJSON part relevant to the problem?

Can you show a brief example of the input that contains an error, how you want to report an error for it, and a runnable example of what the code looks like?

What if you just put a Reader in front of the input that counts bytes, and lets you know where you are?

Thanks.

@ianlancetaylor ianlancetaylor added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Apr 30, 2018
@gopherbot
Copy link
Contributor

Timed out in state WaitingForInfo. Closing.

(I am just a bot, though. Please speak up if this is a mistake or you have the requested information.)

@golang golang locked and limited conversation to collaborators May 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge Proposal WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Development

No branches or pull requests

4 participants