Merge remote-tracking branch 'origin/dev' into master #591
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Major changes: * New table-driven optimization for marshal, unmarshal, and merge * Unknown field preservation for Proto3 * Generated source-file annotation for Kythe * Various bug fixes
Found with honnef.co/go/tools/cmd/unused.
* Regenerate * Correct some mistakes Most notable is the bug in (*proto.Properties).Parse, for which I've added a test. Found with honnef.co/go/tools/cmd/staticcheck. 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:1686:5: this value of obj is never used (SA4006)
Found with honnef.co/go/tools/cmd/gosimple.
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)
Run gofmt across the whole repo. Remove lint-hint that was accidentally submitted in a prior PR.
…d is not set (#472) Change Marshal/Unmarshal to return error if any required field is not set. For Unmarshal, this means JSON is either missing any required field or has required field set to null.
The %v verb in Errorf automatically calls the String method. This is safer than manually calling reflect.Type.String since the reflect.Type may be nil.
There is an internal type to Google called RawMessage that is similar to json.RawMessage. Since there is no proper proto reflection API, we special-cased the Bytes method of RawMessage to access the raw bytes. This is a gross hack since Bytes() []byte is such a common method signature. Remove this hack. Fixes #311
Change the pre-gofmt generated code for oneof discriminant interfaces
from:
type T interface { T() }
to:
type T interface {
T()
}
Prior to Go 1.9, gofmt rewrote the single-line form into the multi-line
one. Go 1.10 and later preserve the single-line form for single-method
interfaces. Generating code that is handled identically by both versions
of gofmt avoids spurious diffs with golden test data.
Change-Id: Ibb229827823a50e963bcd1b34e59a286654b415f
Modify GetExtension to return the raw bytes if the ExtensionDesc is type-incomplete (i.e., the ExtensionType is nil).
The documentation for Timestamp.nanos says: <<< Non-negative fractions of a second at nanosecond resolution. Negative second values with fractions must still have non-negative nanos values that count forward in time. Must be from 0 to 999,999,999 inclusive. >>> The documentation for Duration.nanos says: <<< Signed fractions of a second at nanosecond resolution of the span of time. Durations less than one second are represented with a 0 `seconds` field and a positive or negative `nanos` field. For durations of one second or more, a non-zero value for the `nanos` field must be of the same sign as the `seconds` field. Must be from -999,999,999 to +999,999,999 inclusive. >>> Thus, we forbid values beyond the documented range of valid values.
…its if possible (#490) According to the proto3 to JSON specification, the JSON representation of a timestamp field: >>> Uses RFC 3339, where generated output will always be Z-normalized and uses 0, 3, 6 or 9 fractional digits. <<<
Not all proto.Message implementations will be updated to be using the most recent protoc-gen-go. Thus, they will lack an XXX_DiscardUnknown method. Add logic to handle older protobufs.
The proto2 and proto3 specifications explicitly say that strings must be composed of valid UTF-8 characters. The C++ implementation prints an error anytime strings with invalid UTF-8 is present during parsing or serializing. For proto3, it explicitly errors out when parsing an invalid string. Thus, we cause marshal/unmarshal to error if any string fields are invalid. The error returned is fail-fast and indistinguishable. This means that the we stop the unmarshal process the moment we detect an invalid string, as opposed to finishing the unmarshal. An indistinguishable error means that we provide no API for the user to programmatically check specifically for invalid UTF-8 errors so that they can ignore it. In conversations with the protobuf team, they felt strongly that there should be no ability for users to ignore the UTF-8 validation error. However, if this change is overly problematic for users, we can consider a workaround for users already depending on strings containing invalid UTF-8.
This flag was necessary inside Google for the transition to having proto3 preserve unknown fields by default. Outside of Google, this is unnecessary since the dev branch is the transition.
For now, use fmt.Sprintf to get the proper type name that is agnostic to the package that the type is defined in. However, in the future, we should avoid such brittle testing and have more distinguishable errors.
In Go 1.10, go test automatically runs go vet as well. This uncovered a vet failure in TestUnmarshalRepeatingNonRepeatedExtension where pb.ComplexExtension is passed as a fmt.Stringer to t.Errorf. The string method exists only on the pointer-receiver type, *pb.ComplexExtension.
The proposal in golang/go#23172 was accepted. The "purego" build tag is intended to be a community agreed upon soft-signal to indicate the forbidden use of unsafe, assembly, or cgo. A change in the future will remove special-casing the appengine and js tags once the related toolchains support purego (possibly after some bake-in period).
Tools in the Go ecosystem treat code documented with "Deprecated:" as special for purposes of warning users not to use them. When a file, message, field, enum, enum value, service, or method is marked as deprecated, the generator will emit "// Deprecated: Do not use." comments on all relevant exported Go package information related to the deprecated element. This is an outline of how deprecation of each element effects the generated proto: * file - a comment is added to the top of the generated proto. * message - a comment is added above its type definition. * field, enum, or enum value - a comment is added inline with the defintion. * field - an additional comment is added above the field Getter. * service - a comment is added above generated Client interface and New method. * service - a comment is added above generated Server interface and Registration method. * method - a comment is added above the generated method definition. Fixes #396.
protoc-gen-go: indicate deprecated fields in documentation
Previously, an error was returned during unmarshal when a wiretype was encountered that did not match the expected wiretype. In order to match the behavior of the C++ and Python implementations, we no longer return an error and instead store the bad wire fragment as an unknown field (or skip them if unknown field preservation is disabled). The generator still produces code that references ErrInternalBadWireType for unmarshal logic for oneof fields. However, the current proto package does not use the generated unmarshalers for oneofs, so their existence has no bearing on unmarshal semantics. Cleaning up the generator to stop producing these is future work.
"make test" now compiles all the source files in testdata/ using the protoc-gen-go in the current working directory and compares the output to golden versions, "make regenerate" rebuilds the golden files. Add a go_package option to each proto source file. Put the sources for each package in the proper directory. Add a golden_test.go which arranges to compile each source file using the compiler in the working tree. This does not touch the content of any of the sources in testdata/ other than to add go_package options and fix up import paths. Change-Id: Iea5bef9bba626116b8ce5ea136a15f3cff4f5bcc
Change-Id: I4dcf2ed1c9edf713bdf20cdb844599921e36566c
…nore. Change-Id: I11370c095fcb2638ef5a050e6a843517107da107
Change-Id: I57a5edf2e9e0c4f70efc4d4fa97b79cd97925757
Change-Id: I9172ec10cc25d2e966ccd8d2af94130a546d2792
…562) The generator includes a hack for generating the name of fields which extend proto2.bridge.MessageSet. (The ultimate motivation appears to be compatibility with proto1 text formats.) Apply this hack based on whether the message being extended defines the message_set_wire_format option, not on the local name of that message type.
A prior changed (see #511) allows Unmarshal to treat fields with unknown wire types with known tags as unknown fields. When doing so, we must be careful not to set the required field bit. For example, suppose we have: message Foo { require fixed32 my_field = 1; } Then parsing a message with a field of type varint and tag 1 should not satisfy the required that Foo.my_field be set.
Introducing the GoImportPath and GoPackageName types to distingish between these values changed the exported API of the generator package in a few small ways. While the API of this package comes with no stability guarantee, the benefit to the exported part of the change is small. Revert the exported changes to avoid gratuitous breakage. Specific changes: Generator.ImportPrefix reverts to type string Generator.ImportMap reverts to type map[string]string (There are other externally-visible changes to the package API which are worth making and will not be reverted, most notably the removal of the PackageName method from FileDescriptor.)
An earlier change inadvertendly made the import_path flag stop setting the package name. In addition, we intentionally removed the behavior where a go_package option in one file would set the package name for other generated files, in anticipation of allowing a single compilation action to generate code for many packages. This change restores the import_path behavior. It also permits a go_package option in one file to affect other files *in the same package*. (Every source file should include a go_package option, which makes this case moot, but this minimizes the amount of breaking change that we're introducing.) Also tweak assignment of import paths to allow the import_path flag to set the import path for all generated files.
Skip generating getters for fields that are not defined in the same file
as the publicly imported message.
This is the wrong way to fix this problem. It is, however, the expedient
one to hold us over until (soon, I hope) we completely redo public
imports as type aliases and make all of this moot.
The scenario:
a.proto publicly imports b.proto
b.proto has a message M
M has a field F of type T
We generate forwarding methods for public imports.
// T is defined in the same Go package as a.proto.
func (m *M) GetF() T { ... }
Depending on what package T is defined in, we might need to qualify its
name (foo.T), and we might need to add an import for that package.
That's not what we used to do, however: Instead, we'd *only* generate
the GetF forwarder if T is defined in b.proto.
Commit 9d4962b made it so that we'd generate the forwarder if it T is
defined in the *same Go package* as b.proto, which seems safe and
reasonable. It turns out that something elsewhere in the generator is
getting confused by this, however. Rather than figuring out what--which
would also change a lot of generated code by adding forwarding methods
that didn't used to be there--this just reverts back to the behavior of
looking at files rather than packages.
All of this goes away completely once we start using type aliases,
because we don't need any forwarding methods at all at that point.
Type aliases were added in Go 1.9, so this change bumps the minium required Go version for protos which use public imports.
Reported by honnef.co/go/tools/cmd/unused: proto/text.go:460:6: func writeRaw is unused (U1000)
The 1.9 release tag is "go1.9", not "1.9".
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
1 similar comment
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
|
It is possible that this pull request may break you. Please read the previous announcement here for potential sources of breakages and ways to fix it. If the breakage seems to be a actual bug in the Go protobuf implementation, please file an issue, and we will take a look. |
neild
approved these changes
Apr 30, 2018
This was referenced May 1, 2018
This was referenced May 1, 2018
BACKPORT Update Go sdk to comply with golang/protobuf update
hyperledger-archives/sawtooth-core#1605
Merged
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.