Protobuf: Megacheck report

[meling]

There are at least some of these issues that should be fixed, for example, I think proto/properties.go:218:2 is a bug. Whereas, I think some of the unused issues fields may be used through reflection and thus should not be removed.

jsonpb/jsonpb.go:977:4: empty branch (SA9003)
proto/properties.go:218:2: this value of s is never used (SA4006)
proto/properties.go:329:5: ineffective break statement. Did you mean to break out of the outer loop? (SA4011)
protoc-gen-go/generator/generator.go:1685:5: this value of obj is never used (SA4006)
jsonpb/jsonpb.go:806:60: should use make([]*stpb.Value, len(s)) instead (S1019)
proto/all_test.go:522:12: should use !strings.Contains(err.Error(), "Kind") instead (S1003)
proto/all_test.go:1170:3: should merge variable declaration with assignment on next line (S1021)
proto/all_test.go:1857:5: should use !strings.Contains(err.Error(), "RequiredField.Label") instead (S1003)
proto/all_test.go:1873:5: should use !strings.Contains(err.Error(), "RequiredField.{Unknown}") instead (S1003)
proto/all_test.go:1882:5: should use !strings.Contains(err.Error(), "RequiredField.Label") instead (S1003)
proto/equal.go:149:2: should use 'return <expr>' instead of 'if <expr> { return <bool> }; return <bool>' (S1008)
proto/message_set.go:97:2: should use 'return <expr>' instead of 'if <expr> { return <bool> }; return <bool>' (S1008)
proto/properties.go:386:4: redundant break statement (S1023)
proto/properties.go:434:4: redundant break statement (S1023)
proto/properties.go:510:5: redundant break statement (S1023)
proto/properties.go:520:5: redundant break statement (S1023)
proto/properties.go:539:5: redundant break statement (S1023)
proto/text.go:482:2: 'if err != nil { return err }; return nil' can be simplified to 'return err' (S1013)
proto/text_parser.go:891:2: 'if pe != nil { return pe }; return nil' can be simplified to 'return pe' (S1013)
protoc-gen-go/generator/generator.go:137:22: should use make([]string, n) instead (S1019)
proto/all_test.go:139:6: func fail is unused (U1000)
proto/all_test.go:1389:6: type nillableMessage is unused (U1000)
proto/all_test.go:1398:2: field nm is unused (U1000)
proto/decode_test.go:45:2: var bytesBlackhole is unused (U1000)
proto/lib.go:318:2: field int32s is unused (U1000)
proto/lib.go:319:2: field int64s is unused (U1000)
proto/lib.go:320:2: field float32s is unused (U1000)
proto/lib.go:321:2: field float64s is unused (U1000)
proto/properties.go:61:7: const startSize is unused (U1000)
proto/properties.go:190:2: field def_uint64 is unused (U1000)
proto/properties.go:783:6: func propByIndex is unused (U1000)
proto/text.go:53:2: var gtNewline is unused (U1000)

[tamird]

See the maintainers' attitude toward these fixes in at least: https://github.com/golang/protobuf/pull/316, https://github.com/golang/protobuf/pull/314, and https://github.com/golang/protobuf/pull/313. Welcome to Google's open source projects.

[dsnet]

The internal (to Google) and external (open-source) implementation of Go protobufs have significantly diverged over time, and it was a significant maintenance burden dealing with different implementations at the same time. As of yesterday, I pushed to a new branch dev, which reconciles these differences. From this point on, the github.com/golang/protobuf repo will be considered the source of truth for Go protobufs. This should hopefully make it easier to review and merge changes, without having to check the other implementation if the change conflicts.

Unfortunately, the new changes breaks code that makes buggy assumptions about protobufs. For that reason, we will unfortunately leave these changes in dev and won't merge them to master until approximately 6 months later (per the "breaking change" policy specified in the README). Alternatively, if Go gets dependency management, we'll merge this to master and just conservatively mark it as v2. Thus, from this point on, we will be developing primarily in dev, and cherry picking relevant changes into master.

