Skip to content

Conversation

@WGH-
Copy link
Contributor

@WGH- WGH- commented Aug 19, 2025

This is a breaking change.

Previously an attempt to unmarshal a YAML mapping into a struct implementing encoding.TextUnmarshaler would succeed, but likely do the wrong thing.

Consider this example:

type Foo struct {
    IP netip.Addr `yaml:"ip"`
}

netip.Addr is a struct type implementing TextUnmarshaler, but containing no exported fields. So unmarshaling {"ip": "127.0.0.1"} would use TextUnmarshaler (as expected), but {"ip": {"whatever": 123}} would silently do nothing.

With this commit, it will now result in a type error.

encoding/json/v2 documentation states:

If the value type implements encoding.TextUnmarshaler, then the input
is decoded as a JSON string and the UnmarshalText method is called with
the decoded string value. This fails with a SemanticError if the input
is not a JSON string.

encoding/json documentation is somewhat ambiguous on this, but it also behaves the same way (https://go.dev/play/p/z9okIXkUzvQ).

@ingydotnet
Copy link
Member

This PR seems good to me but I'd like to see more discussion.
It's technically breaking but do people think it will actually break anything?
Is there any way to search for public code that it would possibly break?

@WGH-
Copy link
Contributor Author

WGH- commented Aug 26, 2025

This PR seems good to me but I'd like to see more discussion. It's technically breaking but do people think it will actually break anything? Is there any way to search for public code that it would possibly break?

I've been thinking about this for a bit, but I have no good ideas how to do so.

We could port this patch to v3, and run tests across popular repositories with go mod replaced dependency, but I doubt that YAML unmarshalling is covered by unit tests well.

Going the conservative way, we could add a deprecation warning, and an option to opt in immediately.

@ingydotnet ingydotnet force-pushed the forbid-mapping-to-text-unmarshaler branch from 24e288b to 83b57fd Compare August 29, 2025 20:24
@ingydotnet ingydotnet mentioned this pull request Sep 3, 2025
@SVilgelm
Copy link
Contributor

I would suggest to do same as json/v2 - unmarshal to a string first, then pass it to TextUnmarshaler

@SVilgelm
Copy link
Contributor

@copilot create an issue based on this PR

@WGH-
Copy link
Contributor Author

WGH- commented Nov 1, 2025

I would suggest to do same as json/v2 - unmarshal to a string first, then pass it to TextUnmarshaler

This PR already does the same thing as json/v2.

@WGH-
Copy link
Contributor Author

WGH- commented Nov 1, 2025

The only difference perhaps is that handling of TextUnmarshaler is somewhat less explicit. encoding/json/v2 switches on target type, whereas go-yaml switches on YAML value type first, and only then methods (*decoder).mapping, (*decoder).sequence, etc. check the target type.

...now that I spelled this out, I think odd cases like unmarshaling a YAML sequence into a slice type implementing TextUnmarshaler might be handled differently to encoding/json/v2. Thanks for pointing that out, I'll check it again.

@WGH- WGH- force-pushed the forbid-mapping-to-text-unmarshaler branch from 83b57fd to 33eb195 Compare November 14, 2025 15:16
@WGH- WGH- changed the title Forbid unmarshaling mapping into a TextUnmarshaler Forbid unmarshaling non-scalars into a TextUnmarshaler Nov 14, 2025
@WGH-
Copy link
Contributor Author

WGH- commented Nov 14, 2025

I've updated this by making the check more explicit.

This is a breaking change.

Previously an attempt to unmarshal a YAML mapping into a struct
implementing encoding.TextUnmarshaler would succeed, but likely do the wrong
thing.

Consider this example:

    type Foo struct {
        IP netip.Addr `yaml:"ip"`
    }

netip.Addr is a struct type implementing TextUnmarshaler, but
containing no exported fields. So unmarshaling {"ip": "127.0.0.1"} would
use TextUnmarshaler (as expected), but {"ip": {"whatever": 123}} would
silently do nothing.

With this commit, it will now result in a type error.

This matches the behavior of both encoding/json and encoding/json/v2.

The documentation of the latter states:

> If the value type implements encoding.TextUnmarshaler, then the input
> is decoded as a JSON string and the UnmarshalText method is called with
> the decoded string value. This fails with a SemanticError if the input
> is not a JSON string.

encoding/json documentation is somewhat ambiguous on this, but it also
behaves the same way (https://go.dev/play/p/z9okIXkUzvQ).
@WGH- WGH- force-pushed the forbid-mapping-to-text-unmarshaler branch from 33eb195 to bd5ce19 Compare November 14, 2025 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants