Skip to content
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

Consider removing HasFoo/ClearFoo() for proto2 message fields in C# #7395

Closed
jskeet opened this issue Apr 17, 2020 · 3 comments
Closed

Consider removing HasFoo/ClearFoo() for proto2 message fields in C# #7395

jskeet opened this issue Apr 17, 2020 · 3 comments
Assignees

Comments

@jskeet
Copy link
Contributor

jskeet commented Apr 17, 2020

To clarify that title, by "proto2 message fields" I mean "fields in a proto2-syntax message, where the type of the field is a message type".

This code:

var person = GetPersonFromSomewhere();
if (person.HasAddress)
{
   // Do something with the address field
}

is just equivalent to:

var person = GetPersonFromSomewhere();
if (person.Address != null)
{
   // Do something with the address field
}

or (more modern idiom)

var person = GetPersonFromSomewhere();
if (person.Address is object)
{
   // Do something with the address field
}

Likewise person.ClearAddress() is just equivalent to person.Address = null;

We should consider removing the presence accessors here.

Note that this becomes trivial to do after #7382 is merged.

@jskeet
Copy link
Contributor Author

jskeet commented Apr 27, 2020

Note that we can also remove the Has/Clear for oneof fields, given that users can use the existing FooCase/ClearFoo pair.
There's less of a compelling case to do so, however: oneof fields can never be explicitly optional, so we don't get into the inconsistency between "explicitly optional in proto3 vs implicitly optional in proto2" situation.

@jskeet
Copy link
Contributor Author

jskeet commented Apr 27, 2020

The case for removing it from oneof fields is still fairly compelling though:

  • It reduces the change when updating from proto2 to proto3
  • It reduces the number of members to look through in Intellisense
  • It avoids the slightly odd effect that Clear only has an effect if the oneof was set to that case already.

@jskeet
Copy link
Contributor Author

jskeet commented Apr 27, 2020

Oh, and I think other languages don't generate Has/Clear for individual oneof fields, either.

Looks like that's not the case. It also looks like this is an expected inconsistency when upgrading from proto2 to proto3 in other languages, so I'll leave it in C# too.

jskeet added a commit to jskeet/protobuf that referenced this issue Apr 27, 2020
This is a breaking change in terms of proto2 code generation: users who were previously using these members will have to change to null checks for message fields.
After toying with removing Has/Clear for proto2 oneof fields, I've left them alone in this commit, for consistency with other languages. The inconsistency between proto2 and proto3 won't come up here, because proto3 oneof fields can never be explicitly optional.

Fixes protocolbuffers#7395.
@jskeet jskeet closed this as completed in 9d74aaf May 1, 2020
haberman pushed a commit to haberman/protobuf that referenced this issue May 12, 2020
This is a breaking change in terms of proto2 code generation: users who were previously using these members will have to change to null checks for message fields.
After toying with removing Has/Clear for proto2 oneof fields, I've left them alone in this commit, for consistency with other languages. The inconsistency between proto2 and proto3 won't come up here, because proto3 oneof fields can never be explicitly optional.

Fixes protocolbuffers#7395.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant