16

Consider this example. Let's say I have this object which is ubiquitous throughout my codebase:

type Person struct {
    Name string
    Age  int
    [some other fields]
}

Somewhere deep in the codebase, I also have some code that creates a new Person struct. Maybe it's something like the following utility function (note that this is just an example of some function that creates a Person-- the point of my question is not to ask about the copy function specifically):

func copyPerson(origPerson Person) *Person {
    copy := Person{
        Name: origPerson.Name,
        Age:  origPerson.Age,
        [some other fields]
    }
    return &copy
}

Another developer comes along and adds a new field Gender to the Person struct. However, because the copyPerson function is in a distant piece of code they forget to update copyPerson. Since golang doesn't throw any warning or error if you omit a parameter when creating a struct, the code will compile and appear to work fine; the only difference is that the copyPerson method will now fail to copy over the Gender struct, and the result of copyPerson will have Gender replaced with a nil value (e.g. the empty string).

What is the best way to prevent this from happening? Is there a way to ask golang to enforce no missing parameters in a specific struct initialization? Is there a linter that can detect this type of potential error?

Improve this question
4
  • 1
    The best way would be to write a custom linter. Commented Feb 5, 2019 at 18:42
  • A hypothetical problem easily discovered during unit testing so nothing to worry about. Commented Feb 5, 2019 at 19:38
  • @Volker Your comment made me think of Beeblebrox's Peril Sensitive Sunglasses. 😄 Not trying to upset you though. Commented Feb 7, 2022 at 21:53
  • Are you coming from C++? I encountered this a lot with C++ copy-constructors. Luckilly, Go does not have the problem since making a copy of the struct will copy all the (non-blank) fields. Commented Jul 19, 2022 at 3:52

8 Answers 8

Reset to default
7

The way I would solve this is to just use NewPerson(params) and not export the person. This makes it so the only way to get a person instance is to go through your New method.

package person

// Struct is not exported
type person struct {
    Name string
    Age  int
    Gender bool
}

// We are forced to call the constructor to get an instance of person
func New(name string, age int, gender bool) person {
    return person{name, age, gender}
}

This forces everyone to get an instance from the same place. When you add a field, you can add it to the function definition and then you get compile time errors anywhere they are constructing a new instance, so you can easily find them and fix them.

Improve this answer
Sign up to request clarification or add additional context in comments.

3 Comments

Having fields only accessible through getter/setter methods is unidiomatic Go.
@Adrian not really the point of what I'm showing. Using interfaces is completely idiomatic, and if you need access to the fields, you simply return the struct instead of the interface - the exported fields of an unexported struct are still accessible.
Using this approach, it'll become verbose to write functions that accept person struct as argument, the consumers in other package will have to define each fields themselves as interface.
5

First of all, your copyPerson() function does not live up to its name. It copies some fields of a Person, but not (necessarily) all. It should've been named copySomeFieldsOfPerson().

To copy a complete struct value, just assign the struct value. If you have a function receiving a non-pointer Person, that is already a copy, so just return its address:

func copyPerson(p Person) *Person {
    return &p
}

That's all, this will copy all present and future fields of Person.

Now there may be cases where fields are pointers or header-like values (like a slice) which should be "detached" from the original field (more precisely from the pointed object), in which case you do need to make manual adjustments, e.g.

type Person struct {
    Name string
    Age  int
    Data []byte
}

func copyPerson(p Person) *Person {
    p2 := p
    p2.Data = append(p2.Data, p.Data...)
    return &p2
}

Or an alternative solution which does not make another copy of p but still detaches Person.Data:

func copyPerson(p Person) *Person {
    var data []byte
    p.Data = append(data, p.Data...)
    return &p
}

Of course, if someone adds a field which also needs manual handling, this won't help you out.

You could also use unkeyed literal, like this:

func copyPerson(p Person) *Person {
    return &Person{
        p.Name,
        p.Age,
    }
}

This will result in a compile-time error if someone adds a new field to Person, because an unkeyed composite struct literal must list all fields. Again, this will not help you out if someone changes the fields where the new fields are assignable to the old ones (e.g. someone swaps 2 fields next to each other having the same type), also unkeyed literals are discouraged.

