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

Fix packed reflection handling bug in edition 2023. #18405

Merged
merged 1 commit into from
Sep 18, 2024

Conversation

mkruskal-google
Copy link
Member

This is a very narrow edge case where touching a packed extension via generated APIs first, and then doing so reflectively will trigger a DCHECK. Otherwise, reflective APIs will work but not use packed encoding for the extension. This was likely a pre-existing bug dating back to proto3, where it would only be visible on custom options (the only extensions allowed in proto3).

To help qualify this and uncover similar issues, unittest.proto was migrated to editions. This turned up some other minor issues in DebugString and python.

PiperOrigin-RevId: 675785611

This is a very narrow edge case where touching a packed extension via generated APIs first, and then doing so reflectively will trigger a DCHECK.  Otherwise, reflective APIs will work but not use packed encoding for the extension.  This was likely a pre-existing bug dating back to proto3, where it would only be visible on custom options (the only extensions allowed in proto3).

To help qualify this and uncover similar issues, unittest.proto was migrated to editions.  This turned up some other minor issues in DebugString and python.

PiperOrigin-RevId: 675785611
@mkruskal-google mkruskal-google added the back-port Cherrypick PRs to release branches label Sep 18, 2024
@mkruskal-google mkruskal-google requested review from tonyliaoss and sbenzaquen and removed request for a team September 18, 2024 22:06
@mkruskal-google mkruskal-google merged commit c4124f9 into 28.x Sep 18, 2024
187 checks passed
@mkruskal-google mkruskal-google deleted the packed-dcheck-fix branch September 18, 2024 22:35
Copy link

@Joj501 Joj501 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Submit changes to update

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
back-port Cherrypick PRs to release branches
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants