Skip to content
This repository has been archived by the owner on Mar 6, 2022. It is now read-only.

fix: signature helper #31

Merged
merged 3 commits into from
Apr 29, 2020

Conversation

camilledejoye
Copy link
Contributor

@camilledejoye camilledejoye commented Apr 23, 2020

I noticed there was a problem when an argument was a string containing a comma:
The argument count was off because the comma was interpreted as an argument separator.

So I tried to look around for examples on uncommon cases that might not been handled: nested calls, multiline arguments, etc.
I found some cases that was not handled so add a few more tests.

I try to explain my logic when it come to solving the issues in the commit message, don't hesitate to check it out.

There one case that might be a personal preference and I don't know how it should behave.
So any one reading this, please gave us your opinion so we can try to choose the more commonly accepted behavior:

<?php
function hello(string $foo, int $bar) {};
function goodbye(string $good, int $by) {};
hello(
    "hello",
    goo<>dbye("good", "by")
);

<> being the cursor position when we ask for the signature to be resolved.
I personally expect to have this result:

hello(string $foo, int $bar)

But I get that it can be confusing since:

<?php
function hello(string $foo, int $bar) {};
he<>llo("hello", 1);

Would resolved as:

hellow(string $foo, int $bar)

I guess it can be sum up to, would you gave the priority to resolve the signature of:

  • the call expression the cursor is on
  • the call expression the cursor is inside of

The idea is that if we already are **in** a call expression then we want the
signature of this call expression.
If we aren't then we want to signature of the call expression we are on.
If we are neither in or on an call expression, then we can't resolve any signature.

The idea is that 3 scenarios:
1. The cursor is inside a call expression (nested or not)
2. The cursor is on a call expression
3. The cursor is not inside or on a call expression

My idea was to focus more on the `ArgumentExpressionList` than on the
call expression.
There is two different kind of call expression which makes it awkward,
but for any arguements list expression the parent will always gave us
the closest call expression.
Also, in order ot fix the problem of comma being part of string which
messed up the active argument, I wanted to rely on the number of
`ArgumentExpression` instead on the comma characted.

It should be trivial:
1. Find the closest call expression (first ancestor in the AST)
2. Don't care about the `ArgumentExpressionList` since we are not in it
3. throw an exception

But the AST does not work like that, for example:
* If the cursor is on a variable which is an argument, the node is
`VariableExpression` so we need to find the closest
`ArgumentExpressionList`
* If we are on a blank line (multiline arguments) then the node at the
position is resolve to the call expression instead of the argument list,
so we need to check if there is a child `ArgumentExpressionList`

At then end it's messy and I'm not sure of every possible cases so I
decided to trust our tests.
I wrote every scenarios I could think of and that was not already in the
test file.
@dantleech dantleech merged commit 9d23f25 into phpactor:master Apr 29, 2020
@camilledejoye camilledejoye deleted the hotfix/signature-helper branch April 29, 2020 12:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants