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

matcher: Intended Private Member Field Behavior is Incorrect #39

Open
kb-sp opened this issue Aug 20, 2023 · 4 comments
Open

matcher: Intended Private Member Field Behavior is Incorrect #39

kb-sp opened this issue Aug 20, 2023 · 4 comments
Labels
edge case An error occurred.

Comments

@kb-sp
Copy link

kb-sp commented Aug 20, 2023

I like the concept of copygen, where in theory the generated could approach ~20x faster than a json.Marshal -> protojson.Unmarshal sandwich (and infinitely faster than jinzhu/copier).

However, when using on structs that aren't flat with concrete builtins, things seem to go a little sideways.

Is there something I'm missing on how to handle non-concrete builtin types?

Setup

By default, Created will be ignored because its name/type combo don't match between the structs, which makes sense.

However, with tag .* json (which is required in my case) or adding a type override, copygen performs unexpectedly:

YML

generated:
  setup: ./gen.go
  output: ./generated/generated.go

Go

package copygen

import (
	"time"

	"google.golang.org/protobuf/runtime/protoimpl"
	"google.golang.org/protobuf/types/known/timestamppb"
)

type Copygen interface {
	EntityToPB(E *Entity) *EntityPB
	EntityToPBCreated(E *Entity, Created *timestamppb.Timestamp) *EntityPB
	// tag .* json
	EntityToPBTag(E *Entity) *EntityPB
	// tag .* json
	EntityToPBCreatedTag(E *Entity, Created *timestamppb.Timestamp) *EntityPB
}

type Entity struct {
	Created *time.Time `json:"created,omitempty"`
	Name    string     `json:"name,omitempty"`
}

type EntityPB struct {
	state         protoimpl.MessageState
	sizeCache     protoimpl.SizeCache
	unknownFields protoimpl.UnknownFields

	Created *timestamppb.Timestamp `protobuf:"bytes,2,opt,name=created,proto3,oneof" json:"created,omitempty"`
	Name    string                 `protobuf:"bytes,4,opt,name=name,proto3" json:"name,omitempty"`
}

Generation (gofmted for better readability)

// Code generated by github.com/switchupcb/copygen
// DO NOT EDIT.

package copygen

import (
	"time"

	"google.golang.org/protobuf/runtime/protoimpl"
	"google.golang.org/protobuf/types/known/timestamppb"
)

type EntityPB struct {
	state         protoimpl.MessageState
	sizeCache     protoimpl.SizeCache
	unknownFields protoimpl.UnknownFields

	Created *timestamppb.Timestamp `protobuf:"bytes,2,opt,name=created,proto3,oneof" json:"created,omitempty"`
	Name    string                 `protobuf:"bytes,4,opt,name=name,proto3" json:"name,omitempty"`
}
type Entity struct {
	Created *time.Time `json:"created,omitempty"`
	Name    string     `json:"name,omitempty"`
}

// EntityToPB copies a *Entity to a *EntityPB.
func EntityToPB(tE *EntityPB, fE *Entity) {
	// *EntityPB fields
	tE.Name = fE.Name
}

// EntityToPBCreated copies a *Entity, *timestamppb.Timestamp to a *EntityPB.
func EntityToPBCreated(tE *EntityPB, fE *Entity, fT *timestamppb.Timestamp) {
	// *EntityPB fields
	tE.state = fT.state
	tE.sizeCache = fT.sizeCache
	tE.unknownFields = fT.unknownFields
	tE.Created = fT
	tE.Name = fE.Name
}

// EntityToPBTag copies a *Entity to a *EntityPB.
func EntityToPBTag(tE *EntityPB, fE *Entity) {
	// *EntityPB fields
	tE.Created = tE.Name = fE.Name
}

// EntityToPBCreatedTag copies a *Entity, *timestamppb.Timestamp to a *EntityPB.
func EntityToPBCreatedTag(tE *EntityPB, fE *Entity, fT *timestamppb.Timestamp) {
	// *EntityPB fields
	tE.Created.Seconds = fT.Seconds
	tE.Created.Nanos = fT.Nanos
	tE.Created = tE.Name = fE.Name
}

Errors

	tE.state = fT.state

Copying private member fields isn't going to end well. ;-)

	tE.Created = tE.Name = fE.Name
// ...
	tE.Created = tE.Name = fE.Name

It seems like custom fields confuses the internal type-matching logic.

Operating System: darwin/arm64 or linux/amd64
Copygen Version: latest == v0.4.0

@kb-sp kb-sp added the edge case An error occurred. label Aug 20, 2023
@switchupcb
Copy link
Owner

Problem: Private Member Field Behavior

Copygen copies the private member fields using the automatcher because EntityPB.state has an equivalent name and type to the timestamppb.Timestamp.state field. This condition is also true the subsequent matches of private member fields in EntityToPBCreated.

So Copygen performs as expected in this case.

With that said, this issue proposes the question on whether Copygen should attempt to match these private member fields. The answer is that matching private member fields is applicable in certain cases, but not always applicable. Therefore, Copygen should not match (copy) private member fields from objects that aren't defined in the generated output package.

Solution

Alternatively

@switchupcb
Copy link
Owner

Problem: Tag Behavior

I cannot confirm whether the specified tagging error exists as you are using an outdated version of Copygen; where tag logic was modified in d4722f4.

Please update to the latest version using go install github.com/switchupcb/copygen@main or go install github.com/switchupcb/copygen@update and try again.

@switchupcb switchupcb changed the title Generating sub-structures seems to be delicate and/or unsupported matcher: Intended Private Member Field Behavior is Incorrect Aug 21, 2023
@kb-sp
Copy link
Author

kb-sp commented Aug 21, 2023

Installing copygen at main doesn't seem to change the behavior with the above test case:

an error occurred while formatting the generated code.
/Users/kb/go/src/github.com/StackPackCo/shishito/cp/generated/generated.go:43:22: expected '==', found '=' (and 1 more errors)

@switchupcb
Copy link
Owner

switchupcb commented Aug 28, 2023

I will be able to address these issues in 1 - 3 weeks with the next update of Copygen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
edge case An error occurred.
Projects
None yet
Development

No branches or pull requests

2 participants