Handle unknown fields part of a oneof #324
Conversation
|
Companion CL https://dart-review.googlesource.com/c/sdk/+/125345 has tests. |
| @@ -265,8 +265,9 @@ class _FieldSet { | |||
| } | |||
| } | |||
|
|
|||
| // neither a regular field nor an extension. | |||
| // TODO(skybrian) throw? | |||
| if (_hasUnknownFields) { | |||
There was a problem hiding this comment.
Is it intentional that _clearField(tagNumber) checks unknown field set, but e.g. _hasField(tagNumber) does not?
There was a problem hiding this comment.
Good question!
I think we could change the _hasField semantics also.
Done
There was a problem hiding this comment.
Since we are now changing semantics to include unknowns, I agree that it is inconsistent if we don't also update _hasField. Good catch!
| if (oneofIndex != null) { | ||
| _clearField(_oneofCases[oneofIndex]); | ||
| _oneofCases[oneofIndex] = tag; | ||
| _oneofCases[oneofIndex] = newTagnumber; |
There was a problem hiding this comment.
Maybe only call it conditionally
final currentTag = _oneofCases[oneofIndex];
if (currentTag != null) _clearField(currentTag);There was a problem hiding this comment.
Yes - that's better
| /// will be kept. | ||
| /// | ||
| /// If the tagNumber is present as an unknown field, that field will be | ||
| /// removed. |
There was a problem hiding this comment.
GeneratedMessage.mergeFromMessage() to call _FieldSet._mergeFromMessage(), which looks like this:
void _mergeFromMessage(_FieldSet other) {
for (FieldInfo fi in other._infosSortedByTag) {
var value = other._values[fi.index];
if (value != null) _mergeField(fi, value, isExtension: false);
}
if (other._hasExtensions) {
var others = other._extensions;
for (int tagNumber in others._tagNumbers) {
var extension = others._getInfoOrNull(tagNumber);
var value = others._getFieldOrNull(extension);
_mergeField(extension, value, isExtension: true);
}
}
if (other._hasUnknownFields) {
_ensureUnknownFields().mergeFromUnknownFieldSet(other._unknownFields);
}
}This merging does not seem to account for oneofs. Does it need to be updated as well? Are there more places?
There was a problem hiding this comment.
_FieldSet._mergeFromMessage() calls _mergeField that ends up calling _setNonExtensionFieldUnchecked that handles the oneofs
There was a problem hiding this comment.
Maybe my comment was not clear, what I meant is the following:
We seem to have an invariant now
- there is at most one of the
oneofcases set (either a) it's set in field set b) it's set in unknown field set c) none are set) - whenever we add a new field value, we are guaranteed to clear the old one if one exists.
This invariant is violated because of this line:
if (other._hasUnknownFields) {
_ensureUnknownFields().mergeFromUnknownFieldSet(other._unknownFields);
}We might have already a oneof candidate in the fieldset (or the unknown fieldset). Yet this code blindly copies other._unknownFields, which possibly has a new oneof value, but we do not clear the old one.
There was a problem hiding this comment.
Ah - now I get it - thanks! I updated the merging of unknown fields to go field by field, and also update the one-ofs.
| if (oneofIndex != null) { | ||
| _clearField(_oneofCases[oneofIndex]); | ||
| _oneofCases[oneofIndex] = tag; | ||
| _oneofCases[oneofIndex] = newTagnumber; |
There was a problem hiding this comment.
Yes - that's better
| /// will be kept. | ||
| /// | ||
| /// If the tagNumber is present as an unknown field, that field will be | ||
| /// removed. |
There was a problem hiding this comment.
_FieldSet._mergeFromMessage() calls _mergeField that ends up calling _setNonExtensionFieldUnchecked that handles the oneofs
| @@ -265,8 +265,9 @@ class _FieldSet { | |||
| } | |||
| } | |||
|
|
|||
| // neither a regular field nor an extension. | |||
| // TODO(skybrian) throw? | |||
| if (_hasUnknownFields) { | |||
There was a problem hiding this comment.
Good question!
I think we could change the _hasField semantics also.
Done
|
I have also improved the _clearField methods handling of oneofs. It was doing the work twice, and only for known fields @szakarias Could you look over that? |
|
@sigurdm it seems like the changes to |
|
Yeah I think splitting is fine if it simplifies things |
No description provided.