Add prepare rename provider

[amiralies]

No description provided.

[amiralies]

https://microsoft.github.io/language-server-protocol/specifications/specification-current/#textDocument_prepareRename

this shows a feedback that the current position is not valid to be renamed to the user.
for exmaple try that on a let expression before and after this commit.

let abc = 123
 ^ 

for some reason the { defaultBehavior: boolean } approach didn't work for me

[cristianoc]

(pushed a commit by mistake on this branch). I was going to force-remove it, but don't know if you have some pending changes, so I'll let you remove it.

[cristianoc]

Wondering about the logic (and duplication of logic) here.
It seems intuitively, this should fire exactly when rename returns a non-empty set of changes.

Looking at the commands in Commands.ml, rename returns results always when locations is a non-empty array. So it seems this logic here is unnecessary. Or if necessary, how comes the same logic is not in the rename command?

Should this just call rename instead and check if the result is empty? Not sure why the protocol splits the logic in this way.

Also, just noticed utils.getReferencesForPosition was created when there was more than one use. Before this PR, there was a single use, so it should probably have been removed and inlined.

[amiralies]

I think the split is because of checking rename availability without actually analyzing refs. (I.e check the cursor position is an ident)

Ill look at it tomorrow to see if we can do it better.

[cristianoc]

One difference w.r.t. rename is that the new name is not supplied. So technically, the rename command cannot be called.

I think this prepare operation makes most sense in an architecture when one has direct access to parsing the document, in which case prepareRename in some cases could be resolved by just parsing.

In our case, we make use of the function that finds references. Note this is more expensive, but it should not matter much as rename is interactive.

[cristianoc]

Oh I see the command should find a range for the symbol. OK then the logic is necessary.

The only remaining comment I have is about returning a boolean directly, which seems to work on vscode but can't see supported in the spec.
Will make a suggestion for a change.

[amiralies]

(pushed a commit by mistake on this branch). I was going to force-remove it, but don't know if you have some pending changes, so I'll let you remove it.

Feel free to apply changes to this branch

[cristianoc]

@amiralies see my proposed change. The only real logical difference is avoiding to return a boolean directly.

[amiralies]

The boolean return was inside find method. I'm okay with the change forEach makes sense. it also matches with other parts of the codebase.

[cristianoc]

The boolean return was inside find method. I'm okay with the change forEach makes sense. it also matches with other parts of the codebase.

That only shows my little knowledge of the language. I did not know that return works like that!

[cristianoc]

Still, even if the semantics is correct, the readability would not be great. The only way to correctly interpret such a return as a reader would be to spot the little symbol => above.

[cristianoc]

Looks good!