Support LSP Function parameter completion

[vicapow]

Before

After

[nmote]

This looks pretty good. I'll have to double-check it against the LSP spec, but your screencast is compelling evidence that it works. I think I'll wait to review until (1) #7500 has landed (since it looks like you'll have to solve merge conflicts) and (2) you've added tests (which I assume you're waiting on #7500 for).

[vicapow]

the CI failure seems unrelated?

pushd : Cannot find path 'C:\projects\flow\bin' because it does not exist.
At C:\projects\flow\resources\appveyor\after_build.ps1:2 char:3
+   pushd "C:\projects\flow\bin"
+   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : ObjectNotFound: (C:\projects\flow\bin:String) [Push-Location], ItemNotFoundException
    + FullyQualifiedErrorId : PathNotFound,Microsoft.PowerShell.Commands.PushLocationCommand
 
    Directory: C:\projects\flow

[nmote]

The real error is higher up:

File "hack/heap/sharedMem.ml", line 105, characters 8-19:
Error: Unbound module EventLogger

The blame appears to be 3c80858, so yeah, unrelated.

[nmote]

Overall this looks pretty good to me. Unfortunately, when I was reading the spec I noticed that insertText is deprecated. It suggested using textEdit instead, although I'm not sure how you would get the desired tab-stop behavior using that interface.

What do you think about this? Should we respect the deprecation and try to find an alternative? Is there even a viable alternative?

[vicapow]

@nmote I've updated this PR to avoid the deprecated API. It was a little more work but I think this makes sense.

[nmote]

I have a few comments:

• Unless I'm mistaken, you haven't updated the tests with the new changes. Could you update them?
• It looks like it still provides insertTextFormat even though it's no longer providing insertText -- was that an oversight, or does the spec say that insertTextFormat applies to textEdits as well?
  • Along the same lines, it looks like it uses the snippet format for textEdits.
• Could you verify that you've tested this iteration manually with VSCode?

[vicapow]

I have a few comments:

• Unless I'm mistaken, you haven't updated the tests with the new changes. Could you update them?

Good catch. Updated.

• It looks like it still provides insertTextFormat even though it's no longer providing insertText -- was that an oversight, or does the spec say that insertTextFormat applies to textEdits as well?

The spec seems to say insertTextFormat applies to textEdit (and textEdits) as well. From the spec:

	/**
	 * The format of the insert text. The format applies to both the `insertText` property
	 * and the `newText` property of a provided `textEdit`.
	 */
	insertTextFormat?: InsertTextFormat;

• Along the same lines, it looks like it uses the snippet format for textEdits.

We use the snippet format with insertTextFormat = 2 (Snippet).

• Could you verify that you've tested this iteration manually with VSCode?

I have tested this iteration manually in VSCode, as well.

One more thing, I just realized, we should check for snippetSupport from the client capabilities response.

[nmote]

This looks good, thanks. The manual position manipulation makes me a little bit nervous, so I'm going to take a closer look at that in a bit. First though, I'll import it and spend some time playing around with it to make sure it behaves the way I expect.

Unfortunately we're having some technical difficulties, and I can't import PRs right now. Hopefully that will be resolved soon, and I'll be able to move forward.

[facebook-github-bot]

@nmote has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[nmote]

The lsp/completion tests failed for me when I tried running them locally (both on OS X and Linux). Could you verify that the tests pass for you?

[facebook-github-bot]

@nmote has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[gabelevi]

This looks awesome! One inline question about the test

[facebook-github-bot]

@nmote has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[facebook-github-bot]

@nmote merged this pull request in cbab31e.