28 add basic autocompletion #51

laginha87 · 2018-03-01T20:19:24Z

This PR improves scry's autocompletion by adding method completion for some cases.
The strategy applied was inspired by https://github.com/techMagister/cracker/ where:

Methods from all the files required by the file that is being edited are preloaded in a db.
When method completion is activated it tries to guess the type of the object the method it's being called on by using a bunch of regexes on the text from the beginning to the file until the point the completion is being called on.
After identifying the type it queries the db for the methods for that type matching the text.

For now it only covers the following scenarios:

Variable was assigned a boolean (a = true; a.m)
Variable was assigned an int (a = 1; a.m)
The var's type is declared in the method definition (ex: def(a: File); a.me)
The object where its called is a class. (ex: File.exp)
The method is defined as a property/getter/setter

Also it doesn't cover:

Namespaces
Method documentation
Returns of method calls
Toplevel methods
Instance methods inside own class

…h name

…e modules

laginha87 · 2018-03-01T20:30:56Z

Still want to had some more tests but wanted to have some input on the direction I took this.

faustinoaq · 2018-03-02T00:02:20Z

Hi @laginha87 Thank you for this PR 👍

I'm trying to test this on VSCode but I got an error:

Received message which is neither a response nor a notification message:
{
    "jsonrpc": "2.0",
    "error": {
        "code": -32001,
        "message": "Missing hash key: \"/home/main/Projects/Console/program.cr\"",
        "data": [
            "Hash(String, Tuple(Scry::TextDocument, Scry::Completion::MethodDB))",
            "Hash(String, Tuple(Scry::TextDocument, Scry::Completion::MethodDB))",
            "Scry::Workspace#get_file<Scry::TextDocumentIdentifier>:Tuple(Scry::TextDocument, Scry::Completion::MethodDB)",
            "Scry::Context#dispatchRequest<Scry::TextDocumentPositionParams, Scry::RequestMessage>:(Scry::ResponseMessage | Nil)",
            "Scry::Context#dispatch<Scry::RequestMessage>:(Scry::Initialize | Scry::ResponseMessage | Nil)",
            "Scry::start:(IO+ | Int32 | Nil)",
            "__crystal_main",
            "_crystal_main<Int32, Pointer(Pointer(UInt8))>:Nil",
            "Crystal::main_user_code<Int32, Pointer(Pointer(UInt8))>:Nil",
            "Crystal::main<Int32, Pointer(Pointer(UInt8))>:Int32",
            "main",
            "__libc_start_main",
            "_start",
            "???"
        ]
    }

Here is my scry log file

faustinoaq · 2018-03-02T00:09:22Z

Oh, I'm testing this 👇 code

def foo(bar : Int32)
  bar
end

foo(42)

laginha87 · 2018-03-02T08:03:13Z

😢
@faustinoaq thanks for the feedback, I will take a look.
I'm still not familiar with somethings in vscode and LSP. In your example you don't have a workspace right? Cause there's a lot of code that runs when the workspace opens that preloads a bunch of stuff.

faustinoaq · 2018-03-02T21:20:21Z

Hi @laginha87

In your example you don't have a workspace right?

No, I'm using a workspace in that example, look at titlebar, the workspace name is Console

@laginha87 I tried sublime as well, and same error happens, maybe we're missing some json field? 😅

Scry logs on Sublime Text 3 LSP

laginha87 · 2018-03-03T09:02:18Z

@faustinoaq I was preloading every crystal file under src/ your example doesn't have that changed it to preloading every cr file under the workspace root.

faustinoaq · 2018-03-03T16:27:39Z

Hi @laginha87, Thank you for yout last commit! Scry server is running now 🎉

althought completion doesn't work yet on vscode nor sublime

I tried with/without a workspace and I got this:

Variable was assigned a boolean (a = true; a.m)

a = true; a.to_s

D, [2018-03-03 11:01:48 -05:00 #9095] DEBUG -- : Content-Length: 171
D, [2018-03-03 11:01:48 -05:00 #9095] DEBUG -- :
D, [2018-03-03 11:01:48 -05:00 #9095] DEBUG -- : {"jsonrpc":"2.0","id":93,"method":"textDocument/completion","params":{"textDocument":{"uri":"file:///home/main/Projects/Foo/foo.cr"},"position":{"line":2,"character":12}}}
D, [2018-03-03 11:01:48 -05:00 #9095] DEBUG -- : textDocument/completion
D, [2018-03-03 11:01:48 -05:00 #9095] DEBUG -- : Couldn't find type Bool
D, [2018-03-03 11:01:48 -05:00 #9095] DEBUG -- : Scry::ResponseMessage(@jsonrpc="2.0", @id=93, @result=[], @error=nil)
D, [2018-03-03 11:01:48 -05:00 #9095] DEBUG -- : Content SEND: Content-Length: 37

{"jsonrpc":"2.0","id":93,"result":[]}

Variable was assigned an int (a = 1; a.m)

b = 1; b.to_s

D, [2018-03-03 11:04:40 -05:00 #9095] DEBUG -- : Content-Length: 172
D, [2018-03-03 11:04:40 -05:00 #9095] DEBUG -- :
D, [2018-03-03 11:04:40 -05:00 #9095] DEBUG -- : {"jsonrpc":"2.0","id":115,"method":"textDocument/completion","params":{"textDocument":{"uri":"file:///home/main/Projects/Foo/foo.cr"},"position":{"line":18,"character":9}}}
D, [2018-03-03 11:04:40 -05:00 #9095] DEBUG -- : textDocument/completion
D, [2018-03-03 11:04:40 -05:00 #9095] DEBUG -- : Couldn't find type Int32
D, [2018-03-03 11:04:40 -05:00 #9095] DEBUG -- : Scry::ResponseMessage(@jsonrpc="2.0", @id=115, @result=[], @error=nil)
D, [2018-03-03 11:04:40 -05:00 #9095] DEBUG -- : Content SEND: Content-Length: 38

{"jsonrpc":"2.0","id":115,"result":[]}

The var's type is declared in the method definition (ex: def(a: File); a.me)

def foo(c : File)
  c
end

foo(File.open("./foo.cr"))

D, [2018-03-03 11:11:05 -05:00 #9095] DEBUG -- : Content-Length: 171
D, [2018-03-03 11:11:05 -05:00 #9095] DEBUG -- : 
D, [2018-03-03 11:11:05 -05:00 #9095] DEBUG -- : {"jsonrpc":"2.0","id":184,"method":"textDocument/completion","params":{"textDocument":{"uri":"file:///home/main/Projects/Foo/foo.cr"},"position":{"line":1,"character":4}}}
D, [2018-03-03 11:11:05 -05:00 #9095] DEBUG -- : textDocument/completion
D, [2018-03-03 11:11:05 -05:00 #9095] DEBUG -- : Couldn't find type File
D, [2018-03-03 11:11:05 -05:00 #9095] DEBUG -- : Scry::ResponseMessage(@jsonrpc="2.0", @id=184, @result=[], @error=nil)
D, [2018-03-03 11:11:05 -05:00 #9095] DEBUG -- : Content SEND: Content-Length: 38

{"jsonrpc":"2.0","id":184,"result":[]}

The object where its called is a class. (ex: File.exp)

File.exists?("./foo.cr")

D, [2018-03-03 11:11:47 -05:00 #9095] DEBUG -- : Content-Length: 172
D, [2018-03-03 11:11:47 -05:00 #9095] DEBUG -- : 
D, [2018-03-03 11:11:47 -05:00 #9095] DEBUG -- : {"jsonrpc":"2.0","id":192,"method":"textDocument/completion","params":{"textDocument":{"uri":"file:///home/main/Projects/Foo/foo.cr"},"position":{"line":59,"character":6}}}
D, [2018-03-03 11:11:47 -05:00 #9095] DEBUG -- : textDocument/completion
D, [2018-03-03 11:11:47 -05:00 #9095] DEBUG -- : Couldn't find type File.class
D, [2018-03-03 11:11:47 -05:00 #9095] DEBUG -- : Scry::ResponseMessage(@jsonrpc="2.0", @id=192, @result=[], @error=nil)
D, [2018-03-03 11:11:47 -05:00 #9095] DEBUG -- : Content SEND: Content-Length: 38

{"jsonrpc":"2.0","id":192,"result":[]}

The method is defined as a property/getter/setter

class Foo
  property bar : Int32?
end

baz = Foo.new

baz.bar

D, [2018-03-03 11:13:50 -05:00 #9095] DEBUG -- : Content-Length: 172
D, [2018-03-03 11:13:50 -05:00 #9095] DEBUG -- : 
D, [2018-03-03 11:13:50 -05:00 #9095] DEBUG -- : {"jsonrpc":"2.0","id":215,"method":"textDocument/completion","params":{"textDocument":{"uri":"file:///home/main/Projects/Foo/foo.cr"},"position":{"line":81,"character":6}}}
D, [2018-03-03 11:13:50 -05:00 #9095] DEBUG -- : textDocument/completion
D, [2018-03-03 11:13:50 -05:00 #9095] DEBUG -- : Couldn't find type Foo
D, [2018-03-03 11:13:50 -05:00 #9095] DEBUG -- : Scry::ResponseMessage(@jsonrpc="2.0", @id=215, @result=[], @error=nil)
D, [2018-03-03 11:13:50 -05:00 #9095] DEBUG -- : Content SEND: Content-Length: 38

{"jsonrpc":"2.0","id":215,"result":[]}

Full scry logs here

BTW, I see you're using four spaces to indent crystal code

Use two spaces to indent code inside namespaces, methods, blocks or other nested contexts

Crystal code guidelines says 2 spaces 😅

Try using crystal tool format in your code 👍

faustinoaq · 2018-03-03T16:38:51Z

I tried to setup CRYSTAL_PATH as a global env using /etc/profile but still no completion and same debug output.

BTW, require completion works like a charm ❤️

laginha87 · 2018-03-03T17:28:29Z

@faustinoaq Thanks for trying this out.
I'm really sorry it didn't work, don't want to waste your time.
I tried it locally using a single file and it works for me.
I added some logging to the class that preloads the completion data.
Would you mind trying it out and sending me the logs to figure out what's going on?

laginha87 · 2018-03-03T17:29:07Z

About the CRYSTAL_PATH it should be set up automatically by scry.

faustinoaq · 2018-03-03T21:14:59Z

I tried it locally using a single file and it works for me.

@laginha87 Thank you for all your effort! 👍

I tried creating a new crystal project using crystal init app and looks like completion is working now 🎉

Seems that src folder did the trick 😎

Would you mind trying it out and sending me the logs to figure out what's going on?

Here is my latest debug logs (testing this completion feature)

I approve this PR 😄 , although I think more people should try it.

BTW, Scry completion is not working on sublime, I don't know why, sublime has other issues with scry though

Maybe someone can try Atom (@keplersj) or NVIM (@bew) 👀 😅

faustinoaq · 2018-03-03T21:34:27Z

This is awesome! ✨

Very useful to get some module or class method in standard library 💯

faustinoaq

Very nice PR, Just waiting for more reviews 👍

faustinoaq · 2018-03-05T19:43:23Z

BTW, Scry completion is not working on sublime, I don't know why, sublime has other issues with scry though

I did it! Scry completion is working in sublime as well! 🎉 ✨

~~I had to tweak LSP sublime client with some config copied from scry logs:~~

// Settings in here override those in "LSP/LSP.sublime-settings",

{
  "clients":
  {
    // Each new client must have the following structure.
    "crystal":
    {
      
  
      // The command line required to run the server.
      "command": ["/home/main/Projects/scry/scry"],
  
      // Use: "Show Scope Name" from Sublime's Developer menu
      "scopes": ["source.crystal", "source.cr"],
  
      // Run: view.settings().get("syntax") in console
      "syntaxes": ["Packages/Crystal/Crystal.tmLanguage"],
  
      // See: https://github.com/Microsoft/language-server-protocol/issues/213
      "languageId": "crystal",

      "only_show_lsp_completions": true,

      "log_payloads": true,

      "show_diagnostics_phantoms": true,
      
  
      // Sent to server once using workspace/didConfigurationChange notification
      "settings": {
        "problems":"syntax",
        "implementations":false,
        "hover":false,
        "completion":false,
        "mainFile":"",
        "maxNumberOfProblems":100,
        "processesLimit":3,
        "compiler":"crystal",
        "server":"/home/main/Projects/scry/scry",
        "logLevel":"debug",
        "bashOnWindows":false
       },
  
      // Sent once to server in initialize request
      "initializationOptions": {
        "capabilities": {
          "workspace": {
            "applyEdit": true
          },
          "textDocument": {
            "synchronization": {
              "didSave": true
            },
            "completion": {
              "completionItem": {
                "snippetSupport": true
              }
            }
          }
        }
      }
    }
  }
}

faustinoaq · 2018-03-06T00:38:26Z

@keplersj @bmulvihill Can you review this?, I would like to get it merged 😅

bmulvihill · 2018-03-06T01:06:20Z

@faustinoaq @laginha87 This is a big PR. One thing sticks out to me, a lot of the code here is building up a dependency graph

Is that necessary? Are we able to just use some sort of main file or root path for the project? From there we can get all the .cr files and parse them.

It seems like maybe that is a simpler approach. Although I don't have all the context regarding this PR, and maybe that won't work for this.

faustinoaq · 2018-03-06T01:26:44Z

Hi @bmulvihill 😄

I think @laginha87 is trying to implement auto completion based on cracker by @TechMagister

Cracker is using a similar mechanism, I don't know if is the best way, though

bmulvihill · 2018-03-06T02:59:30Z

@faustinoaq I took a look at cracker, I didn't notice any use of dependency graphs, unless I missed something.

laginha87 · 2018-03-06T12:25:47Z

@bmulvihill you're right the dependancy graph stuff isn't in cracker. In cracker they only define a preload method and your integration with the editor handles what to preload.

I added the dependancy graph cause I didn't like that the autocomplete suggestions could be inaccurate by providing completion hints for files that are not required by the file you're working on.

I understand that this would be picked up by the compiler but since you can reopen classes it can lead to some strange behaviour the compiler just tells the method doesn't exist on the class you know you're requiring correctly.

I was also worried with the memory footprint specially if you're in a project with a lot of libraries.

The dependancy graph might have been a premature optimization and it can be handled later.

If we just preload the files from the main root file we lose completion for files with no workspace and for any helper methods defined in the specs.

laginha87 · 2018-03-06T12:29:14Z

@bmulvihill I can try it out without the graph to shorten the PR and it can be added later if it seems like a reasonable approach.

I have done a lot more work around completion in scry so I split the work in smaller chunks so they'd be easier to review.

bmulvihill · 2018-03-06T14:20:50Z

@laginha87 Thank you for the explanation, like I said I definitely don't have all the context, I am just curious about the approach.

Would some of the problems mentioned be solved by always updating the method DB every time we update a file? What I mean is we create the method DB when opening the project based on some root path/main file and then make updates to it as we update files via the DidChangeTextDocument notification. Or am I misunderstanding the issue?

I was thinking whatever approach is used here eventually could also be used to solve Workspace symbols as well.

keplersj

Looks okay to me. Looks like a good implementation. And of course once it's out there we can dogfood it and having an implementation like this starts making life easier

laginha87 · 2018-03-06T19:02:23Z

@bmulvihill those are all valid points.
I think if we preload everything the method db should be a more complex structure because there will be a lot more stuff to search through, also updating the method db on a file change might involve some complex logic as well.
Those can all be managed anyway, but the major issue I have with an overall methodDB is because of scenarios like:
if you have this 2 files.

# a/a.cr

class A
    def a
    end
end

# b/a.cr

class A
    def b
    end
end

and you're editing this:

require "a/a.cr"

foo = A.new
foo.

The completion will suggest both a and b which is incorrect as it should only suggest the a method.

And also thought this approach would be better to later store all this info in the filesystems for a faster boot up.

Right now the solution isn't very dependant on the graph and I can easily remove it and we can add it later if there's an actual need for it.

bmulvihill · 2018-03-06T21:05:12Z

@laginha87 Ok cool, thanks again for explaining. I think it is good the way it is 👍

faustinoaq · 2018-03-07T00:39:11Z

Seems this PR is enough approved, I'll merge it

Please if you found some problem trying these new features open a new issue or a PR fixing it.

Thank you so much for these contributions 😄 👏 ✨

faustinoaq · 2018-03-07T15:54:52Z

I had to tweak LSP sublime client with some config copied from scry logs:

Looks like those settings aren't needed at all

😅

laginha87 added 7 commits February 28, 2018 19:06

Add dependency graph to link requires between crystal files

490467a

Load dependency graph on workspace open, fix typos on dependency grap…

3ccb55e

…h name

Add the method db just with simple usecases by classes, doesn't handl…

b565910

…e modules

Ignore private and toplevel methods

83da3d3

Hook method db generation on file open

b92f451

Fix method completion for properties

6cac66a

Remove mistaken property

f1dabea

faustinoaq requested a review from a team March 1, 2018 23:05

Preload everything in workspace not just src/

912e557

laginha87 added 2 commits March 3, 2018 16:52

Format the code

3be3a53

Adds logging messages to the dependancy graph

f494945

faustinoaq approved these changes Mar 4, 2018

View reviewed changes

keplersj approved these changes Mar 6, 2018

View reviewed changes

faustinoaq merged commit c48f421 into master Mar 7, 2018

faustinoaq deleted the 28-add-basic-autocompletion branch March 7, 2018 00:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

28 add basic autocompletion #51

28 add basic autocompletion #51

laginha87 commented Mar 1, 2018 •

edited

Loading

laginha87 commented Mar 1, 2018

faustinoaq commented Mar 2, 2018

faustinoaq commented Mar 2, 2018

laginha87 commented Mar 2, 2018 •

edited

Loading

faustinoaq commented Mar 2, 2018

laginha87 commented Mar 3, 2018

faustinoaq commented Mar 3, 2018 •

edited

Loading

faustinoaq commented Mar 3, 2018

laginha87 commented Mar 3, 2018

laginha87 commented Mar 3, 2018

faustinoaq commented Mar 3, 2018

faustinoaq commented Mar 3, 2018

faustinoaq left a comment

faustinoaq commented Mar 5, 2018 •

edited

Loading

faustinoaq commented Mar 6, 2018

bmulvihill commented Mar 6, 2018 •

edited

Loading

faustinoaq commented Mar 6, 2018

bmulvihill commented Mar 6, 2018

laginha87 commented Mar 6, 2018 •

edited

Loading

laginha87 commented Mar 6, 2018

bmulvihill commented Mar 6, 2018 •

edited

Loading

keplersj left a comment

laginha87 commented Mar 6, 2018

bmulvihill commented Mar 6, 2018

faustinoaq commented Mar 7, 2018

faustinoaq commented Mar 7, 2018

28 add basic autocompletion #51

28 add basic autocompletion #51

Conversation

laginha87 commented Mar 1, 2018 • edited Loading

laginha87 commented Mar 1, 2018

faustinoaq commented Mar 2, 2018

faustinoaq commented Mar 2, 2018

laginha87 commented Mar 2, 2018 • edited Loading

faustinoaq commented Mar 2, 2018

laginha87 commented Mar 3, 2018

faustinoaq commented Mar 3, 2018 • edited Loading

Variable was assigned a boolean (a = true; a.m)

Variable was assigned an int (a = 1; a.m)

The var's type is declared in the method definition (ex: def(a: File); a.me)

The object where its called is a class. (ex: File.exp)

The method is defined as a property/getter/setter

faustinoaq commented Mar 3, 2018

laginha87 commented Mar 3, 2018

laginha87 commented Mar 3, 2018

faustinoaq commented Mar 3, 2018

faustinoaq commented Mar 3, 2018

faustinoaq left a comment

Choose a reason for hiding this comment

faustinoaq commented Mar 5, 2018 • edited Loading

faustinoaq commented Mar 6, 2018

bmulvihill commented Mar 6, 2018 • edited Loading

faustinoaq commented Mar 6, 2018

bmulvihill commented Mar 6, 2018

laginha87 commented Mar 6, 2018 • edited Loading

laginha87 commented Mar 6, 2018

bmulvihill commented Mar 6, 2018 • edited Loading

keplersj left a comment

Choose a reason for hiding this comment

laginha87 commented Mar 6, 2018

bmulvihill commented Mar 6, 2018

faustinoaq commented Mar 7, 2018

faustinoaq commented Mar 7, 2018

laginha87 commented Mar 1, 2018 •

edited

Loading

laginha87 commented Mar 2, 2018 •

edited

Loading

faustinoaq commented Mar 3, 2018 •

edited

Loading

faustinoaq commented Mar 5, 2018 •

edited

Loading

bmulvihill commented Mar 6, 2018 •

edited

Loading

laginha87 commented Mar 6, 2018 •

edited

Loading

bmulvihill commented Mar 6, 2018 •

edited

Loading