matcher: Intended Private Member Field Behavior is Incorrect

[kb-sp]

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

[switchupcb]

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

• Update the matcher at https://github.com/switchupcb/copygen/blob/main/cli/matcher/matcher.go#L54C1-L55C1 to implement the intended private member field logic using field.Package and the generated outputPkgPath (similar to https://github.com/switchupcb/copygen/blob/main/cli/parser/parse.go#L75).
• Add a test to multi.

Alternatively

• Add a field option that indicates whether the field option should be used in the matcher.
• Set this field option when the field.Package is being set at https://github.com/switchupcb/copygen/blob/main/cli/parser/parse.go#L190.
• Check for this field option in the Match{} function.
• Add a test to multi.

[switchupcb]

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.

[kb-sp]

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]

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

[HackenPai]

when EntityPB and Entity have the same field 'State' ,and ignore EntityPB.state, copygen will fail.
panic: runtime error: slice bounds out of range [1:0]
error line:

copygen/cli/matcher/matcher.go

Line 59 in 5c067b0

toField.FullDefinition() == fromField.FullDefinition()[1:]) ||

[switchupcb]

@HackenPai What does your setup.yml and setup.go file look like?

The definition of a field represents the definition (e.g., protoimpl.MessageState in EntityPB).

// Definition represents the type definition of the field (i.e int in ID int, Logger in log.Logger).
— https://github.com/switchupcb/copygen/blob/main/cli/models/field.go#L27

The referenced check indicates fromField.FullDefinition() is len() == 0.

— https://github.com/switchupcb/copygen/blob/main/cli/models/field.go#L180

So, you must show how you are "ignoring" EntityPB.state (when it is being referenced in the automatcher with an empty definition).

[switchupcb]

Here is another note regarding this issue.

How urgent is solving this issue for you or @kb-sp companies?

You can sponsor SwitchUpCB now with a one-time sponsorship to create a pull request which solves this issue for you as a priority.

This issue is not a priority for the maintainer at the current time.

[HackenPai]

@switchupcb
The definitions like this:

type Copygen interface {
	// ignore .*\.[a-z]+.*
	EntityToPB(E *Entity) *EntityPB
}

type Entity struct {
	Name  string `json:"name,omitempty"`
	State string `json:"state,omitempty"`
}

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

	Name  string `protobuf:"bytes,1,opt,name=name,proto3" json:"name,omitempty"`
	State string `protobuf:"bytes,2,opt,name=state,proto3" json:"state,omitempty"`
}

[kb-sp]

Here is another note regarding this issue.

How urgent is solving this issue for you or @kb-sp companies?

You can sponsor SwitchUpCB now with a one-time sponsorship to create a pull request which solves this issue for you as a priority.

This issue is not a priority for the maintainer at the current time.

We're currently using goverter for PB marshaling, so there's no urgency on my end (this issue was posted back in August, 2023). I'd imagine that pb marshaling and field name overlaps (state vs State) aren't terribly uncommon, though.

[switchupcb]

// ignore ..[a-z]+.
— @HackenPai

There is no ignore option. Nothing is being ignored.

field name overlaps
— @kb-sp

A definition is not a field name.

Here is a reminder.

You can implement the steps in #39 (comment) or sponsor SwitchUpCB now to fix this issue when this is a priority for you.

Use goverter when you can't do either of these options.

[HackenPai]

I remove the ignore option, but copygen runs fail. and it throws the same error. So the Public Member Field cannot have the same name as Intended Private Member Field ?

[switchupcb]

So the Public Member Field cannot have the same name as Intended Private Member Field ?
@HackenPai

You should be able to use Public Member Fields with the same name as a Private Member Field when the correct behavior is implemented.

The correct behavior in #39 (comment) is not implemented yet. So, you may experience issues with private fields.