If you are already on that topic would mind also adding readonly as a keyword?

I initially wanted to add more keywords (abstract and final too) here but they seem to need a separate attention, while this PR can already be reviewed and possibly merged.

In other words, let do it separately.

in general I'm nervous about adding these completions as the mechanism for determining if they should be provided or not is not good vs. the effort of typing a few well known characters.

probably more instances where the keywords are either not provided or provided in the wrong place.

in general I'm nervous about adding these completions as the mechanism for determining if they should be provided or not is not good vs. the effort of typing a few well known characters.

probably more instances where the keywords are either not provided or provided in the wrong place.

I will try to look at these cases again.

I share your annoyance with such cases, but on the other hand, I'm even more bothered by the lack of completions in expected places. Habits of using lua-language-server raise my expectations here too.

At the moment I reproduced only the second case (in real use) - the first does not work after a few guesses.

This condition checks nodes on truncated document (up to cursor).

I noticed that commenting the above condition does not affect any test result. Where it could be needed?

dap> $nodeBeforeOffset === $node->parent->parent
true

Isn't it a bug?

At the moment I still confused about borders of classMembersBody: should we expect true if $node is like in your example (and the new failing data set)?

Now I think we need a separate method in CompletionContext to determine whether the current node is in CompoundStatement within some method or function declaration.

Now it seems to be ready to review again.

Some keywords like break, continue needs additional conditions. Maybe I'll do it here, as well as it can be done later.

instanceof seems to be more separate case. It follows expression and does not start any statement.

in general I'm nervous about adding these completions as the mechanism for determining if they should be provided or not is not good vs. the effort of typing a few well known characters.

probably more instances where the keywords are either not provided or provided in the wrong place.

I tried to set better priority for keywords:

https://github.com/phpactor/phpactor/pull/2622/files#diff-2ba75b9069ec2e0eca8655a5fa214ecb0050bf8656e6c1842d2be83b91d31f27R136

Experience with this PR shows that there can still exist lots of cases to consider to be statement context or not. Since implementing the new method in CompletionContext adding any next case became simple - especially not mixed with other contexts.

IMHO we could already merge this PR as is and possibly support new cases in the future. Before possible merge please report any annoying cases that I haven't found yet.

In practice I merge this and other PRs to one experimental branch and see what happens (independently of tests). For example I found

Fixed case like below:

The wrong suggestions also happen when there is no invalid syntax. Like in this fully defined match:

Yes, I caught it too and still have no time to focus on it.

Your example shows allowed behavior like

match(match(1) {1 => 1}) {1 => 1}

I meant rather an example like

or

or even

😀

At the previous edit I added the screenshot with the right side of instanceof but my bad - it can be an expression.

Again, I am nervous about that PR - it adds lots of conditionals to make up for the fact that the parser breaks in strange and unpredictable ways. It would need constant maintainence and will likely never behave properly. The last one I merged introduced false positives which I'd like to revert (const and match IIRC). If I merge it I will need to test it and I'm unlikely to find the time to do that.

I'd rather have no suggestions than wrong ones, for me at least the value is in completing class and function names, and not so much in keywords.

Feel free to close this PR if you think so (and no one lets us know they need it). I have some use for it and I want to continue this experiment even on a branch detached from any PR.

Test failure: #3007