encoding/xml: use typed error for "non-pointer passed to Unmarshal"

[mojixcoder]

In the encoding/xml package when a non-pointer is passed to the function, it returns a new error with errors.New function.
The problem is that we cannot handle errors like this:

err := xml.NewDecoder(readCloser).Decode(out)
// How to explicitly handle a non-pointer passed to Unmarshal in here?
if err == ??? {
}

encoding/json returns *json.InvalidUnmarshalError when a non-pointer is passed and it can be handled on the client side because it doesn't return the error with errors.New function.

It happens in here:

go/src/encoding/xml/read.go

Line 150 in db36eca

return errors.New("non-pointer passed to Unmarshal")

[beoran]

How about this?

if err.Error() == "non-pointer passed to Unmarshal" {
// Handle error here.
}

[mojixcoder]

How about this?

if err.Error() == "non-pointer passed to Unmarshal" { // Handle error here. }

It also works but it's not considered the best practice to handle errors by using their string value.
That's one of the reasons that so many packages have pkg.ErrXXX errors for better and easier error handling.
Also for handling errors this way you either need to print the error value or reading the source code and copying the error string that is used in the function to be able to compare it with another error string.

[ianlancetaylor]

Generally it's useful to have a typed error for an error that can be caused by user input, where the program may need to take different action depending on what it sees. It's less clear that that's useful for a case like this where the only possible fix is to change the program. When would it be useful for a program to explicitly detect this case? What would it do when it happens?

[ianlancetaylor]

I would suggest the 500 response for any unrecognized error. Does it help to categorize this particular error?

[mojixcoder]

I would suggest the 500 response for any unrecognized error. Does it help to categorize this particular error?

How many recongnized errors do we have?
At least 3 or 4 in this case, I think it's better to handle a specific error instead of writing 3 or 4 extra checks for an unrecognized error.
I think it will be even better to validate user input manually instead of writing extra checks because of an unrecognized error.

[mojixcoder]

Generally it's useful to have a typed error for an error that can be caused by user input, where the program may need to take different action depending on what it sees. It's less clear that that's useful for a case like this where the only possible fix is to change the program. When would it be useful for a program to explicitly detect this case? What would it do when it happens?

Sorry I accidentally deleted my reply to this comment, so I am writing it again.
For example when someone is writing a web framework and wants to read request body as JSON.
When the programmer passes a non-pointer value the framework should panic or send a 500 response because the error comes from the developer.
But when a client calls our service and sends an invalid XML payload it's better not to panic or send a 400 error instead of 500.

[seankhliao]

Indeed, and there's a xml.SyntaxError for that case.
There will be a wide variety of errors (eg from the underlying reader) that can be returned, I think the expectation is to special case the few known ones related to user input and report those appropriately, with a default for everything else. After all, Go functions don't (and most can't) give an exhaustive list of errors they can return.

[mojixcoder]

Indeed, and there's a xml.SyntaxError for that case. There will be a wide variety of errors (eg from the underlying reader) that can be returned, I think the expectation is to special case the few known ones related to user input and report those appropriately, with a default for everything else. After all, Go functions don't (and most can't) give an exhaustive list of errors they can return.

I know there is xml.SyntaxError.
Actually now I'm handling it like err.Error() == "my string".
I just faced a situation in which I thought it would be better if I could handle it like this as usually most people do that and it's considered a best practice.
It's definitly not critical or something that can't be handled in other ways at all, I just wanted to introduce a better way.

[jacobishao]

@mojixcoder, I am a bit curious about why you need handle this error. could you please give some explanation?In my option, say that if you passed a non-pointer variable to the function, when you fix it, the error will never appear again.

[mojixcoder]

@mojixcoder, I am a bit curious about why you need handle this error. could you please give some explanation?In my option, say that if you passed a non-pointer variable to the funciton, when you fix it, the error will never appear again.

I've already said that:
#57557 (comment)

[jacobishao]

@mojixcoder Sorry. how about just returning the error to web framework user? just like Gin framework
https://github.com/gin-gonic/examples/blob/master/basic/main.go#L61

[mojixcoder]

@jacobishao I'm returing error at the end but wanted to be able to return different errors.
Actually I'm handling it with err.Error() now.