capnproto / go-capnp

Cap'n Proto library and code generator for Go
https://capnproto.org
Other
1.22k stars 110 forks source link

"Remember" errors from setters, errWriter style #249

Open zenhack opened 2 years ago

zenhack commented 2 years ago

Per discussion starting at https://github.com/capnproto/go-capnproto2/issues/64#issuecomment-1133531818, handling errors for setters is easy to forget and doing it correctly is tedious. It might be nice if messages stored an error, in the style of errWriter: https://go.dev/blog/errors-are-values. This would allow users to check for errors from setters once, at the end.

flowchartsman commented 3 months ago

I think a generated <T>Edit type could work well for this, where it has non-returning methods that add to an error accumulator behind the scenes. Alternatively, it could be just a single error as with the errorwriter example, where any methods after the first error are simply ignored.

func (s Person)Edit(fn func(pe *PersonEdit))error{
    pe := &PersonEdit{
        target: s,
        errs: []error, 
    }
    fn(pe)
    if len(pe.errs) >0 {
        return errors.Join(pe.errs...)
    }
    return nil
}

// and later

err := PersonVal.Edit(func(pe *PersonEdit){
    pe.SetFirstName("foo")
    pe.SetLastName("bar")
})

if err != nil{
    // remediate
}
flowchartsman commented 3 months ago

After a bit of fiddling with this, I have a generated builder pattern I'd be interested in feedback on. Given the following schema

struct Msg {
    cost @0 :Resources;
    union {
        person @1 :Person;
        object @2 :Object;
    }
}

struct Person{
    firstname @0 :Text;
    lastname @1 :Text;
}

struct Object{
    name @0 :Text;
    size @1 :UInt64;
}

struct Resources {
    ram @0 :UInt64;
    cpu @1 :UInt64;
}

It generates code that allows for the following usage patterns:

import "path/to/msg" // capnp-generated package

/* ... */

rootMsg, err := msg.NewRootMsg(seg)
if err != nil {
    // handle error
}

// basic imperative with set at the end
person := rootMsg.BuildPerson()
person.Firstname("bob")
person.Lastname("everyman")
if err := rootMsg.Set(person); err != nil { // not actually necessary at current, see below
    // handle error
}

// shiny fluent-style method-chaining
err = rootMsg.Set(rootMsg.BuildCost().Ram(5).Cpu(40))
if err != nil {
    // handle error
}

It works by generating an interface type per-container type and then implementations of this interface for each sub-struct. Currently, this is not necessary, since there's no need for the _setContainingType call, since I couldn't get it to work. More on this below:

// Set added to containing struct type to take
// builder interface values
func (s Msg) Set(builder _MsgBuilder) error {
    return builder._setMsg(s)
}

// interface for that particular struct type
// for any builders that target it. Type and
// interface method munged to try and
// avoid collision 
type _MsgBuilder interface {
    _setMsg(Msg) error
}

// Method added to struct type to
// spawn a new builder
func (s Msg) BuildCost() *_MsgNewCostBuilder {
    b := &_MsgNewCostBuilder{}
    b.v, b.err = s.NewCost()
        // originally:
        // b.v, b.err = s.Cost()
    return b
}

// concrete type for the builder for Resources
type _MsgNewCostBuilder struct {
    v   Resources
    err error
}

// implementation of the interface
func (b *_MsgNewCostBuilder) _setMsg(s Msg) error {
    if b.err != nil {
        return b.err
    }
        // originally:
        // return b.SetCost(b.v)
    return nil
}

// Setter method with abbreviated name and method chaining.
func (b *_MsgNewCostBuilder) Ram(v uint64) *_MsgNewCostBuilder {
    if b.err == nil {
        b.v.SetRam(v)
    }
    return b
}

/* OTHER METHODS AND TYPES */

Originally the idea with the interface was to avoid s.Set<Type> until the end, but I am probably missing something about making this work, since it currently panics if I replace s.NewFoo() with s.Foo() and attempt to call s.SetFoo(value) at the end. Probably my lack of familiarity, or just something I can't easily generate from outside with AST parsing and go/types. Is there a better way to do this, or is there any point in trying?

Would appreciate feedback. I am certainly not attached to abbreviated setters (e.g. Ram vs SetRam). These were an experiment to avoid verbosity, especially in the method-chaining context, but I realize that's not for everyone. (I'm ambivalent on it myself, but I wanted to see how short I could get it!)

Also worth noting that this has the side-effect of restricting builder methods to only field names in the schema instead of any extra methods on the generated type such as SetBit, SetPtr etc. This has made autocomplete in the builder context nicer in my experience, but my schema is simple, so I don't know the full ramifications for more complex schemata yet, and would appreciate anyone weighing in to tell me of any potential pitfalls.

flowchartsman commented 3 months ago

After mulling this over and going over the discussion in #64, which has a lot of overlap, I have a few different ideas, and was curious about the style and feasibility.

Given the above schema, the normal setters:

p, err := rootMsg.NewPerson()
if err != nil {
    // handle error
}
err := p.SetFirstname("a")
if err != nil {
    // handle error
}
err := p.SetFirstname("b")
if err != nil {
    // handle error
}
c, err := rootMsg.NewCost()
if err != nil {
    // handle error
}
c.SetCpu(1)
c.SetRam(2)

Builder With New<Type> under the hood

personB := rootMsg.BuildPerson()
err := personB.Firstname("a").LastName("b")
if err != nil {
    // handle error
}
if err := rootMsg.BuildCost().Cpu(1).Ram(2); err != nil {
    // handle error
}

Builder With Set Method Added To Container

personB := rootMsg.BuildPerson()
personB.Firstname("a")
personB.Lastname("b")
if err := rootMsg.Set(personB); err != nil {
    // handle error
}
if err := rootMsg.Set(rootMsg.BuildCost().Cpu(1).Ram(2); err != nil {
    // handle error
}

BuildT Methods With Closure

err := rootMsg.BuildPerson(func (pb *Person_B){
    pb.SetFirstname("a")
    pb.SetLastname("b")  
})
if err != nil {
    // handle error
}
err := rootMsg.BuildCost(func (cb *Cost_B){
    cb.SetCpu(1)
    cb.SetRam(2)
})
if err != nil {
    // handle error
}

Generated Struct Type With New Setter Method

err := rootMsg.AllocPerson(PersonT{
    FirstName: "a",
    LastName: "b"
))
if err != nil {
    // handle error
}

if err := rootMsg.AllocCost(CostT{
    Cpu: 1,
    Ram: 2,
}); err != nil {
    // handle error
}

This method would essentially auto-generate pogs-style structs that values were copied from. These are just placeholder names.

Non-pointer types with default values set in the schema might have to be converted to pointers with helper methods to generate pointers (ala AWS SDK Go), or a new nullable type could be used, or a schema annotation could mark a field as $zerodefault where similar rules to JSON omitempty replace zero values with their defaults.

I could also see this enabling an ApplyTo method on generated types where they could go the opposite direction, like:

type PersonT struct {
    FirstName string
    LastName string
}
func (pt PersonT) ApplyTo(fn func(PersonT)error) error {
    return fn(pt)
}

// ...
if err := PersonT{FirstName: "a",LastName: "b}.ApplyTo(sometype.SetSomething); err != nil {
    // handle error
}