I just want to thank you @cfroystad and @maxbrunsfeld for doing all the work here. I have no idea how all this parsing stuff works, but you make my day job A LOT easier ❤️

I see you took option two, very nice! One trip-up I should have mentioned when branching off values in the valid_symbols array: if tree-sitter encounters a parse error and enters error recovery mode, the first thing it does is call your external scanner with all symbols marked as valid (I first thought this was a bug). So it's probably a good idea to detect this has happened and then decide what to do; see my code here: https://github.com/tlaplus-community/tree-sitter-tlaplus/blob/4c05e1c54e1e26624b5f9cccc840159ab8bb924b/src/scanner.cc#L1626

Thanks for yet another helpful insight!

Since the main grammar is in the internal parser, I suppose it's correct to do as you've done in your code: Hand control back from the external scanner and rather have the main parser call it again if it should find it useful.

One idea I had was to add a token $.error_sentinel (unused within the grammar) to the very end of my externals list, then branch only on its value within the external scanner to reduce the fragility of this error recovery detection scheme. But I haven't checked whether that works. Perhaps I should do that now.

Okay I just tested it and it works, see here: https://github.com/tlaplus-community/tree-sitter-tlaplus/blob/11c65d44e60a4086556b4819037499399b903a8d/src/scanner.cc#L1619

You need to make the change on your grammar's side too, like here: https://github.com/tlaplus-community/tree-sitter-tlaplus/blob/4c05e1c54e1e26624b5f9cccc840159ab8bb924b/grammar.js#L84

Brilliant! That made the detection less brittle to future changes! Thanks 🙂

• "hello ${a} world"; becomes (string [1, 1] - [1, 17]), but I would expect a variable_name to be in there.
• "hello \0 world"; becomes a string, but I would expect an escape_sequence in there.
• The following gives an error:

<?php
$ab = "ab";
echo "hello ${"a"."b"} world";

Also, braces can be escaped with a backslash. The following is valid in PHP but gives an error with tree-sitter in this branch:

<?php "\{$";

Edit: maybe it's not true that braces can be escaped. If you run echo "\{$";, it prints \{$. Not {$, which is what you would expect if the backslash escapes the brace. The docs also don't mention this escape sequence.

Thank you for testing and identifying some of the things I've missed. I'll look into fixing them when I get home from work. Most of them look like they'll be easy to fix (although slightly embarrassing 😄)

All review comments have been addressed in the last commit.

Thank you both for identifying bugs and suggesting good improvements.

With regard to escaping \{ it appears it's not really escaping but whenever those two letter coincide, they're both printed instead of interpreted. I've changed the implementation accordingly.

Nice work. It seems I was mistaken about dynamic variable names. "$$$$$$$$$$$$$a" is interpreted by PHP as many dollars and a single variable, not as a dynamic variable.

I also found some more edge cases that fail to parse:

• "${a}["
• "\u{$a}"

Nice work. It seems I was mistaken about dynamic variable names. "$$$$$$$$$$$$$a" is interpreted by PHP as many dollars and a single variable, not as a dynamic variable.

I also found some more edge cases that fail to parse:

• "${a}["
• "\u{$a}"

Thanks for testing!

I've fixed the two first items, but might need some guidance on the last one.

With the current implementation, I'd need to have two candidate mark_end (which as far as I understand is not supported):

" <- mark_end[string end: potential escape sequence]
\
u <- mark_end[string end: potential complex string element]
{ <- we don't know which to use yet
$ <- Ah, it's a complex string element, use "string end: potential complex string element" end
a
}
"

This might all be a consequence of me doing too much in the external scanner due to not having found a way to solve them in the main parser.

Any pointers/ideas on how to handle this would be greatly appreciated! (one option is of course to wait until Hack is done ironing out the same bugs, and try to adopt their approach)

I guess the lexer needs to scan both \u{0123} and \u{$a}, and the parser (grammar.js) needs to distinguish between the two.

However, this is really an exceptional edge case, so it would also be fine to merge this PR and leave it for now, in my opinion.

Thank you, @Sjord! That was the pointer I needed (in combination with driving along the coast of Norway, having loads of time while on ferries). The last edge case is no fixed - even though slightly hackish.

The edge case "adasdfafd\u{$a} will be translated into:

(program
    (php_tag)
    (expression_statement
        (encapsed_string
            (string)
            (string)
            (variable_name (name)
        )
    )
)

There's a conflict. You probably need to merge master in your branch (or rebase).

Thanks! I forgot to update the branch after merging another PR earlier. However, the conflict is only in the generated parser and should thus not be a blocker for final review (my merging powers for tree-sitter-php doesn't include changes as complex and wide-reaching as this)

I'll try to find a few minutes to rebase when I get home from work in a few hours 🙂

Rebased