@meling, Can you trying running megacheck on the dev branch?

@tamird, I believe one of my co-workers has mentioned that the externalization of internal changes was under-progress. I apologize that it took a while. This was a difficult and onerous undertaking, especially when I have other responsibilities to attend to.

[meling]

@dsnet Thanks for explaining the current state, and for posting to dev.

I ran megacheck on dev:

jsonpb/jsonpb.go:977:4: empty branch (SA9003)
proto/properties.go:158:2: this value of s is never used (SA4006)
proto/properties.go:254:5: ineffective break statement. Did you mean to break out of the outer loop? (SA4011)
protoc-gen-go/generator/generator.go:1778:5: this value of obj is never used (SA4006)
jsonpb/jsonpb.go:806:60: should use make([]*stpb.Value, len(s)) instead (S1019)
proto/all_test.go:527:12: should use !strings.Contains(err.Error(), "Kind") instead (S1003)
proto/all_test.go:1214:3: should merge variable declaration with assignment on next line (S1021)
proto/all_test.go:1963:5: should use !strings.Contains(err.Error(), "RequiredField.Label") instead (S1003)
proto/all_test.go:1979:5: should use !strings.Contains(err.Error(), "RequiredField.Label") instead (S1003)
proto/all_test.go:1979:62: should use !strings.Contains(err.Error(), "RequiredField.{Unknown}") instead (S1003)
proto/all_test.go:1988:5: should use !strings.Contains(err.Error(), "RequiredField.Label") instead (S1003)
proto/discard.go:187:6: unnecessary nil check around range (S1031)
proto/equal.go:149:2: should use 'return <expr>' instead of 'if <expr> { return <bool> }; return <bool>' (S1008)
proto/message_set.go:98:2: should use 'return <expr>' instead of 'if <expr> { return <bool> }; return <bool>' (S1008)
proto/table_marshal.go:1179:5: should omit comparison to bool constant, can be simplified to !v (S1002)
proto/table_merge.go:128:32: should omit comparison to bool constant, can be simplified to !*sfp.toBool() (S1002)
proto/table_merge.go:459:29: should omit comparison to bool constant, can be simplified to v (S1002)
proto/text.go:485:2: 'if err != nil { return err }; return nil' can be simplified to 'return err' (S1013)
proto/text_parser.go:880:2: 'if pe != nil { return pe }; return nil' can be simplified to 'return pe' (S1013)
protoc-gen-go/generator/generator.go:139:22: should use make([]string, n) instead (S1019)
proto/all_test.go:144:6: func fail is unused (U1000)
proto/all_test.go:1434:6: type nillableMessage is unused (U1000)
proto/all_test.go:1443:2: field nm is unused (U1000)
proto/decode_test.go:45:2: var bytesBlackhole is unused (U1000)
proto/message_set.go:171:5: field mu is unused (U1000)
proto/size_test.go:63:28: field x is unused (U1000)

[dsnet]

• jsonpb/jsonpb.go:977:4: empty branch (SA9003)
  Keep. Exists to mark a TODO

• proto/properties.go:158:2: this value of s is never used (SA4006)
  Fix. Looks like a bug where it should be s += ','

• proto/properties.go:254:5: ineffective break statement. Did you mean to break out of the outer loop? (SA4011)
  Fix. Looks like the break should be a return instead.

• protoc-gen-go/generator/generator.go:1778:5: this value of obj is never used (SA4006)
  Fix. Looks like we can avoid the assignment to obj in the if statement.

• jsonpb/jsonpb.go:806:60: should use make([]*stpb.Value, len(s)) instead (S1019)
  Fix.

• proto/all_test.go:527:12: should use !strings.Contains(err.Error(), "Kind") instead (S1003)
  Fix.

• proto/all_test.go:1214:3: should merge variable declaration with assignment on next line (S1021)
  Fix. Let's just push the assignment to x inline with every if statement.

