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

Collapse union types #370

Merged
merged 12 commits into from
Oct 10, 2024
Merged

Collapse union types #370

merged 12 commits into from
Oct 10, 2024

Conversation

max-hoffman
Copy link

@max-hoffman max-hoffman commented Oct 10, 2024

Replace union type with one interface type. Static type access in reducer stack become runt-time interface conversions. The compiler builder loses the ability to do type checking at build time, so type safety has to be checked with testing.

Additional type enforcements are needed for nil-safety. Nil return values have to be typed correctly in the interface variable for casts up the stack to pass. Interface types do not have default nil values, so I've added tryCastXXX helper functions to accommodate untyped nils.

@max-hoffman max-hoffman requested a review from zachmu as a code owner October 10, 2024 17:49
Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

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

Didn't read grammar changes too closely, in general seems good. Make sure there's a recover around any parse methods now that we will panic on type errors.

@@ -547,7 +556,7 @@ func (q QueryOpts) Format(buf *TrackedBuffer) {
// Select represents a SELECT statement.
type Select struct {
Comments Comments
QueryOpts QueryOpts
QueryOpts *QueryOpts
Copy link
Member

Choose a reason for hiding this comment

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

I would avoid changing things to pointers or vice versa if possible, there are so many places we inspect the AST where expect a nil / zero value for a sub field etc. If it's required for perf that's fine, but be very careful and check all refs to them.

Copy link
Author

Choose a reason for hiding this comment

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

Restored previous pointer/merge behavior.

@max-hoffman
Copy link
Author

Locally testing bumps with GMS/Dolt look clean. Going to merge.

@max-hoffman max-hoffman merged commit 9d4f54b into main Oct 10, 2024
4 checks passed
@max-hoffman max-hoffman deleted the max/collapse-union-types branch October 10, 2024 20:14
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