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

Jump to bottom

Remove Has/Clear members for C# message fields in proto2 #7429

Merged
merged 3 commits into from May 1, 2020
Merged

Remove Has/Clear members for C# message fields in proto2 #7429

merged 3 commits into from May 1, 2020

Conversation

jskeet
Copy link
Contributor

@jskeet jskeet commented Apr 27, 2020

@jskeet
Don't generate Has/Clear members for proto2 message fields.
39efb4b 
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
Regenerate C# code based on the previous commit
fb70c01 
(This removes the Has/Clear members for message types in proto2.)
@jskeet
Fix to C# support library code
b56a8de 
(This was the only use of a HasXyz property for a message type.)
@jskeet jskeet requested a review from haberman April 27, 2020 06:22
@jskeet jskeet mentioned this pull request Apr 28, 2020
Merged
@jskeet
Copy link
Contributor Author

jskeet commented Apr 29, 2020

@haberman Ping? Just checking you've seen this...

@haberman
Copy link
Member

haberman commented May 1, 2020

Sorry for the slow reply. I think this is fine from a correctness and API design perspective. My only possible worry is around the sorts of concerns you raised on #7434 (comment). It's true though that you only get the change when you regenerate your code.

@jskeet
Copy link
Contributor Author

jskeet commented May 1, 2020

My only possible worry is around the sorts of concerns you raised on #7434 (comment). It's true though that you only get the change when you regenerate your code.

Definitely understand the concern. I'm less worried here though, for a few reasons:

  • You're very unlikely (although it's not impossible) to end up "accidentally" upgrading your version of protoc due to some other dependency, whereas transitive dependencies mean it's quite likely that the version of the support library you end up with may not be the one you specify directly.
  • Any problems are found at build time rather than execution time
  • The proto2 generation has previously been labeled as experimental, whereas option fetching wasn't. Even though they were done in the same set of changes, I think there's a higher compatibility bar for anything in the support library.

If you're happy with those justifications, I'll merge. If not, I'm definitely happy to chat about it more and see whether there are alternative options (although I can't think of any right now).

cc @jtattermusch just for the sake of interest in compatibility.

@jskeet jskeet merged commit ed5c874 into protocolbuffers:master May 1, 2020
@jskeet jskeet deleted the message-presence branch May 1, 2020 08:06
@haberman haberman mentioned this pull request May 12, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@haberman haberman haberman approved these changes

Assignees
No one assigned
Labels
c# cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jskeet @haberman @googlebot