return typed nil so you can use nil safe methods of the fields

[alovak]

Background

Before v0.24.0, GetField always returned a field instance, even if the field hadn’t been set on the message. Since v0.24.0 (see: https://github.com/moov-io/iso8583/releases/tag/v0.24.0
), callers need to handle a nil return value from GetField.

What this PR changes

With this PR, methods on an unset (nil) field can be called safely and return sensible defaults instead of panicking.

Example:

f := msg.GetField(2)
if f == nil {
    fmt.Println("f is nil")
}

str, err := f.String()
// err => nil
// str => ""

This helps in scenarios where you receive a message and need to read a field that may or may not be present, without forcing nil checks before every method call.

While this change may prevent some old integrations from panicking, it looks dangerous as not all field types are nil-safe. So, calling String, Bytes, Value is safe for most of the fields, but not for Composite. Other methods are not nil-safe.

As a migration path it's recomended to use msg.GetString(n), msg.GetBytes(n) which will not panic if field is not set.

Discussion in open, but I'm leaning towards not making this change.

[gemini-code-assist]

Summary of Changes

Hello @alovak, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request enhances the robustness and usability of the GetField method within the iso8583 library. By changing the return behavior for unset fields from a generic nil to a typed nil (a zero value of the field's type), it eliminates potential panics when developers attempt to call methods on fields that are not present in a message. This allows for more resilient code that can safely interact with message fields, receiving default zero values and no errors, even if the field is absent.

Highlights

• Typed Nil Return for Unset Fields: The GetField method now returns a typed nil (a zero value of the field's type) instead of a plain nil when a requested field is not set in the message. This prevents panics when calling methods on an unset field, allowing for safer access patterns.
• Reflective Type Handling: The implementation uses Go's reflect package to dynamically determine the type of the unset field from the message specification and create a corresponding zero value. This ensures that the returned nil conforms to the field.Field interface, enabling method calls.
• New Test Case for GetField Behavior: A new test, TestMessageGetField, has been added to message_test.go. This test specifically verifies that calling GetField on an unset field returns a nil that can safely have its String() method invoked, which should return an empty string and no error.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request is a great improvement to the API's usability, aiming to prevent panics when methods are called on fields that are not set. By returning a typed nil instead of a bare nil, you allow for nil-safe method calls. However, the current implementation has a critical flaw: it can cause a panic when dealing with Composite fields, as their methods are not nil-safe. I've added comments with details on how to address this and improve test coverage to prevent similar issues in the future.

[gemini-code-assist]

This change successfully returns a typed nil for unset fields, which is a good approach. However, it introduces a potential panic for Composite fields. When GetField is called for an unset Composite field, it returns a (*Composite)(nil). Methods on Composite, such as String() or Bytes(), do not currently handle a nil receiver and will cause a panic (e.g., f.mu.Lock() on a nil pointer).

To fully achieve the goal of this PR, all field.Field implementations should have nil-safe methods. Please add nil checks to the methods of field.Composite to prevent this panic.

[alovak]

I agree with this and maybe we shouldn't introduce typed nil at all.

[adamdecaf]

/gemini what are your thoughts on returning typed nil for the entire iso8583 package methods/types? We'd like to reduce the possibility of panics, but typed nils / zero values leaking out might be unexpected for some callers.

[gemini-code-assist]

That's an excellent question, adamdecaf, and it touches on a fundamental design decision in Go APIs, especially when dealing with interfaces and potential nil values.

My initial comment highlighted a critical point: while the PR aims to return a typed nil (specifically, a zero value of the concrete type wrapped in the field.Field interface) to enable nil-safe method calls, this only works if all methods on all concrete field.Field implementations are truly nil-safe. As demonstrated with field.Composite, this is not currently the case, leading to panics.

Here are my thoughts on the trade-offs:

Arguments for returning a typed nil / zero value (as in the current PR):

• Convenience for callers: If all methods were nil-safe, callers could avoid explicit if f == nil checks, leading to slightly cleaner code.
• Consistency: The idea is that f.String() would always return a valid string (e.g., "") even if the field isn't set.

Arguments against returning a typed nil / zero value (and for returning an untyped nil):

• Clarity of intent: An untyped nil (nil interface value) unequivocally means "the field is not present." A non-nil interface value holding a typed nil concrete value can be ambiguous. Callers might assume a non-nil interface means a present field.
• Implementation complexity: Ensuring every method on every field.Field implementation (and any future ones) is nil-safe is a significant and error-prone task. Missing even one can reintroduce panics, defeating the purpose.
• Unexpected behavior: As you mentioned, "typed nils / zero values leaking out might be unexpected for some callers." If f.String() returns "" for an unset field, it blurs the line between an empty field and a non-existent field.
• Performance: While likely minor, using reflect.Zero has some overhead compared to simply returning nil.

Given the critical nature of financial messaging and the need for clear, predictable behavior, I lean towards alovak's sentiment that returning an untyped nil when a field is not set is the safer and clearer approach for GetField.

For scenarios where callers want nil-safe access without explicit checks, the existing pattern of providing helper methods on the Message struct, such as msg.GetString(n) or msg.GetBytes(n), is generally preferred. These methods can internally handle the nil check from GetField and return appropriate zero values (e.g., "", nil slice) or errors, abstracting away the complexity for the caller.

In summary, while the goal of reducing panics is laudable, the current approach in the PR introduces significant implementation burden and potential for subtle bugs if not every single method across all field types is made nil-safe. Returning an untyped nil from GetField and providing higher-level, nil-safe accessors on the Message struct offers a more robust and maintainable solution.

[adamdecaf]

and potential for subtle bugs if not every single method across all field types is made nil-safe.

Yea. That's the main thing to consider IMO. It's easier for generated code to be nil-safe (look at protobufs), but harder for hand written library code. Plus we do have nil-safe methods to get values, which is used more often.