Best would be for the package owner to provide a copy constructor, next to the Person type definition. So if someone changes Person, he / she should be responsible keeping CopyPerson() still operational. And as others mentioned, you should already have unit tests which should fail if CopyPerson() does not live up to its name.

The best viable option?

If you can't place the CopyPerson() next to the Person type and have its author maintain it, go ahead with the struct value copying and manual handling of pointer and header-like fields.

And you can create a person2 type which is a "snapshot" of the Person type. Use a blank global variable to receive compile-time alert if the original Person type changes, in which case copyPerson()'s containing source file will refuse to compile, so you'll know it needs adjusting.

This is how it can be done:

type person2 struct {
    Name string
    Age  int
}

var _ = Person(person2{})

The blank variable declaration will not compile if fields of Person and person2 do not match.

A variation of the above compile-time check could be to use typed-nil pointers:

var _ = (*Person)((*person2)(nil))
Improve this answer

Comments

2

Since you specifically asked for a linter to solve the problem: Nowadays there's a linter called exhaustruct which is included in recent versions of golangci-lint.

Improve this answer

Comments

1

I'm not aware of a language rule that enforces that.

But you can write custom checkers for Go vet if you'd like. Here's a recent post talking about that.

That said, I would reconsider the design here. If the Person struct is so important in your code base, centralize its creation and copying so that "distant places" don't just create and move Persons around. Refactor your code so that only a single constructor is used to build Persons (maybe something like person.New returning a person.Person), and then you'll be able to centrally control how its fields are initialized.

Improve this answer

Comments

1

The idiomatic way would be to not do this at all, and instead make the zero value useful. The example of a copy function doesn't really make sense because it's totally unnecessary - you could just say:

copy := new(Person)
*copy = *origPerson

and not need a dedicated function nor have to keep a listing of fields up to date. If you want a constructor for new instances like NewPerson, just write one and use it as a matter of course. Linters are great for some things but nothing beats well-understood best practices and peer code review.

Improve this answer

Comments

0

The best solution I have been able to come up with (and it's not very good) is to define a new struct tempPerson identical to the Person struct and put it nearby to any code which initializes a new Person struct, and to change the code that initializes a Person so that it instead initializes it as a tempPerson but then casts it to a Person. Like this:

type tempPerson struct {
    Name string
    Age  int
    [some other fields]
}

func copyPerson(origPerson Person) *Person {
    tempCopy := tempPerson{
        Name: orig.Name,
        Age:  orig.Age,
        [some other fields]
    }
    copy := (Person)(tempCopy)
    return &copy
}

This way if another field Gender is added to Person but not to tempPerson the code will fail at compile-time. Presumably the developer would then see the error, edit tempPerson to match their change to Person, and in doing so notice the nearby code which uses tempPerson and recognize that they should edit that code to also handle the Gender field as well.

I don't love this solution because it involves copying and pasting the struct definition everywhere that we initialize a Person struct and would like to have this safety. Is there any better way?

Improve this answer

1 Comment

There is a good idea in this, but it's unnecessary to construct a value of tempPerson and to convert and assign it to a Person every time copyPerson() is called. After all, you just need to know if the struct types (their fields) match, which check can be performed at compile time. See my answer how to do that.
0

Approach 1 Add something like copy constructor:

type Person struct {
    Name string
    Age  int
}

func CopyPerson(name string, age int)(*Person, error){
    // check params passed if needed
    return &Person{Name: name, Age: age}, nil
}


p := CopyPerson(p1.Name, p1.age) // force all fields to be passed

Approach 2: (not sure if this is possible)

Can this be covered in tests say using reflection?
If we compare the number of fields initialised(initialise all the field with values different than the default values) in the original struct and the fields in copy returned by the copy function.

Improve this answer

Comments

-1

Here is how i would do it:

func copyPerson(origPerson Person) *Person { 
    newPerson := origPerson

    //proof that 'newPerson' points to a new person object
    newPerson.name = "new name"
    return &newPerson
}

Go Playground

Improve this answer

1 Comment

I don't think he's asking for a better way to copy a person, he's asking how to handle when fields get added to the struct, to ensure that everywhere that creates an instance sets the value and it doesn't just get the default value.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.