Primitive one-of type always passes nil as its argument for decoder

[pouyayarandi]

Primitive one-of type always passes nil as its argument for decoder

[pouyayarandi]

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)
  }
}()

[thomasvl]

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.

[thomasvl]

We'll need to give this some more thought, but for primitive fields, the fact that we pass nil might not matter, yes it could be a single to the decoder the value was set, but for we also have the handleConflictingOneOf callback for that. The pace we really need to pass it is where merge would be adding instead of replacing, so sub message, maps?, and probably repeated fields. So it might make sense to only do the fetch in those cases to keep the code sizes down.

[pouyayarandi]

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 oneof_int32(int32) and oneof_int64(int64). If we merge m1 with m2 by a field mask of both oneof_int32 and oneof_int64 paths, we expect that m1 oneof field should be same as m2. But currently, since merge decoder can't get value of oneof paths, it becomes nil.

[thomasvl]

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.

[pouyayarandi]

I guess reproducing and fixing of this bug would be easier in case #1505 get merged first

[thomasvl]

Generally, fixing this seems worthwhile independent of that PR. Just need someone to code up some test cases and the fixes.

[pouyayarandi]

I close this issue as I commented in #1632 my problem fixed without any additional code generation.