Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make generated code better match Go conventions, and improve usage ergonomics #83

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

thomaspurchas
Copy link

@thomaspurchas thomaspurchas commented Jun 30, 2024

The current generated go code has a couple of quirks in it what makes it more difficult to work with than it needs to be. Two primary culprits are:

All fields with structs are always pointers to structs, rather than concrete structs

When providing a API interface to a dataset (such as a Restful API), it's normal for optional fields to be pointers, and non-optional fields to concrete types, including structs. This approach minimises the number of nil checks people have to do, and generally Go discourages the usage of pointers unless they're actually needed because you want to share a value.

Making every struct value in the generated code is very unergonomic, as it either requires nil checks littered at every level of your code that interacts with these structs, or (more likely) people don't bother with the nil checks, relying on Pkl to enforce the existence of values. However that approach means there's no indication as to what might break if a non-optional field is later make optional. By following go conventions, this change will result in build failures, rather the current runtime failures.

Structs sometimes have an Impl postfix and sometimes don't

Currently the generated code will create an Interface for every open or abstract Pkl class, and name the associated struct SomthingImpl. There's nothing inherently wrong with this approach, but it does make it unnecessarily painful to change a Pkl class from closed to open, or visa-versa.

If a class is made open then the existing Go struct will have Impl appended to its name, and its existing name will become an interface, breaking all existing code that previously referenced the original struct. There's no need for this breakage as there is also a Go convention to prefix interfaces with I (although frowned upon, but no more than appending Impl to structs). Using the interface prefix, rather than the struct postfix, means that the names of structs remains static regardless of the modifiers on the Pkl class. Thus when a Pkl class is made open a new ISomething interface is created, but the existing Somthing struct remains untouched, preventing unnecessary breakage.

Example of changes

These changes are probably best viewed by looking at the diffs of the snippet test outputs, but here's an example that demonstrates most the resulting changes to the generated go code.

Original:

type Person interface {
	Being

	GetBike() *Bike

	GetFirstName() *uint16

	GetLastName() map[string]*uint32

	GetThings() map[int]struct{}
}

var _ Person = (*PersonImpl)(nil)

// A Person!
type PersonImpl struct {
	IsAlive bool `pkl:"isAlive"`

	Bike *Bike `pkl:"bike"`

	// The person's first name
	FirstName *uint16 `pkl:"firstName"`

	// The person's last name
	LastName map[string]*uint32 `pkl:"lastName"`

	Things map[int]struct{} `pkl:"things"`
}

New:

type IPerson interface { <----- Interface now has I prefix
	IBeing

	GetBike() Bike

	GetFirstName() *uint16

	GetLastName() map[string]*uint32

	GetThings() map[int]struct{}
}

var _ IPerson = Person{}

// A Person!
type Person struct { <------ Person no longer has Impl postfix
	IsAlive bool `pkl:"isAlive"`
        
	Bike Bike `pkl:"bike"` <------ Bike is no longer a pointer

	// The person's first name
	FirstName *uint16 `pkl:"firstName"`

	// The person's last name
	LastName map[string]*uint32 `pkl:"lastName"`

	Things map[int]struct{} `pkl:"things"`
}

@thomaspurchas thomaspurchas force-pushed the match-go-convensions branch 2 times, most recently from 5cf1012 to a1d01f8 Compare June 30, 2024 20:11
@thomaspurchas thomaspurchas marked this pull request as draft June 30, 2024 20:11
Normal go conventions encourage the returning on structs rather than
pointers or interfaces. It's also normal convension to make optional
values pointers, and non-optional concrete types, including stucts,
rather than pointers to structs.

As it very easy for a someone to choose to take a pointer to a struct in
go and pass that around if they want to avoid copies (although the go
compiler is heavily optimised to support passing complete structs, even
large ones, around), there's little benifit in making embeded structs
pointers. It just requires that downstream code needs to be filled with
annoying nil checkes, and generally makes it harder to write robust,
clean code.

Finally rather than having interfaces and "Impl" structs, this change
swaps those around. Structs don't have any postfix at all, and
interfaces now have an I prefix. The big advantage of this approach is
that it makes struct naming much more stable. Previously making a Pkl
class open or abstract would result in the generated structs changing
their names include "Impl", and the original names becoming interfaces.
In turn breaking any code already using the existing structs. Now those
breakages won't occur, and devs can opt into using the newly generated
interfaces as needed, rather than having to re-write existing code to
support the change.
We've made some big changes in how the codegen works, so we need to
update all the snippet outputs to reflect that.
Codegen changes means that the old test fixtures are out of date, and
can't be used to test the go binding code. Updating these fixtures
breaks the binding code, and identifies the areas that need enhancement.
The codegen changes means we need to support decoding data into concreet
structs, and not just pointers to structs.
@bioball
Copy link
Contributor

bioball commented Jul 3, 2024

Per our conversation: please split this into two PRs. Thanks!

@thomaspurchas thomaspurchas mentioned this pull request Jul 17, 2024
@thomaspurchas
Copy link
Author

@bioball #92 covers the changes to structs and pointers, but remove the naming changes to interfaces etc. I'll probably wait until we've got that merged before creating an interface naming PR because some of the changes in the PR will make it much easier to make the interface naming change later.

@bioball
Copy link
Contributor

bioball commented Jul 17, 2024

Sounds good! Will take a look.

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.

2 participants