• proto/all_test.go:1963:5: should use !strings.Contains(err.Error(), "RequiredField.Label") instead (S1003)
  Fix.

• proto/all_test.go:1979:5: should use !strings.Contains(err.Error(), "RequiredField.Label") instead (S1003)
  Fix.

• proto/all_test.go:1979:62: should use !strings.Contains(err.Error(), "RequiredField.{Unknown}") instead (S1003)
  Fix.

• proto/all_test.go:1988:5: should use !strings.Contains(err.Error(), "RequiredField.Label") instead (S1003)
  Fix.

• proto/discard.go:187:6: unnecessary nil check around range (S1031)
  Fix.

• proto/equal.go:149:2: should use 'return <expr>' instead of 'if <expr> { return <bool> }; return <bool>' (S1008)
  Fix.

• proto/message_set.go:98:2: should use 'return <expr>' instead of 'if <expr> { return <bool> }; return <bool>' (S1008)
  Fix.

• proto/table_marshal.go:1179:5: should omit comparison to bool constant, can be simplified to !v (S1002)
  Keep. The style matches matches the style around it.

• proto/table_merge.go:128:32: should omit comparison to bool constant, can be simplified to !*sfp.toBool() (S1002)
  Keep. The style matches matches the cases below.

• proto/table_merge.go:459:29: should omit comparison to bool constant, can be simplified to v (S1002)
  Fix.

• proto/text.go:485:2: 'if err != nil { return err }; return nil' can be simplified to 'return err' (S1013)
  Keep. The style matches matches the logic above it.

• proto/text_parser.go:880:2: 'if pe != nil { return pe }; return nil' can be simplified to 'return pe' (S1013)
  Fix.

• protoc-gen-go/generator/generator.go:139:22: should use make([]string, n) instead (S1019)
  Fix.

• proto/all_test.go:144:6: func fail is unused (U1000)
  Fix.

• proto/all_test.go:1434:6: type nillableMessage is unused (U1000)
  Keep. Seems important for the test.

• proto/all_test.go:1443:2: field nm is unused (U1000)
  Keep. Seems important for the test.

• proto/decode_test.go:45:2: var bytesBlackhole is unused (U1000)
  Fix.

• proto/message_set.go:171:5: field mu is unused (U1000)
  Keep. Without digging further, this may be used elsewhere via reflection or type assertions.

• proto/size_test.go:63:28: field x is unused (U1000)
  Keep

[dsnet]

Feel free to submit a PR.

[meling]

@tamird Seems you already fixed some of these in other PRs. Do you want to migrate your changes into dev as requested above?

[tamird]

Yes, I'll take this. Thanks for all the insights.

On Nov 9, 2017 17:42, "Hein Meling" [e-mail] wrote:

@tamird https://github.com/tamird Seems you already fixed some of these
in other PRs. Do you want to migrate your changes into dev as requested
above?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/golang/protobuf/issues/454#issuecomment-343316159,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABdsPG3dufoU3vpqGnzmYfZYqj9lSSfCks5s03_egaJpZM4QXWu-
.

[dsnet]

I believe all PRs for this have been merged. Closing.

[jhump]

@dsnet: if I want to be using the latest protobuf stuff and contributing back to it, would you recommend I just use the dev branch instead of master? You mentioned that dev will be merged to master in six months: six months from when exactly (when will this merge take place)?

Also, if I have a change (I have a couple of unreviewed PRs actually), could I prepare PRs for both branches if I wanted to get a change merged before then?

[dsnet]

Please submit all future (or rebase current) PRs to the dev branch. I haven't made the announcement yet, but should be making it shortly. There are some newly added APIs (relative to master) that I want to adjust.

As for the policy regarding master, that has yet to be determined. I think it's safe to say that any new features on dev will not be added to master. However, any surgical bug fixes may be cherry-picked to master. When we merge dev to master, we want to avoid as much headache as possible.