Potential improvement to generated getters for pointer-optionals

[mpbarlow]

Is your feature request related to a problem? Please describe.

I have a proposal that I think would make it easier to take advantage of the useful getter functions this library generates for fields. Consider this simple example, using optional: pointer (or individual pointer: true directives on optional fields):

Schema

type Food {
  name: String!
  # Zero is a valid value if someone really loves celery, so using value optionals isn't appropriate
  calories: Int!
}

type Person {
  name: String!
  # If the Person doesn't have a favourite food, the API returns null for this entire field
  favouriteFood: Food
}

type Query {
  person(name: String): Person
}

Query

query FindPerson($name: String!) {
  person(name: $name) {
    name
    favouriteFood {
      name
    }
  }
}

One of the things genqlient will generate is a getter method for favouriteFood.name:

// GetName returns FindPersonPersonFavouriteFood.Name, and is useful for accessing the field via an interface.
func (v *FindPersonPersonFavouriteFood) GetName() string { return v.Name }

My interpretation of the comment on the getter is that over time, I may find myself with many types having a name field, and might want to start writing functions that can deal with any of them. So, I might want to write something like:

type named interface {
    GetName() string
}

func processName(f named) *string {
    if f == nil { return nil }

    name := f.GetName()

    // Do some processing involving the name

    return &name
}

Of course, I've now shot myself in the foot, because f is a typed nil; the f == nil comparison will return false, and I'll get a nil pointer dereference when I call GetName.

Describe the solution you'd like

I think this could be made smoother if the getters generated for fields of a pointer-optional nested type themselves performed a nil-check and returned pointers (while leaving the actual field in the struct a value, unless it itself was optional). i.e., the generated getter from above would instead be:

// GetName returns FindPersonPersonFavouriteFood.Name, and is useful for accessing the field via an interface.
func (v *FindPersonPersonFavouriteFood) GetName() *string { 
    if v == nil { return nil }
    return &v.Name
}

Now, the call to f.GetName() would return a nil pointer instead of panicking, and I could return that from my function directly. I would probably also want to remove the f == nil check as it never really served a purpose anyway:

func processName(f named) *string {
    name := f.GetName()

    // Do some processing involving the name

    return name
}

As far as I can tell, genqlient always generates separate Go types per-query per-type, so I don't believe this would have any negative effects in cases where the nested type is optional in one parent type and non-optional in another.

This would also support the common response when this behaviour of Go is questioned as a design flaw, which is that the implementer of the methods on an interface should be responsible for handling nil pointers. Or, to say it another way, the getter says it can return a string given any pointer to FindPersonPersonFavouriteFood, but currently if that pointer is nil it cannot.

Of course, there's the fairly big caveat that genqclient hasn't said it implements any interfaces; I have 😁 But I believe such a use case was the intent behind the feature?

Describe alternatives you've considered

I appreciate that there are several reasons the maintainers may not want to implement this, for example:

• It's 100% a breaking change
• The library already supports unmarshalling nulls to a user-provided generic type, which in an ideal world is preferable to the imperfect modelling of missing values using Go nils
• Reflection can be used to check if the concrete value is nil rather than == nil
• I could nil-check the pointer before I pass it to my function

Nevertheless, because it's perfectly valid in Go to call a method on a (typed) nil pointer, I think my suggestion is overall more type-safe if the getters are pointer receivers rather than value receivers.

Additional context

While Go is not my first language, I would be happy to look into implementing this change if there is any interest in it!

[benjaminjkraft]

Thanks for writing this up. To make sure I understand, in summary what you are saying is that the following code breaks:

func processName(f named) {
  if f == nil { return } // checks for untyped nil
  f.GetName() // crashes on typed nil
}

p, err := FindPerson(name)
// (check err, p.Person != nil)
processName(p.Person.FavoriteFood)

because p.Person.FavoriteFood is (*FindPersonPersonFavouriteFood)(nil)? I agree that it's reasonable to want that to work.

Unfortunately I'm not sure if changing the return to a pointer automatically is really reasonable; the field isn't optional by the time we get there; and it's a bit weird (and maybe conflicting, in the case where the type is a fragment used in several places?) to have a pointer dependent on the type of the parent field. For example, if you make person required in your query, now name is a pointer in one place and not the other, so you can no longer use a common interface (We'd also have to put it behind an option for compat, which is a bit awkward for something this specific.)

What we might be able to do is: in the generated getter methods, if both the receiver and return are already pointers (receiver probably always is), generate the nil-checks. Then you could at least ask for a pointer there and get what you want. It's a bit of an awkward foot-gun, but it seems like a safe enough change to make (I should hope no one is depending on the fact that that panics) and at least solves part of the problem. Do you want to try a PR to see how that looks?