-
Notifications
You must be signed in to change notification settings - Fork 443
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Primitive one-of type always passes nil as its argument for decoder #1608
Comments
For more details, I also found an issue in generated code for one-of fields. First for a better vision, when a non one-of field is getting decoded it passes its current value to the decoder as an inout parameter. Like this: case 11: try { try decoder.decodeSingularFloatField(value: &_storage._singularFloat) }() But in one of fields it happens completely different. It's a sample of code that generated for decoding a one-of field: case 71: try {
var v: Float?
try decoder.decodeSingularFloatField(value: &v)
if let v = v {
if _storage._o != nil {try decoder.handleConflictingOneOf()}
_storage._o = .oneofFloat(v)
}
}() In the above code the inout parameter that is passing to decode method is always nil (v variable is never set before passing to decoder method). My suggestion would be to add a line of code setting the current value to variable if it's possible: case 71: try {
var v: Float?
if case .oneofFloat(let _v)? = _storage._o {v = _v} // add this line
try decoder.decodeSingularFloatField(value: &v)
if let v = v {
if _storage._o != nil {try decoder.handleConflictingOneOf()}
_storage._o = .oneofFloat(v)
}
}() |
We should probably look at some of the upstream binary tests to see if they have some good example of merging tests that we should bring over, would help make sure we catch these types of issues. |
We'll need to give this some more thought, but for primitive fields, the fact that we pass |
I'm not sure we are on the same page, but let me explain a scenario for it: Let's say we have a message (m1) with oneof field and we try to merge it with another message (m2) with the same type. The oneof type has two possible cases |
Ah, that's potentially a different problem then I was thinking of, I was thinking purely about the nested messages within the oneof, and if the same oneof field is set, a merge wouldn't combine the submessage like it would for a non oneof field. Yea, I guess for collection of the field being set, we might need to always provide the value. Anyway, all the more reason for the addition of merging related tests not just specific to the field mask cases. |
I guess reproducing and fixing of this bug would be easier in case #1505 get merged first |
Generally, fixing this seems worthwhile independent of that PR. Just need someone to code up some test cases and the fixes. |
I close this issue as I commented in #1632 my problem fixed without any additional code generation. |
When I implement a decoder for decoding a one-of type, if its associated value of one-of is primitive (string, double, etc) it always pass nil for decoder method that I've implemented as
Decoder
protocol requirement.Please be sure to include:
OS: macOS 14.0
Xcode version: 15.3
Swift version: 5.10
Branch: main
.proto
snippet:The text was updated successfully, but these errors were encountered: