Implement textDocument/signatureHelp #51

olafurpg · 2017-11-22T21:58:40Z

This feature is only triggered when opening a parentheses at a function
call-site. The box disappears as soon as you write the first parameter.
We reuse the completion mechanism to find the method symbol and extract
the parameter lists.

This feature is only triggered when opening a parentheses at a function call-site. The box disappears as soon as you write the first parameter. We reuse the completion mechanism to find the method symbol and extract the parameter lists.

ShaneDelmore · 2017-11-22T23:42:27Z

I was unable to get this to work. I tried duplicating your success step exactly but when I type Predef.assert(
then pause I see no dialog, only a message recorded in the output dialog of vscode

[Error - 3:29:22 PM] Request completionItem/resolve failed.
  Message: Invalid params
  Code: -32602 
[object Object]

tailing metaserver.log shows me a request was made, but I don't see any response for it. Not sure if I am looking in the correct places or not.

15:38:48.997 DEBUG l.core.Connection - Received {"jsonrpc":"2.0","method":"textDocument/didChange","params":{"textDocument":{"uri":"file:///Users/sdelmore/workspace/language-server/test-workspace/a/src/test/scala/example/UserTest.scala","version":28},"contentChanges":[{"text":"package example\n\nclass UserTest {\n  val user = a.User.apply(name = \"\", 1)\n  user.copy(age = 2)\n  List\n    .apply(1, 2)\n    .map(x => x.+(2))\n  scala.runtime.CharRef.create('a')\n  Predef.assert()\n}\n"}]}}

laughedelic · 2017-11-23T04:03:30Z

I just updated my Atom plugin to try this out. And it works!

Although I'm not sure it's supposed to look like this.. 🤔

tailing metaserver.log shows me a request was made, but I don't see any response for it. Not sure if I am looking in the correct places or not.

@ShaneDelmore you see didChange notification, but what you should look for instead is something like this:

DEBUG l.core.Connection - Received {"jsonrpc":"2.0","id":40,"method":"textDocument/signatureHelp","params":{"textDocument":{"uri":"file:///Users/laughedelic/dev/laughedelic/language-server/test-workspace/a/src/main/scala/example/User.scala"},"position":{"line":7,"character":13}}}

and response:

INFO  s.m.l.Compiler - Signature help at /Users/laughedelic/dev/laughedelic/language-server/test-workspace/a/src/main/scala/example/User.scala:7:13
DEBUG l.core.MessageWriter - payload: {"jsonrpc":"2.0","result":{"isIncomplete":false,"items":[]},"id":39}
DEBUG l.core.MessageWriter - payload: {"jsonrpc":"2.0","result":{"signatures":[{"label":"apply","documentation":"(name: String, age: Int)a.User","parameters":[{"label":"name","documentation":"name: String"},{"label":"age","documentation":"age: Int"}]}]},"id":40}

check that the VS Code extension refers to the right version of the server, that you published locally (now you have to set it explicitly (i.e. fe14433))
check that you opened test-workspace/a/. I opened test-workspace/ at first and then things were weird, some stuff worked, other not (probably it's Atom specific).

laughedelic · 2017-11-23T04:49:58Z

Here's what I got with some little changes (laughedelic@0749371). I think documentation field is only for the scaladoc (which we don't have yet, right?), and label is directly what will be shown in the interface:

(notice that the first parameter is highlighted in bold)

Probably you guys don't care much for Atom, but I think it's useful to see how different clients implement these things to stay unbiased. Here's atom-ide-ui docs for this thing. Some relevant implementation details:

Once activated, all signature help providers (matching the current grammar scope) will continuously be queried for signatures when the cursor location changes, regardless of the value of triggerCharacters.

laughedelic · 2017-11-23T04:58:37Z

And here is how it looks in VSCode:

which is quite the same as in Atom ☺️ except it let's you to check the second alternative (I guess in atom-ide-ui it's just not implemented yet)

gabro · 2017-11-23T05:00:34Z

Nice! does it highlight the second parameter when you get to it?

laughedelic · 2017-11-23T05:02:48Z

No, but it's just not implemented yet as far as I understand. I didn't read the code in details yet, so I'm not sure how exactly it works.

laughedelic · 2017-11-23T05:13:43Z

