Allow to get source position of unmarshal errors (enhanced version)

[andig]

Fix #759

Enhanced version of #760. Added capability is that terminal errors, i.e. errors where the parser fails, can also record their source position.

This introduces a second type of errors. We could check if it would be helpful to create a SourceLocator interface for abstracting the ability to retrieve the line number. Didn't think that's worth the additional code since handling the potential slice of UnmarshalErrors requires type checking anyways.

[andig]

I'll look into adding a test for terminal error with line number.

[lovromazgon]

This change seems very useful and even necessary to be able to get more control over errors 👏 I added a suggestion for an addition, which would solve my problem - I need to warn the user if the file contains unknown fields, but continue processing the file if it's the only thing that's wrong.

[lovromazgon]

I suggest adding a way to differentiate what type of error actually happened, because not all errors have the same severity. For instance, an error because the file contains an unknown field may only constitute printing a warning, while not being able to unmarshal a known field would probably be reason enough to stop processing the file altogether.

I added a snippet of one possible solution that could plug into the current code. Another would be to create separate Go types for each error, although that would be a bigger change.

Suggested change

	type UnmarshalError struct {
	Message string
	Line int
	Column int
	}
	type UnmarshalError struct {
	Type UnmarshalErrorType
	Message string
	Line int
	Column int
	}
	type UnmarshalErrorType int
	const (
	UnknownField UnmarshalErrorType = iota
	CannotUnmarshal UnmarshalErrorType
	// ...
	)

[andig]

I would propose to do this in a separate PR as you'll also need to touch the places where these types are generated?

[lovromazgon]

First a disclaimer: I'm not a maintainer of this project, so you don't get the wrong idea, my comments don't have a lot of weight 🙂

The type would only have to be added to the lines where UnmarshalError is created, so all the lines that would need to change are already touched in this PR. That said, a separate PR sounds reasonable to me! I might create it and base it on your PR since I need the functionality in my project.

[lovromazgon]

I had something like this in mind: lovromazgon@456dd73

It changes UnmarshalError to an interface and creates specific types for each error. This allows you to know exactly what error happened and act accordingly.

I won't create a PR until @niemeyer responds to this PR though, to not create more noise and confusion.

[andig]

@niemeyer could we kindly ask for a review?

[WGH-]

Maybe it would make sense to wrap errors from custom UnmarshalYAML roughly like this, too?

diff --git a/decode.go b/decode.go
index b680695..d96a5c4 100644
--- a/decode.go
+++ b/decode.go
@@ -363,7 +363,7 @@ func (d *decoder) callUnmarshaler(n *Node, u Unmarshaler) (good bool) {
                return false
        }
        if err != nil {
-               fail(err)
+               fail(fmt.Errorf("custom unmarshal failed on line %d column %d: %w", n.Line, n.Column, err))
        }
        return true
 }
@@ -385,7 +385,7 @@ func (d *decoder) callObsoleteUnmarshaler(n *Node, u obsoleteUnmarshaler) (good
                return false
        }
        if err != nil {
-               fail(err)
+               fail(fmt.Errorf("custom unmarshal failed on line %d column %d: %w", n.Line, n.Column, err))
        }
        return true
 }

(I say "roughly" because this way errors might be wrapped multiple times, and we're only interested in the innermost one: custom unmarshal failed on line 1 column 1: custom unmarshal failed on line 111124 column 5: custom unmarshal failed on line 111554 column 13: custom unmarshal failed on line 111555 column 15: custom unmarshal failed on line 111560 column 22)

[andig]

Friendly ping @niemeyer- it has been half a year now ;)

[lyda]

I like this idea better than mine in #995. Still, it would be nice if something was picked.

[andig]

ping @niemeyer another year has passed.

[andig]

Note I've forked at https://github.com/andig/yaml/tree/error-position-plus-failures. I also added andig#1 to allow retrieving source position, i.e. line numbers, for non-terminal TypeErrors.

[andig]

@niemeyer it would still be great to get this added