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

chore(hogql): Full error & NULL handling in C++ parser (1.0.0) #18240

Merged
merged 7 commits into from
Oct 27, 2023

Conversation

Twixes
Copy link
Member

@Twixes Twixes commented Oct 27, 2023

Changes

This is the final stage of making the C++ parser production-ready: to prevent any and all segfaults, we must check for NULL pointers in all Python/C API call returning PyObject*, and for -1 in all calls returning an integer (e.g. PyList_Size(), but also PyObject_SetAttrString()).

I had to do a bunch more reading of CPython docs and code to get all of this right, which is why this is not the way I initially wrote the converter… even though that would have made for much more readable PRs than this one.

The way I went about making sure nothing is missed is basically just checking every match for this regex:

(Py(Object_|Unicode_|Dict_|List_|Long_|Float_|Tuple_|Import_|_(Va)?Build))|build_ast_node\(|is_ast_node_instance\(|get_ast_enum_member\(

I had to rewrite nearly each of the 200+ call sites to handle every situation where a NULL might pop up. This means throwing an error after decrementing refcounts for all PyObject*s in scope.

Additionally, I made sure we similarly clean up refcounts if any visit() call throws (which can be due to a Python exception, but also a syntax exception).

There's some new info on all of the above in hogql_parser/CONTRIBUTING.md.

How did you test this code?

All parser tests succeed.

@Twixes Twixes requested a review from mariusandra October 27, 2023 09:27
@PostHog PostHog deleted a comment from posthog-bot Oct 27, 2023
@PostHog PostHog deleted a comment from posthog-bot Oct 27, 2023
@Twixes Twixes force-pushed the turbo-parser-null-safe branch from 6089019 to 2e1f136 Compare October 27, 2023 10:15
@Twixes Twixes temporarily deployed to pypi-hogql-parser October 27, 2023 10:36 — with GitHub Actions Inactive
Copy link
Collaborator

@mariusandra mariusandra left a comment

Choose a reason for hiding this comment

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

Looks great, though I'm definitely unable to verify all these changes.

@Twixes Twixes changed the title chore(hogql): Be defensive against NULLs in the C++ parser chore(hogql): Be defensive against NULLs in the C++ parser (1.0.0) Oct 27, 2023
@Twixes Twixes changed the title chore(hogql): Be defensive against NULLs in the C++ parser (1.0.0) chore(hogql): Full error and NULL handling in C++ parser (1.0.0) Oct 27, 2023
@Twixes Twixes changed the title chore(hogql): Full error and NULL handling in C++ parser (1.0.0) chore(hogql): Full error & NULL handling in C++ parser (1.0.0) Oct 27, 2023
@Twixes Twixes merged commit 8f62e85 into master Oct 27, 2023
79 checks passed
@Twixes Twixes deleted the turbo-parser-null-safe branch October 27, 2023 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants