Fix cases where the fix for issue260 was not enough #261

Serabe · 2017-02-14T15:49:07Z

The previous fix had some corner cases not covered by the useTime fix, since time.Time might only be used in the unmarhsaler plugin.

This PR makes useTime public and adds it to the lines of unmarshaler where it is needed.

Sorry, for having to do this fix.

awalterschulze · 2017-02-14T16:12:36Z

Why is p.typesPkg.Use() not enough in the unmarshaler?

Serabe · 2017-02-14T16:43:55Z

typesPkg is the gogoprotobuf types, I need to signal that I need the time package. This changes are not because of the line after but because of the line before.

awalterschulze · 2017-02-14T17:36:27Z

Ok yes, my bad.
I am guessing creating a timesPkg might also have some issues conflicting with time when its already imported, but it might be worth exploring?

What about all the other plugins?

If timesPkg isn't an option, could we maybe let generator.UseTime() return a string "time"

Serabe · 2017-02-14T17:53:53Z

I can do the second option. I review the rest of the plugins, but I was looking for uses of tim.Time or time.Duration and did not see anything besides the unmarshaler. I'll recheck tomorrow first thing in the morning.

Thank you!

awalterschulze · 2017-02-14T20:08:25Z

No! Thank YOU very much.

Serabe · 2017-02-15T15:39:09Z

My main problem is that GoType is used for several things at once. If I add the useTime() in there, I'll be calling it without knowing for sure that I'll be using it. The only thing I can think of is changing GoType to returning something like:

type SingleType interface {
    Use() string
}

type singleType string

func (s *singleType) Use() string {
...
}

type singleTypeWithSideEffect struct {
    name string
    fn .     func()
}

func (s *singleType) Use() string {
    // Sideeffect
}

The refactor is quite big, but it would allow us to manage this deps in a better way.

awalterschulze · 2017-02-15T19:28:15Z

I am busy making a simpler fix.

Basically only when time is imported I also add the var _ = time.Kitchen statement.
That way imports stay to a minimum except when typedecl=false and stdtime=true is used with nullable=false.

awalterschulze · 2017-02-15T19:46:55Z

Sorry that I stepped in, but I don't like having things broken and it was quicker to fix it myself than to communicate.
I didn't know how I was going to fix it and trying things on the code myself also helped with getting the idea.
I also had to knock my head against the GoType function to see the real problem, like you originally said.
Thanks again for your help.
I really appreciate it.
And again sorry about how I handled this.

Serabe · 2017-02-15T21:39:49Z

I have a fix for this case I'll be submitting tomorrow. But improving testing related to #262, I saw another problem with stdtime. More on it tomorrow, been a long day navigating generator.go

Serabe · 2017-02-16T08:41:30Z

Rebasing...

johanbrandhorst · 2017-02-16T09:28:48Z

👍 for the work here, good job!

Serabe · 2017-02-16T10:44:52Z

Ooook, so here it goes a summary of yesterday navigating protobuf.

Initial case

My first attempt was to track where the import was lacking and making sure it would not happen again. The thing is that, for tracking that imports are done correctly, one cannot keep all the cases in one proto test since you can even have no individual case right but the proto as a whole can pass (more on this, later).

Some of the main problems with generating a message is that the generateMessage is long (~1K lines one method) and that the GoType method is the paradigm of coupling. GoType returns both a string with the typename and the wire type, and sometimes it gets called for the typename, sometimes for the wire type, and sometimes for both. That makes it pretty hard to add imports just based on resolving the typename because maybe the call is just interested on the wire type.

Then I considered applying the refactor I talked about here #261 (comment) but changing typename from string to its own struct/interface is a hell (again, having a really long generateMessage does not help at all) and I even started implementing it but, sadly, it is too much and would required much more work than one could expect from it. It would also mean it is a pain to maintain changes coming from golang/protobuf.

Therefore, I went to a useTime approach just before inserting time.X in the code. Works.
I also added a timePkg struct to all plugins that might use time.

The case of maps with stdtime/stdduration

When I started working on the fix, the first thing I did was splitting stdtype test in several tests. This helped me debugging the issue and showed a different issue with stdtime. The following paragraphs will be copypasted in a new issue.

If we have a field whose type is a map with Timestamp as its value type, and the stdtime is used in that field, then the imports are broken because...

