KeywordCompletor: complete statement keywords

[przepompownia]

No description provided.

[mamazu]

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

[przepompownia]

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.

[dantleech]

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.

[przepompownia]

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.

[przepompownia]

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

[przepompownia]

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

[przepompownia]

phpactor/lib/Completion/Bridge/TolerantParser/CompletionContext.php

Lines 208 to 210 in b890cfb

	if ($nodeBeforeOffset instanceof CompoundStatementNode && $node->getStartPosition() < $nodeBeforeOffset->getEndPosition()) {
	return false;
	}

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

[przepompownia]

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)?

[przepompownia]

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.

[przepompownia]

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.

[przepompownia]

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

[przepompownia]

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.

[przepompownia]

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