Open maros7 opened 6 years ago
awalterschulze
commented
6 years ago As you can see it tries its best. It might work, but it is super ugly.
time.Time has so many private fields and no way that I can think of that deepcopy or compare can be automatically derived.
Any ideas would be appreciated.
awalterschulze
commented
5 years ago I finally have a solution: Extend DeepCopy to take additional optional functions as parameters that can be matched to certain types. For example:
deriveDeepCopy(dst *T, src *T, deepCopyTime)
func deepCopyTime(dst *time.Time, src *time.Time) {
newTime := time.Unix(0, src.UnixNano())
dst = *newTime
}Then DeepCopy can work for any type that has private fields.
This technique can then be used on all other deep functions, like Equal and Compare
lx-world
commented
3 years ago I finally have a solution: Extend DeepCopy to take additional optional functions as parameters that can be matched to certain types. For example:
deriveDeepCopy(dst *T, src *T, deepCopyTime) func deepCopyTime(dst *time.Time, src *time.Time) { newTime := time.Unix(0, src.UnixNano()) dst = *newTime }Then DeepCopy can work for any type that has private fields.
This technique can then be used on all other deep functions, like Equal and Compare
it is error, why?
deriveDeepCopy does not have two arguments
awalterschulze
commented
3 years ago The solution is "designed", by my comment. But unfortunately it is not implemented yet. This is why the issue is still open.
We are looking for someone to implement it. Let me know if you are interested.
josephbuchma
commented
3 years ago I finally have a solution: Extend DeepCopy to take additional optional functions as parameters that can be matched to certain types. For example:
deriveDeepCopy(dst *T, src *T, deepCopyTime) func deepCopyTime(dst *time.Time, src *time.Time) { newTime := time.Unix(0, src.UnixNano()) dst = *newTime }Then DeepCopy can work for any type that has private fields.
This technique can then be used on all other deep functions, like Equal and Compare
I think for better DX, would be nicer to be able to feed functions like deepCopyTime into goderive separately, so you don't end up typing something like
deriveDeepCopy(dst, src, deepCopyThis, deepCopyThat, deepCopySomethingElse, deepCopyEvenMore)when you need more than few of custom copy functions.
awalterschulze
commented
3 years ago I would prefer it there wasn't any state associated with goderive, but maybe this is still possible in some way to avoid this long list of parameters.
We can't have a list of deepcopy types, since they are all different types: []<wat>{deepCopyThis, deepCopyThat, deepCopySomethingElse, deepCopyEvenMore}, but we could have a struct that contains all your deepcopy types as fields and then the user can just always pass in this one value.
But maybe there is a better way, that doesn't introduce state?
josephbuchma
commented
3 years ago We can't have a list of deepcopy types, since they are all different types
It does not have to be a list though, goderive could generate implementation that accepts all those functions as individual parameters. But struct is definitely nicer approach.
I would prefer it there wasn't any state associated with goderive
It already has state associated with it in form of derive<...> function calls in your codebase. I don't see a problem having it recognise noderive<...> funcs with implementation for specific types. For example, when you have this function in your codebase
func noderiveDeepCopyTime(dst, src *time.Time) {
if src.IsZero() { return }
*dst = time.Unix(0, src.UnixNano()).In(src.Location())
}goderive will use it in generated code for copying time.
Possibly, we're not on the same page on "state" definition in this context 🙂 Also, there is likely better alternative for "noderive" prefix 😄
awalterschulze
commented
3 years ago Oh I see, so the state wouldn't be passed in by the user, this would just be internal for the standard library types? So in this case I still wonder how many types we would have to support, but at least it would be more easily extendible/maintainable.
awalterschulze
commented
3 years ago Oh wait, sorry. I see. So we would detect these functions also based on prefix. Hmmm, I haven't thought about that. So then where would these functions live and how would we distinguish between two functions in different user libraries?
josephbuchma
commented
3 years ago AFAIK, goderive does not work across packages, and generated functions are unexported, so there is no need for any distinction between functions in different libraries. "noderive" functions can be defined anywhere in the user code (within current package), will be picked up by goderive just like derive<...> calls.
Then there also can be an option (CLI flag) for importing custom "noderive" (or rather "Noderive") functions from separate packages, so you don't have to copy-paste "noderive" functions for common types like time.Time.
awalterschulze
commented
3 years ago Okay so then these noderive functions would be redefined in every package where we want to use them?
Although it does seem elegant, I am a bit worried that users will find this logic hard to understand.
For example if someone decided to log something inside noderiveDeepCopyTime users would have to read through the generated code to find the call that has the side effect. But technically this is all possible to debug and an edge case. On the other hand a passed in function might be harder to type, but this type of magic would be much easier to spot for the reader of the code.
josephbuchma
commented
3 years ago Okay so then these noderive functions would be redefined in every package where we want to use them?
From my previous comment:
there also can be an option (CLI flag) for importing custom "noderive" (or rather "Noderive") functions from separate packages, so you don't have to copy-paste "noderive" functions for common types like
time.Time.
For example, say there is this package ("github.com/awalterschulze/goderive/time"):
package time
import "time"
func DeepCopyTime(dst, src *time.Time) {
if src.IsZero() { return }
*dst = time.Unix(0, src.UnixNano()).In(src.Location())
}Then goderive --import=github.com/awalterschulze/goderive/time will add that import to generated file and use DeepCopyTime when appropriate.
This way we can have libraries of reusable "noderive" functions for specific types.
For example if someone decided to log something inside noderiveDeepCopyTime
I think it's a matter of documentation ("Code Generation for Functional Programming" should already ring some bells though 🙂). We can also put some notice into the comment in generated code like "generated code may be using functions defined by user, see more at https://...", so people unfamiliar with goderive (or this feature of goderive) don't think someone just edited generated file.
awalterschulze
commented
3 years ago Sorry for the long wait, been pretty busy.
I would prefer to not have the import as part of a command line flag, I would prefer to import it in the code as this is more explicit to the reader. This is also why I prefer the previously discussed passed in function as a parameter or the struct if we foresee to have a lot of parameters, it is all very explicit to the reader of the code.
I also really like your solution, but though and I had to give it quite a bit of thought, but I think what doesn't sit well is the sacrifice of the explicitness to the reader, that I just couldn't get passed in the end, really sorry.
josephbuchma
commented
3 years ago I see your point, and looking forward to optimal solution as well. I like when things are explicit. The problem with your original approach is that we're talking about very simple things - nobody will put any custom logic into DeepCopyTime or EqualWhatever. It's a functions that have very specific and clear purpose, and you don't need to read the source code or even docs to understand what they do. There is no point in multiple versions of DeepCopyTime, for example. For that reason, explicitly passing such functions as extra arguments does not add much value, and actually looks a bit weird / confusing. It's not terrible though, I'm just trying to find a better way, if there is one.
Not having to pass imports as part of command line flag would be great. What about:
goderive recognize //go:generate goderive ... and use flags from the command in the comment? 🥉 _ "github.com/foo/bar" that will somehow "register" those functions? 🥈
package example
import "github.com/foo/bar/util"func init() { deriveCustom( util.DeepCopyFoo, util.DeepCopyBar, ) }
Then goderive will generate (in `derived.gen.go`):
```go
package example
import "github.com/foo/bar/util"
var (
customDeepCopyFoo func(dst, src *util.Foo)
customDeepCopyBar func(dst, src *util.Bar)
)
func deriveCustom(a func(dst, src *util.Foo), b func(dst, src *util.Bar)) {
customDeepCopyFoo = a
customDeepCopyBar = b
}
// and then use those functions in the rest of generated code...If any function is unused, goderive command will exit with error.
awalterschulze
commented
2 years ago Sorry for the delay, I have been a bit under the weather, will get back to you, when I am feeling a bit better.
awalterschulze
commented
2 years ago Making goderive recognize //go:generate goderive ... and use flags from the command in the comment? 🥉
We have tests for gogenerate, I think it is already working https://github.com/awalterschulze/goderive/blob/master/example/gogenerate/example.go#L1
But it will still require a command line argument.
Using side-effect imports like _ "github.com/foo/bar" that will somehow "register" those functions? 🥈
Sorry, but I really dislike register functions, have been bitten by them many times and has also made it really hard to explain and debug.
Something similar to "noderive" functions approach, like 🥇
See below for a spin on this idea
I was thinking quite a bit about what you trying to accomplish.
Currently the write can create custom Equal functions for the types it has access to, by simply implementing an Equal method and goderive will pick this up and prefer to call the Equal method over generating it's own function. This way the private fields for this accessible type could still be compared. It doesn't require any explicit parameters to be passed in and makes it possible to have only one version of the Equal method per type. The downside it is a bit magical, but the upside is that it sticks to the convention Go readers are used to. The question is how can we accomplish the same, but for types that we don't have access to.
Okay, so this is pretty close to your number 1 idea and with some flavor of number 3 and what I stated in the above paragraph. Instead of needing to provide custom functions for each "deep" code generation (Equal, DeepCopy, Compare), how about we provide custom types.
type DeriveTime time.Time
func (this DeriveTime) Equal(that DeriveTime) bool {
return time.Time(this).UnixNano() == time.Time(that).UnixNano()
}
func (this *DeriveTime) DeepCopy(that *DeriveTime) {
}We can then provide a default library where at least time.Time is supported and more types can be contributed, but we can also provide a command line flag where users can point to their own package(s) that implement such types.
Goderive would then when it encounters a type, check that it doesn't already support the custom type before attempting to generate the code. If a custom type is supported, the generated code could cast the type and call the method.
What do you think?
Sorry for the late reply, being sick and catching up on things is quite a lot of work.
josephbuchma
commented
2 years ago LGTM!
I think it should also pick up type deriveFoo Foo in the user code by default, i.e. if type is not exported and starts with derive, so there is no need to create separate package and add CLI flag for one-off cases.
awalterschulze
commented
2 years ago Sounds good
Could you then also write:
type deriveTime foo.DeriveTimeto avoid a command line flag?
josephbuchma
commented
2 years ago Yep 👍
OladapoAjala
commented
2 years ago @awalterschulze kindly assign this to me.
OladapoAjala
commented
2 years ago Haven gone through the conversation, I think I have an idea of what the problem is but I don't totally grasp the proposed solution (partly because I'm not an adept user of goderive).
Problem: Absence of support for time.Time types in the deepCopy plugin When generating deepCopy for time.Time types goderive has to do so many checks first comment which may or may-not work.
Proposed Solutions:
Pass custom functions as parameters that uses the time package to instantiate a new time variable using "src" and then assigns that variable to "dst". i. I don't see where the multiple custom parameters come in, .i.e you are only copying on time variable into another why do you need more than one custom function to achieve that.
Pass the custom functions as a command line argument instead of parameters to the derive functions i. these functions would have the 'noderive" prefixes, we'd also provide users with in-built functions so they don't always have to implement functions to handle common types ii. @awalterschulze issue with this implementation is having the users write logic that is then passed into the generated code (hence the logging example)
@josephbuchma went on to propose three other solutions which weren't accepted but lead to the agreed solution.
The Agreed Solution: Pass types to goderive as opposed to functions. @awalterschulze could you please explain this further?
Finally:
But maybe there is a better way, that doesn't introduce state? What do you mean by doesn't introduce state?
OladapoAjala
commented
2 years ago @awalterschulze waiting.....
awalterschulze
commented
2 years ago 1i. I don't see where the multiple custom parameters come in, .i.e you are only copying on time variable into another why do you need more than one custom function to achieve that.
There are other types, not just time.Time for which this solution would also need to work, given a struct with time.Time and some other std lib type as fields, it could happen that you need to pass in multiple custom functions.
What do you mean by doesn't introduce state?
Passing everything to a function that it uses, helps to keep the function pure (it contains all its own state), instead of referring to some register pattern, where there is other state outside of the function (which functions are registered, etc). I want to try and keep things as pure as possible. And preferably at least avoid register patterns.
Pass types to goderive as opposed to functions. @awalterschulze could you please explain this further?
Yeah that is fair, I see this wasn't covered very well.
We allow the user to write their own custom overrides for methods, for example here DeriveTime "overrides" time.Time methods, which technically don't exist, but which goderive would check for.
type DeriveTime time.Time
func (this DeriveTime) Equal(that DeriveTime) bool {
return time.Time(this).UnixNano() == time.Time(that).UnixNano()
}
func (this *DeriveTime) DeepCopy(that *DeriveTime) {
}Now the user passes in a command line argument to goderive pointing to the package, where time.Time is provided with these extra methods. Now goderive checks this package and sees that it should prefer to call these methods, instead of looking for the method on the type or attempting to generating it, itself. The generated code should then also import this package.
We can then also provide a standard library of these extra methods, once the command line parameter version is working.
I hope this helps sorry for late reply, I have been very busy. To set expectations, I will be busy for the next 6 weeks, because of personal reasons. So delays should be expected and shorter questions, will probably result in faster answers. Sorry to not be of more and better help, hopefully this will be better after the next 6 weeks.
OladapoAjala
commented
2 years ago @awalterschulze thanks for the detailed explanation. I'd keep my questions shorter next time, thanks.
OladapoAjala
commented
2 years ago @awalterschulze I've also been a bit busy myself. Just had the time to read through your comment. Seems elegant, so we allow the user specify how each field (dst & src) should be compared and copied.
In addition, we can provide a list of std libraries containing our own extra methods so that in case a user doesn't specify a custom method, goderive konws to use one of the std libraries we provie.
OladapoAjala
commented
2 years ago @awalterschulze could you point me to the right section of the codebase where "deepcopy" code generation takes place. I'm trying to understand how goderive walks through the users code to find unimplemented method calls with the appropriate "prefixes" and then generates the desired code.
awalterschulze
commented
2 years ago so we allow the user specify how each field (dst & src) should be compared and copied.
Yes each field type, can be overridden with a new way to copy.
In addition, we can provide a list of std libraries containing our own extra methods so that in case a user doesn't specify a custom method, goderive konws to use one of the std libraries we provie.
Yes :)
could you point me to the right section of the codebase where "deepcopy" code generation takes place.
Yes here is the deepcopy codegen plugin https://github.com/awalterschulze/goderive/blob/master/plugin/deepcopy/deepcopy.go
I'm trying to understand how goderive walks through the users code to find unimplemented method calls with the appropriate "prefixes" and then generates the desired code.
The hasDeepCopyMethod function https://github.com/awalterschulze/goderive/blob/master/plugin/deepcopy/deepcopy.go#L288
is used to check whether a type already has a DeepCopy method, so that one is not generated for it, if a method than can be called already exists.
Now the deepcopy plugin would also need to know about other types it needs to check for methods, types that the current type can be cast to, which is one of the big parts that is not done.
OladapoAjala
commented
2 years ago I'm back 😎😎, fell ill a couple of times since my last comment and had to catch up with work after regaining my health.
I'd be starting off where I stopped.
Hope to still get enough support from @awalterschulze
OladapoAjala
commented
2 years ago There's a huge learning curve for me here, it's my first time working on a code generator. I've spent the last few days reading the comments and the goderive itself.
I have some understanding of how DeepCopy works but I'm not quite sure where to start implementing this feature.
Any resource to help bring me up to speed would be appreciated.
awalterschulze
commented
2 years ago I am sorry, I never had time to write documentation about how to even implement a plugin, which is my bad. I should have done that when I was developing goderive, because now I definitely don't have time :(
Currently the only way is to look at the plugins that have already been implemented and hope to find something similar to what you want to do. I am sorry this is not good, but it is all we have at the moment.
Some advice though:
OladapoAjala
commented
2 years ago Thanks @awalterschulze, writing the tests seems a bit tricky given that I'd be checking the generated code as opposed to testing say a built in type of golang.
I'd checkout the tests you wrote again to understand it better and do a TDD, hopefully that helps better. Thanks.
OladapoAjala
commented
2 years ago I think I'd pause on this, it's a bit difficult to put in the time whilst working. @awalterschulze maybe I should start with a simpler task that wouldn't require me studying much (just to get myself started) before coming back to this.
awalterschulze
commented
2 years ago Yeah that is fair, I think as we have progressed on a solution, this issue has become mislabeled. It isn't an easy issue or a good one to start with. This is my bad for forgetting to update the labels. I can see now a lot of my issues are labeled. I am sorry.
What you can try is create a new Recursive use case, since this won't be replaced with generics. For example: Marshal (protobuf), Unmarshal (protobuf), VerboseEqual (it returns an error or diff), CopyTo (that takes two different types with some similar field names and types and returns an error at generation time, if it couldn't copy over all fields from src to dst), maybe you have a better idea?
Like the title reads. When generating a struct containing a time.Time field I get this use of un-exported, e.g.
time.zone: