fix(scanner): correct UB in scan's heredoc delete logic

[WillLillis]

I'm fairly certain taking the address of a non-lvalue is undefined behavior. The current Array definition 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.

[clason]

@WillLillis The node bindings are broken (tree-sitter npm package is stuck at 0.25.0); just disable the node tests until that is fixed.

[clason]

That's throwing the baby out with the bathwater ;)

It should be enough to set test-node: false here.

[clason]

(But you probably want to set generate: true to guard against a forgotten tree-sitter generate.)

[calebdw]

I purposely set generate: false because there was some bugs in the ci module---not sure if they've been fixed yet

[clason]

Which module? I am not aware of any bug regarding generate (which is used in nearly all parser repos I am familiar with).

[calebdw]

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

[clason]

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

[clason]

(You may be thinking of the regenerate workflow, which is a separate thing?)

[calebdw]

Then try it and see---it used to not support parallel parsers

[WillLillis]

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?

[clason]

We can also temporarily omit this parser from the fixture list to unblock this; this issue sounds serious enough for a timely bugfix release.

[clason]

looks like the go workflow uses an outdated Go toolchain? Maybe disable those tests for now, too.

Maybe disable those, too. (ceterum censeo these proliferating bindings are a moving target and need to be reined in...)

[WillLillis]

Marking this as a draft until the Array impl changes are in tree-sitter-cli release. I'm not able to easily get tests parsing correctly with 0.26.3 (likely due to the strict aliasing violatings the Array changes fix).