fix panic on in list expansion with a subselect#18
Merged
Conversation
mrj0
force-pushed
the
mj/in-list-panic
branch
from
81432d8 to
4c69699
Compare
ccoVeille
approved these changes
Dec 25, 2025
Contributor
ccoVeille
left a comment
ccoVeille
left a comment
There was a problem hiding this comment.
One minor remark, otherwise 👍
bind.go
Outdated
Comment on lines
215
to
217
| if !inIn || !argMeta.v.IsValid() { | ||
| // this QuestionMark is not in an in-list that needs expansion. | ||
| // if v is invalid, then the arg isn't a slice |
Contributor
There was a problem hiding this comment.
Comment is good, but maybe it's time to give meaningful names to these variable
Suggested change
| if !inIn || !argMeta.v.IsValid() { | |
| // this QuestionMark is not in an in-list that needs expansion. | |
| // if v is invalid, then the arg isn't a slice | |
| isSlice := argMeta.v.IsValid() | |
| if !needExpansion || !isSlice { |
Anything like that. The comments could remain my point is to use variables with meaningful names
ccoVeille
approved these changes
Dec 26, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #17
The in-list expansion previously didn't consider if the argument wasn't a slice and ran into a panic when trying to expand the list.
Alternatively, this fix could have tried to detect and not consider subselects as
inIn, but I don't know off-hand what else that might break. The code is looking back forinand potentially look forward if the next word isselect. But stuff like that feels brittle and I don't know all the possible sql expressions that might be looked for. Alternatively, it could look for the next token being QuestionMark, but that would break valid in-lists likein ('a', ?).