GoMapType gets called with the FieldDescriptorProto of the field itself.
In GoMapType, GoType is called for both the key and value types but...
Protobuf has no notion of types, but of fields, and therefore, bot the key and value ~~types~~ fields are not aware of the options in the map ~~type~~ field.

awalterschulze · 2017-02-16T19:49:40Z

protoc-gen-gogo/generator/generator.go

@@ -2865,12 +2863,14 @@ func (g *Generator) generateMessage(message *Descriptor) {
 				val = "*c"
 			} else if gogoproto.IsStdTime(field) {
 				pkg := g.useTypes()
+				g.UseTime()


Why do we need to UseTime twice?

I think it was a problem with my rebase.

awalterschulze · 2017-02-16T19:50:17Z

protoc-gen-gogo/generator/generator.go

@@ -2879,12 +2879,14 @@ func (g *Generator) generateMessage(message *Descriptor) {
 				val = "c"
 			} else if gogoproto.IsStdDuration(field) {
 				pkg := g.useTypes()
+				g.UseTime()
 				g.P(`if err != nil {`)
 				g.In()
 				g.P(`return true, err`)
 				g.Out()
 				g.P(`}`)
 				g.P(`c := new(time.Duration)`)


What about g.P(c := new(, g.UseTime(), .time.Duration))

awalterschulze · 2017-02-16T19:50:52Z

test/issue261/stdduration/map/nullable/issue261.proto

+import "google/protobuf/duration.proto";
+
+message MapStdTypes {
+  //map<int32,google.protobuf.Duration> duration = 4 [(gogoproto.stdduration) = true];


Should this be commented?

awalterschulze · 2017-02-16T19:55:04Z

There is a lot of tests here, which is great, but it also pointing to how complex is issue and can become.
There might be more cases we didn't think of.
Which scares me from a maintainability viewpoint.
Doesn't the current master still fix all these usecases?

The only downsides are:

it only causes an unnecessary import of time when typedecl=false && (stdtime=true || stdduration=true)
and it only causes a var _ statement when stdtime=true || stdduration=true

awalterschulze · 2017-02-16T19:55:24Z

Great writeup of the issues by the way.

Serabe · 2017-02-16T21:09:07Z

@awalterschulze I understand that it would be a nightmare to maintain but the bugs are still there no matter how. The best way would be to use go imports in the generated files. That would solve a lot of issues (though might open a few other ones). And no, current master does not fix all those use cases. The commented ones have been failing since the very beginning (you can check #266), they were just not tested. There will be many more cases. Yes, but these are the ones I caught.

Maybe a new kind of test that just generates lots and lots of single-field proto tests is needed. I don't know.

The code needs a huge refactor. And I don't mean gogo/protobuf but golang/protobuf. GoType is a nightmare on its own, generateMessage gives me the creeps, not having a notion of type and using fields as types in maps makes me shudder. I don't envy you and I am very thankful for your work. Without gogo/protobuf, src-d/proteus wouldn't have been possible at all. And I'm pretty sure that all those companies in the README are as thankful as me.

Please, feel free to drop this PR. None of my feelings will be hurt for sure. I went on with this so your comment here was a reality.

Proteus is working, which was my main concern. If the generated files have more imports than needed, it is no problem for me.

Thanks for reviewing all my PRs!

awalterschulze

I didn't know we had so many problems.
Wow thanks a lot for bringing this to light.
Now I am unsure whether my or your fix would be best.

So are the commented ones still failing, even with your fixes?

On the one hand I wish I wrote everything from scratch, but on the other I am not in control of the serialization format, so merging in changes makes it easier to stay accurate to the format. Being a fork can be frustrating and great :)

Thanks for the support. I really appreciate it.

awalterschulze · 2017-02-18T14:00:30Z

protoc-gen-gogo/generator/generator.go

@@ -2879,12 +2877,13 @@ func (g *Generator) generateMessage(message *Descriptor) {
 				val = "c"
 			} else if gogoproto.IsStdDuration(field) {
 				pkg := g.useTypes()
+				g.UseTime()


UseTime is already called below, so this one is redundant

awalterschulze · 2017-02-18T14:00:47Z

protoc-gen-gogo/generator/generator.go

@@ -2748,6 +2745,7 @@ func (g *Generator) generateMessage(message *Descriptor) {
 				} else if gogoproto.IsStdDuration(field) {
 					pkg := g.useTypes()
 					if gogoproto.IsNullable(field) {
+						g.UseTime()


Do we need UseTime here?

awalterschulze · 2017-02-18T14:03:03Z

test/issue261/stdduration/map/non-nullable/issue261.proto

+import "google/protobuf/duration.proto";
+
+message MapStdTypes {
+  //map<int32,google.protobuf.Duration> duration = 4 [(gogoproto.stdduration) = true, (gogoproto.nullable) = false];


Should this be commented?

awalterschulze · 2017-02-18T14:03:17Z

test/issue261/stdduration/map/nullable/issue261.proto

+import "google/protobuf/duration.proto";
+
+message MapStdTypes {
+  //map<int32,google.protobuf.Duration> duration = 4 [(gogoproto.stdduration) = true];


awalterschulze · 2017-02-18T14:04:12Z

test/issue261/stdtime/map/non-nullable/issue261.proto

+import "google/protobuf/timestamp.proto";
+
+message MapStdTypes {
+  //map<int32,google.protobuf.Timestamp> timestamp = 2 [(gogoproto.stdtime) = true, (gogoproto.nullable) = false];


Should this be commented?

awalterschulze · 2017-02-18T14:04:23Z

test/issue261/stdtime/map/nullable/issue261.proto

+import "google/protobuf/timestamp.proto";
+
+message MapStdTypes {
+  //map<int32,google.protobuf.Timestamp> timestamp = 2 [(gogoproto.stdtime) = true];


Should this be commented?

Serabe · 2017-02-20T09:55:33Z

Fix what I could fix. The commented tests have never been working, not even before I started contributing.

The problem is that in a map, the keys and values are not represented by Types but by FieldDescriptors. Therefore, when in (g *Generator) GoMapType (g *Generator) GoType is called with the FieldDescriptors for the key type and the value type, the options are not available since they are different fields. Therefore, the GoType method has no access to the stdtime nor stdduration defined in the actual field defined in the proto, not the ones defined to be used as key type or value types in the map.

Serabe · 2017-02-20T13:06:14Z

Rebased

awalterschulze · 2017-02-22T11:27:11Z

Sorry for the delay.
This is quite complex.
You are right, this has always been broken, even before typedecl was added, since it has nothing to do with it.
I'll look into this as soon as I can.
But today isn't good :(

Serabe · 2017-02-22T12:14:16Z

NP

awalterschulze · 2017-02-26T19:07:21Z

This was really hard to find, but this should fix it
f1756eb#diff-e0188cb7643b9790acd32abc395a5659

awalterschulze · 2017-02-26T19:11:10Z

False alarm :(

awalterschulze · 2017-02-26T19:26:32Z

This should be better 83faaee

awalterschulze · 2017-02-26T19:29:06Z

So I think all the bugs that this pull request is pointing out has been fixed.
Or am I still missing something?

In some cases with gogoslick, time is not being imported. If the import is generated for one case but not for the others, having the implementation in the same package would masquerade the bug. Therefore, I've added one package for each of them. This bug is not manifested if tests are generated, therefore these packages are not gentested (but a `go test ./...` would fail given there is an import missing).

Serabe · 2017-02-27T10:52:33Z

Rebased your changes.

awalterschulze · 2017-02-27T11:16:23Z

And I see all the maps are uncommented as well.
So everything works now :)

Serabe · 2017-04-28T11:20:28Z

I am going to create a new extension for making nullable the value types in a map. Can I close this or are you still interested on something from here?

Thank you!

awalterschulze · 2017-04-28T15:00:46Z

Yes I think you can close this.

I am interested in the new usecase with maps? Currently I am holiday for a few weeks.

Serabe · 2017-04-28T15:16:44Z

Some people needs to represent map[Something]*OtherThing, so I was planning to add an option for that in gogo/protobuf.

awalterschulze · 2017-04-28T18:32:49Z

Isn't that already possible for messages or what is Otherthing

…

On Fri, 28 Apr 2017, 17:19 Sergio Arbeo, ***@***.***> wrote: Closed #261 <#261>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#261 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvsLXsOYP3Vp9SZqwyu883ayN0E3p8zks5r0gOcgaJpZM4MAo2Q> .

Serabe · 2017-04-29T12:02:54Z

Sorry, it was a typo map[Something]OtherThing.

awalterschulze · 2017-04-29T16:03:02Z

Doesn't nullable false work?

…

On Sat, 29 Apr 2017, 14:02 Sergio Arbeo, ***@***.***> wrote: Sorry, it was a typo map[Something]OtherThing. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#261 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvsLQsF1oXgb6_voPZVw_VycXJd3escks5r0ybugaJpZM4MAo2Q> .

Serabe · 2017-05-03T10:46:48Z

It does, thank you!

Serabe · 2017-05-10T16:30:17Z

Reopening this since we have found a similar bug again. I'll dive tomorrow or next week to see what happened, if was something that I did not test or a regression.

Thank you!

awalterschulze · 2017-10-07T14:56:03Z

hello?

Serabe · 2019-11-15T00:11:17Z

Closing this as I don't use protobuf anymore. Thank you!

awalterschulze mentioned this pull request Feb 15, 2017

stdtime generation broken, no longer imports time.Time #262

Closed

Serabe mentioned this pull request Feb 15, 2017

Remove unnecessary statement #263

Merged

Serabe force-pushed the fix/corner-case-in-issue260 branch from cb8d1f4 to aa05f33 Compare February 16, 2017 08:37

Serabe force-pushed the fix/corner-case-in-issue260 branch from aa05f33 to afccd71 Compare February 16, 2017 09:16

Serabe mentioned this pull request Feb 16, 2017

Problem with stdtime when the type is inside a map. #264

Closed

awalterschulze reviewed Feb 16, 2017

View reviewed changes

awalterschulze reviewed Feb 18, 2017

View reviewed changes

Serabe force-pushed the fix/corner-case-in-issue260 branch from 6e5606d to 57ba2fa Compare February 20, 2017 09:35

Serabe force-pushed the fix/corner-case-in-issue260 branch from 57ba2fa to 07e2f57 Compare February 20, 2017 13:06

Serabe added 4 commits February 27, 2017 11:41

Fix lack of import in some scenarios with stdtime

6065ff4

Remove

c5cb856

Refactor usages

b200b17

Serabe force-pushed the fix/corner-case-in-issue260 branch from 07e2f57 to b200b17 Compare February 27, 2017 10:51

Serabe closed this Apr 28, 2017

Serabe reopened this May 10, 2017

Serabe closed this Nov 15, 2019

Fix cases where the fix for issue260 was not enough #261

Fix cases where the fix for issue260 was not enough #261

Conversation

Serabe commented Feb 14, 2017

awalterschulze commented Feb 14, 2017

Serabe commented Feb 14, 2017

awalterschulze commented Feb 14, 2017

Serabe commented Feb 14, 2017

awalterschulze commented Feb 14, 2017

Serabe commented Feb 15, 2017

awalterschulze commented Feb 15, 2017

awalterschulze commented Feb 15, 2017

Serabe commented Feb 15, 2017

Serabe commented Feb 16, 2017

johanbrandhorst commented Feb 16, 2017

Serabe commented Feb 16, 2017

Initial case

The case of maps with stdtime/stdduration

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awalterschulze commented Feb 16, 2017

awalterschulze commented Feb 16, 2017

Serabe commented Feb 16, 2017

awalterschulze left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Serabe commented Feb 20, 2017

Serabe commented Feb 20, 2017

awalterschulze commented Feb 22, 2017

Serabe commented Feb 22, 2017

awalterschulze commented Feb 26, 2017

awalterschulze commented Feb 26, 2017

awalterschulze commented Feb 26, 2017

awalterschulze commented Feb 26, 2017

Serabe commented Feb 27, 2017

awalterschulze commented Feb 27, 2017

Serabe commented Apr 28, 2017

awalterschulze commented Apr 28, 2017

Serabe commented Apr 28, 2017

awalterschulze commented Apr 28, 2017 via email

Serabe commented Apr 29, 2017

awalterschulze commented Apr 29, 2017 via email

Serabe commented May 3, 2017

Serabe commented May 10, 2017

awalterschulze commented Oct 7, 2017

Serabe commented Nov 15, 2019