fix ifinite loop caused by php brace matcher in embedded lexer context

[haidubogdan]

This pull request tries to fix issue #7803 .

Php brace matcher used for highlighting (if, foreach ) statements like <?php if ($x): ?> ...<?php endif;?> loops through tokens until it finds the opening parenthesis.
The current implementation can trigger a infinite loop for a embedded usage of Php lexer as there is no exit mechanism beside the PhpTokenId.TOKEN check.

---

^Add meaningful description above

Click to collapse/expand PR instructions

By opening a pull request you confirm that, unless explicitly stated otherwise, the changes -

• are all your own work, and you have the right to contribute them.
• are contributed solely under the terms and conditions of the Apache License 2.0 (see section 5 of the license for more information).

Please make sure (eg. git log) that all commits have a valid name and email address for you in the Author field.

If you're a first time contributor, see the Contributing guidelines for more information.

If you're a committer, please label the PR before pressing "Create pull request" so that the right test jobs can run.

PR approval and merge checklist:

1. Was this PR correctly labeled, did the right tests run? When did they run?
2. Is this PR squashed?
3. Are author name / email address correct? Are co-authors correctly listed? Do the commit messages need updates?
4. Does the PR title and description still fit after the Nth iteration? Is the description sufficient to appear in the release notes?

If this PR targets the delivery branch: don't merge. (full wiki article)

[tmysik]

Approved, thank you.

[matthiasblaesing]

@haidubogdan It would be helpful to see an example that causes a loop. While I agree, that a test would be the best case, I think a review can substitute that. Infinite loops are really ugly to test for (at least the failing variant).

[haidubogdan]

@matthiasblaesing
Sure, a mock module with php embedding would be enough?
I didn't work with functional test netbeans yet. But I can check some triggers action on a thread for brace matcher.

[matthiasblaesing]

@haidubogdan I'd like to "see" the problem, so instructions how to reproduce (if necessary with code) would be great.

[haidubogdan]

@matthiasblaesing, @tmysik

I've created this branch master...haidubogdan:netbeans:t_php_bracematcher_infinite_loop_test

It's just for simulating the infinite loop.

Context:

I have a custom language support (text/x-php-test).
I include embedding on the lexer site for some tokens using PHPTokenId.languageInPHP() interpolated in {{ }}
The snippet code is :{{(TestClass^:)}} .
I've put dump of a counter in PHPBracesMatcher on line 119
Doing the test for the class EmbeddedBraceMatchingTest will output the infinite counter progress.

[matthiasblaesing]

@haidubogdan thank you. It makes sense that this was not caught before as for "normal" PHP situations this problem can't manifest. The scanning stops when the first token with a ID <> PHPTokenId.PHP_TOKEN is found. That will be PHP_OPENTAG. That will always be present as without it we don't get into PHP mode. This needs the embedding case to happen. The problem manifests because LexUtilities.findPreviousToken will report the first token if the target set is not found.

So the change makes sense to me. An alternative I would see is to guard the ts.movePrevious inside the do-while loop like this:

                        if(! ts.movePrevious()) {
                            break;
                        }

The movePrevious will report false and exit the loop for the first element in the token sequence.

What do you think?

[haidubogdan]

@haidubogdan thank you. It makes sense that this was not caught before as for "normal" PHP situations this problem can't manifest. The scanning stops when the first token with a ID <> PHPTokenId.PHP_TOKEN is found. That will be PHP_OPENTAG. That will always be present as without it we don't get into PHP mode. This needs the embedding case to happen. The problem manifests because LexUtilities.findPreviousToken will report the first token if the target set is not found.

So the change makes sense to me. An alternative I would see is to guard the ts.movePrevious inside the do-while loop like this:

                        if(! ts.movePrevious()) {
                            break;
                        }

The movePrevious will report false and exit the loop for the first element in the token sequence.

What do you think?

@matthiasblaesing
Seems cleaner :) .
I will use this solution.

[matthiasblaesing]

@haidubogdan thanks for the update. Looks cleaner and I appreciate the added test!
@tmysik would you mind having a second look? The implementation changed and a test was added. I would be inclined to merge.

[tmysik]

Looks good to me, just a minor comment added (related to the coding style). Thank you