fix(scanner): correct UB in scan's heredoc delete logic#293
fix(scanner): correct UB in scan's heredoc delete logic#293calebdw merged 4 commits intotree-sitter:masterfrom
scan's heredoc delete logic#293Conversation
|
@WillLillis The node bindings are broken ( |
.github/workflows/ci.yml
Outdated
| # with: | ||
| # generate: false | ||
| # test-rust: true | ||
| # test-node: true |
There was a problem hiding this comment.
That's throwing the baby out with the bathwater ;)
It should be enough to set test-node: false here.
There was a problem hiding this comment.
(But you probably want to set generate: true to guard against a forgotten tree-sitter generate.)
There was a problem hiding this comment.
I purposely set generate: false because there was some bugs in the ci module---not sure if they've been fixed yet
There was a problem hiding this comment.
Which module? I am not aware of any bug regarding generate (which is used in nearly all parser repos I am familiar with).
There was a problem hiding this comment.
it had to do with the two grammars which the parser-test-action did not support
Additionally, it only generates the grammar for the tests, not the PR so I would rather ci fail because you didn't regenerate as opposed to the ci passing and then being merged without generation
There was a problem hiding this comment.
And the workflow supports parallel parsers; see https://github.com/tree-sitter-grammars/tree-sitter-markdown/blob/split_parser/.github/workflows/ci.yml or https://github.com/tree-sitter-grammars/tree-sitter-csv/blob/master/.github/workflows/ci.yml
There was a problem hiding this comment.
(You may be thinking of the regenerate workflow, which is a separate thing?)
There was a problem hiding this comment.
Then try it and see---it used to not support parallel parsers
There was a problem hiding this comment.
tree-sitter generate will replace the array.h headers that this PR has included in its second commit with the headers as they were committed in 0.26.3. When the new array_delete macro is replaced with the old one, we then get the same crash observed in CI runs for tree-sitter/tree-sitter#5242. I think this is likely due to the issues outlined in tree-sitter/tree-sitter#4960, the crash is definitely "weird"/ smells of an overly aggressive optimization. (If I add a printf in for any member of word before array_delete, everything works perfectly).
Given this, I'm not sure if this should land until after tree-sitter/tree-sitter#5242 is included in a tagged release for tree-sitter-cli. While it's a bit more work, I suspect we can do something to work around this problem for the purposes of our corpus tests in the core repo. Thoughts @clason?
There was a problem hiding this comment.
We can also temporarily omit this parser from the fixture list to unblock this; this issue sounds serious enough for a timely bugfix release.
425eb42 to
e8074c9
Compare
|
looks like the Maybe disable those, too. (ceterum censeo these proliferating bindings are a moving target and need to be reined in...) |
e8074c9 to
e8f7980
Compare
|
Marking this as a draft until the |
|
tree-sitter 0.26.5 is out now with the array fixes, so we finally have CI passing. |
I'm fairly certain taking the address of a non-lvalue is undefined behavior. The current
Arraydefinition allows for this to work somehow, but this patch works both with current tree-sitter as well as with the changes from tree-sitter/tree-sitter#5242.Ran into this while working on tree-sitter/tree-sitter#5242
After this, would it be possible for a new release? tree-sitter-php is used for the core repo's corpus tests, and I believe this bug is causing CI to fail for tree-sitter/tree-sitter#5242.