Moreover, something strange is happening when you start typing:

P.S. I just discovered that you can switch those alternative signatures with ↑/↓ keys or ^P/^N. Cool! 💥

olafurpg · 2017-11-23T08:27:49Z

@laughedelic that weird behavior is due to the fact that signature helper uses code completions to get the symbols of the methods, it works fine as long as you only invoke it on the trigger character (We need to find a more creative way to handle signature help while you fill out the argument list.

olafurpg · 2017-11-23T08:30:08Z

check that the VS Code extension refers to the right version of the server, that you published locally (now you have to set it explicitly (i.e. fe14433))

This will grow tiring very quickly. Is there any chance to use a stable -SNAPSHOT version number outside of CI?

It grows very tiring to manually update extension.js on every new commit.

Previously we blindly used the offset provided by the client even if that offset didn't point to a trigger character. Now we locate the relevant open paren.

olafurpg · 2017-11-23T09:00:14Z

@laughedelic I've fixed the weird behavior for basic cases but to make it handle multiple nested arguments we'll need a slightly more sophisticated solution explained in #52. Now it behaves like this

laughedelic · 2017-11-23T12:10:58Z

This will grow tiring very quickly. Is there any chance to use a stable -SNAPSHOT version number outside of CI?

I agree. So did you solve it with a9c4cac?

I've fixed the weird behavior for basic cases

Looks good! 👍

jvican

laughedelic · 2017-11-23T15:53:46Z

metaserver/src/main/scala/scala/meta/languageserver/Compiler.scala

+        // symbol of `bar` when we want to match against `foo`.
+        // Related https://github.com/scalameta/language-server/issues/52
+        val lastParenOffset =
+          position.source.content.lastIndexOf('(', position.point)


This works outside of parentheses too 😄

laughedelic · 2017-11-23T15:56:36Z

metaserver/src/main/scala/scala/meta/languageserver/Compiler.scala

+              }
+            }
+            SignatureInformation(
+              label = s"${sym.nameString}${sym.info.toLongString}",


I combined this just from what was already around. But is there a better way to get separately parameters list and return type?

laughedelic · 2017-11-23T15:58:10Z

metaserver/src/main/scala/scala/meta/languageserver/Compiler.scala

+              .completionsAt(lastParenPosition)
+              .matchingResults()
+              .distinct
+            if member.sym.isMethod


What about constructors?

Previously, we found the first open paren but now we keep a stack of how many open parens are in between. This change makes it possible to return the correct active parameter, as well as filtering out overloads that have too few parameters. This also means we no longer return bogus results when outside of an argument list.

olafurpg · 2017-11-23T20:29:33Z

I think I got it working reasonably well now, it tracks the active argument and doesn't return results anymore if you're outside of an argument list! The activeParameter detection is character based so it will count commas inside string literals so that foo(", ", <caret> will say the active argument is 2 when it should be 1. We could use the tokenizer to fix this case, but then we'll have to deal with other problems like unclosed string literals. I think we can leave experiments with that for a future PR!

olafurpg · 2017-11-23T20:30:12Z

What about constructors?

@laughedelic great question! I'll take a look, we can try to fallback to the apply member if the symbol we found is not a method

olafurpg · 2017-11-23T20:40:43Z

That User(<caret> example @laughedelic crashes 😂

java.lang.StringIndexOutOfBoundsException: String index out of range: -2
  java.lang.String.<init>(String.java:196)
  scala.tools.nsc.interactive.Global.typeCompletions$1(Global.scala:1217)

This commit prevents a StringIndexOutOfBounds by triggering a mutative call to `.typedTreeAt` before calling `.completionsAt`. If the completions return no results, which is what happens for `.apply`, we fallback to the symbol of `typedTreeAt`.

olafurpg · 2017-11-23T21:08:28Z

I got .apply working too 💃

Previously, didn't use the open paren offset to find the fallback `.apply` symbol causing a NPE since `.typedTreeAt` returns null on error.

Previously we only returned the primary constructor, which is a bit annoying esp. when dealing with java apis.

olafurpg · 2017-11-23T21:33:58Z

Lastly, secondary constructors also get some love ❤️

Ok, I think this PR is ready to go!

gabro · 2017-11-23T23:55:00Z

Very nice job! These are super useful when paired with autocompletion! A thing I can see improving (for future PRs) is to pretty print the signature. For instance adding a colon between the param list and the result typ

gabro · 2017-11-24T00:18:14Z

metaserver/src/test/scala/tests/compiler/CompilerSuite.scala

+  trait OverloadHack2; implicit object OverloadHack2 extends OverloadHack2
+  trait OverloadHack3; implicit object OverloadHack3 extends OverloadHack3
+  trait OverloadHack4; implicit object OverloadHack4 extends OverloadHack4
+  trait OverloadHack5; implicit object OverloadHack5 extends OverloadHack5


What's this for? If it's for disambiguating the erased signature, can you maybe use DummyImplicit on just one of the targeted signature below?

This is copied from the scalameta test suite where there are 6 targeted overloads. It seems we don't need any overloads here so we can just remove this.

laughedelic · 2017-11-24T01:30:48Z

It works now with User, but I can't make it work with any constructor from the standard lib, like List(), or sys.process.Process().

Also, this looks strange:

(besides different typesetting and no syntax highlighting, which is Atom's fault)

It seems that the first parameter list is missing (and brackets around type parameters too). And by the way, what about multiple parameter lists?

It seems it doesn't matter if we force compilation or not the exception still occurs. Best to catch it for now.

Previously, signature help didn't work for varargs or applications with explicit type parameters. This commit generalizes the open/close trick to work for both brackets and parens, this opens up the possibility to show signature help for type parameters too in the future. Varargs are also special cased since the argument count does not match the length of the method's parameter list.

olafurpg · 2017-11-24T08:44:52Z

@laughedelic I got List(<<1>> working, that one was tricky since there is both an inferred .apply and an inferred type parameter Int. I also added special handling for varargs so that the signature help doesn't disappear on the trailing list of arguments.

Unfortunately, I couldn't get Process to work, the presentation compiler doesn't have the type information. I suspect it's related to how the .apply methods a mixed into the Process object with a trait.

olafurpg · 2017-11-24T08:46:06Z

@laughedelic I am not able to reproduce the missing argument list for map

olafurpg · 2017-11-24T08:46:35Z

Gonna merge this since we have #55 using the same testing infrastructure

Implement textDocument/signatureHelper.

fe14433

This feature is only triggered when opening a parentheses at a function call-site. The box disappears as soon as you write the first parameter. We reuse the completion mechanism to find the method symbol and extract the parameter lists.

olafurpg force-pushed the signaturehelp branch from aa3ba1e to fe14433 Compare November 22, 2017 22:02

laughedelic added the lsp feature label Nov 23, 2017

laughedelic and others added 2 commits November 23, 2017 09:34

Moved what was in documentation field to the label

e69bfc9

Use stable version number outside of CI.

a9c4cac

It grows very tiring to manually update extension.js on every new commit.

olafurpg mentioned this pull request Nov 23, 2017

Use activeSignature and activeParameter fields of SignatureHelp #52

Closed

Prevent false positive signature helps by locating (

a2f684f

Previously we blindly used the offset provided by the client even if that offset didn't point to a trigger character. Now we locate the relevant open paren.

jvican approved these changes Nov 23, 2017

View reviewed changes

laughedelic reviewed Nov 23, 2017

View reviewed changes

olafurpg added 2 commits November 23, 2017 20:45

Implement minimal testing framework for presentation compiler

93acadb

Add special handling for synthetic .apply

e654ee4

This commit prevents a StringIndexOutOfBounds by triggering a mutative call to `.typedTreeAt` before calling `.completionsAt`. If the completions return no results, which is what happens for `.apply`, we fallback to the symbol of `typedTreeAt`.

Use the correct position for fallback symbol.

83b7ede

Previously, didn't use the open paren offset to find the fallback `.apply` symbol causing a NPE since `.typedTreeAt` returns null on error.

Add special handling for secondary constructors.

23bfa64

Previously we only returned the primary constructor, which is a bit annoying esp. when dealing with java apis.

olafurpg mentioned this pull request Nov 23, 2017

Missing test infrastructure for interactive.Global #47

Closed

gabro reviewed Nov 24, 2017

View reviewed changes

gabro mentioned this pull request Nov 24, 2017

Add Hover Test and fix broken cases #55

Merged

olafurpg added 4 commits November 24, 2017 09:05

Handle a case where completionsAt crashed with StringIndexOutOfBounds.

7a0cbe3

It seems it doesn't matter if we force compilation or not the exception still occurs. Best to catch it for now.

Clean up CompilerSuite

8c1f710

Add note that we should be working on trees rather than chars.

883c4f5

olafurpg merged commit c9a6adf into scalameta:master Nov 24, 2017

olafurpg deleted the signaturehelp branch November 24, 2017 08:46

olafurpg mentioned this pull request Nov 24, 2017

Travis build stages #56

Merged

gabro mentioned this pull request Nov 24, 2017

Signature Help request #20

Closed

laughedelic changed the title ~~Implement textDocument/signatureHelper.~~ Implement textDocument/signatureHelp Nov 24, 2017

laughedelic mentioned this pull request Nov 24, 2017

Signature help service support laughedelic/atom-ide-scala#14

Merged

olafurpg mentioned this pull request Nov 25, 2017

Move completions to new compiler testing infrastructure #59

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement textDocument/signatureHelp #51

Implement textDocument/signatureHelp #51

olafurpg commented Nov 22, 2017

ShaneDelmore commented Nov 22, 2017

laughedelic commented Nov 23, 2017 •

edited

laughedelic commented Nov 23, 2017 •

edited

laughedelic commented Nov 23, 2017

gabro commented Nov 23, 2017

laughedelic commented Nov 23, 2017

laughedelic commented Nov 23, 2017 •

edited

olafurpg commented Nov 23, 2017

olafurpg commented Nov 23, 2017

olafurpg commented Nov 23, 2017

laughedelic commented Nov 23, 2017

jvican left a comment

laughedelic Nov 23, 2017

laughedelic Nov 23, 2017 •

edited

laughedelic Nov 23, 2017 •

edited

olafurpg commented Nov 23, 2017

olafurpg commented Nov 23, 2017

olafurpg commented Nov 23, 2017

olafurpg commented Nov 23, 2017

olafurpg commented Nov 23, 2017

gabro commented Nov 23, 2017

gabro Nov 24, 2017

olafurpg Nov 24, 2017

laughedelic commented Nov 24, 2017 •

edited

olafurpg commented Nov 24, 2017

olafurpg commented Nov 24, 2017

olafurpg commented Nov 24, 2017

Implement textDocument/signatureHelp #51

Implement textDocument/signatureHelp #51

Conversation

olafurpg commented Nov 22, 2017

ShaneDelmore commented Nov 22, 2017

laughedelic commented Nov 23, 2017 • edited

laughedelic commented Nov 23, 2017 • edited

laughedelic commented Nov 23, 2017

gabro commented Nov 23, 2017

laughedelic commented Nov 23, 2017

laughedelic commented Nov 23, 2017 • edited

olafurpg commented Nov 23, 2017

olafurpg commented Nov 23, 2017

olafurpg commented Nov 23, 2017

laughedelic commented Nov 23, 2017

jvican left a comment

Choose a reason for hiding this comment

laughedelic Nov 23, 2017

Choose a reason for hiding this comment

laughedelic Nov 23, 2017 • edited

Choose a reason for hiding this comment

laughedelic Nov 23, 2017 • edited

Choose a reason for hiding this comment

olafurpg commented Nov 23, 2017

olafurpg commented Nov 23, 2017

olafurpg commented Nov 23, 2017

olafurpg commented Nov 23, 2017

olafurpg commented Nov 23, 2017

gabro commented Nov 23, 2017

gabro Nov 24, 2017

Choose a reason for hiding this comment

olafurpg Nov 24, 2017

Choose a reason for hiding this comment

laughedelic commented Nov 24, 2017 • edited

olafurpg commented Nov 24, 2017

olafurpg commented Nov 24, 2017

olafurpg commented Nov 24, 2017

laughedelic commented Nov 23, 2017 •

edited

laughedelic commented Nov 23, 2017 •

edited

laughedelic commented Nov 23, 2017 •

edited

laughedelic Nov 23, 2017 •

edited

laughedelic Nov 23, 2017 •

edited

laughedelic commented Nov 24, 2017 •

edited