Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] import declaration autocomplete #332

Merged
merged 2 commits into from
Mar 9, 2016
Merged

[WIP] import declaration autocomplete #332

merged 2 commits into from
Mar 9, 2016

Conversation

nhooyr
Copy link
Contributor

@nhooyr nhooyr commented Mar 8, 2016

This closes issue #257. Work in progress right now.

Detection of being inside an import statement is done. Here is a testing file.

package main

import "fmt"

import f "fmt"

import . "fmt"

import (
    "fmt"
)

import (
    "fmt"
    "log"
)

import (
    f "fmt"
    l "log"
)

import (
    . "fmt"
    l "log"
)

import (
    "fmt"; "log"
)

import (
    . "fmt"; l "log"
)

import _ "fmt"

import (
  "io/
)

I just need to take cc.partial and return all import paths that match.

@nhooyr
Copy link
Contributor Author

nhooyr commented Mar 8, 2016

Alright now it works, autocomplete is functioning, I'm gonna just polish it up.

@nhooyr
Copy link
Contributor Author

nhooyr commented Mar 8, 2016

The plugins will need an update as well. I'll also take a look at vendoring/gb.

edit: never mind plugins are fine, I was making a mistake.

@nsf
Copy link
Owner

nsf commented Mar 8, 2016

The plugins will need an update as well.

That's a really bad joke.

Aside from that I see some bugs, the import doesn't return the correct length of the partial in some cases. That's a second argument form func (c *auto_complete_context) apropos, it's very important because some editors rely on it. Vim and godit in particular. I'll see more into code and will comment it there.

@nsf
Copy link
Owner

nsf commented Mar 8, 2016

Okay, the problem with partial length seems to be the following.

If I do autocompletion here: import "encoding/j#son" (# is the cursor).

The returned length of the partial is: len("encoding/j"), but the candidate itself is "json". It's incorrect. You should return the full "encoding/json" candidate in that case or reduce the returned partial length to be len("j"). Again, length of partial is a second argument returned by apropos method, some editors rely on it.

@nhooyr
Copy link
Contributor Author

nhooyr commented Mar 8, 2016

Got it, i'll fix those issues. struct{} is indeed more efficient, thats why I used it.

@nhooyr
Copy link
Contributor Author

nhooyr commented Mar 8, 2016

@nsf why are get_candidates_from_decl and get_candidates_from_set methods of auto_complete_context. They don't use auto_complete_context. Should I make get_import_candidates a method as well then?

@nsf
Copy link
Owner

nsf commented Mar 8, 2016

Because it came out of my OOP damaged brain. Yes you can make it a method, or a function. Doesn't matter.

@nhooyr
Copy link
Contributor Author

nhooyr commented Mar 8, 2016

@nsf All I have left to do are completing vendoring and gb. or should that be in another PR? I'll work on the rest of it tomorrow, I gotta do some homework now.

And I'm still not sure about the trailing / on directories, it did seem useful.

When inside a import declaration, a new bool field decl_import in
cursor_context is set to true.
@nhooyr
Copy link
Contributor Author

nhooyr commented Mar 9, 2016

Yea after thinking about it a bit more, and using it, I think its worth adding the trailing /. its not very hard to get used to and its useful.

@nhooyr
Copy link
Contributor Author

nhooyr commented Mar 9, 2016

Ah screw it, i'm gonna just do it recursively, I agree with you, I think it'll be much better. LiteIDE actually does it recursively as well.

@nhooyr
Copy link
Contributor Author

nhooyr commented Mar 9, 2016

Alright, its recursive now. its not slow at all. i'm so confused now why I didn't like it when I first implemented it like this, sigh. Much better now...

Previous commit detected the import declaration. This one implements
returning import candidates.

This commit closes #257
@nsf
Copy link
Owner

nsf commented Mar 9, 2016

Okay, I'm merging it, I think it's in good enough shape to be merged in.

@nsf nsf merged commit 40385eb into nsf:master Mar 9, 2016
@nsf
Copy link
Owner

nsf commented Mar 9, 2016

It definitely works with godit and with vim out of the box. My own sublime text 3 plugin fails to handle it properly. Because it ignores returned partial length and just goes back to last word. Will see if I can fix it. Also in emacs company-mode doesn't work at all in string literals for some reason.

Oh, I see: https://github.com/nsf/gocode/blob/master/emacs-company/company-go.el#L191

I guess we'll have to remove that line maybe one day.

@nsf
Copy link
Owner

nsf commented Mar 9, 2016

@nhooyr Thanks for the contribution. It's a nice addition to gocode.

@dominikh
Copy link

dominikh commented Mar 9, 2016

It also seems to be somewhat crash-happy:

2016/03/09 20:11:06 Go project path: sandbox/foo
2016/03/09 20:11:06 Got autocompletion request for '/home/dominikh/prj/src/sandbox/foo/foo.go'
2016/03/09 20:11:06 Cursor at: 24
2016/03/09 20:11:06 -------------------------------------------------------
package main

import (
    #
)
2016/03/09 20:11:06 -------------------------------------------------------
2016/03/09 20:11:06 Error parsing input file (outer block):
2016/03/09 20:11:06  4:2: expected 'STRING', found ';'
panic: runtime error: slice bounds out of range
1(runtime.call32): /usr/lib/go/src/runtime/asm_amd64.s:472
2(runtime.gopanic): /usr/lib/go/src/runtime/panic.go:426
3(runtime.panicslice): /usr/lib/go/src/runtime/panic.go:21
4(main.collect_package_imports): /home/dominikh/prj/src/github.com/nsf/gocode/declcache.go:34
5(main.(*auto_complete_file).process_data): /home/dominikh/prj/src/github.com/nsf/gocode/autocompletefile.go:76
6(main.(*auto_complete_context).apropos): /home/dominikh/prj/src/github.com/nsf/gocode/autocompletecontext.go:293
7(main.server_auto_complete): /home/dominikh/prj/src/github.com/nsf/gocode/server.go:179
8(main.(*RPC).RPC_auto_complete): /home/dominikh/prj/src/github.com/nsf/gocode/rpc.go:26
9(runtime.call64): /usr/lib/go/src/runtime/asm_amd64.s:473
10(reflect.Value.call): /usr/lib/go/src/reflect/value.go:435
11(reflect.Value.Call): /usr/lib/go/src/reflect/value.go:303
12(net/rpc.(*service).call): /usr/lib/go/src/net/rpc/server.go:383
13(runtime.goexit): /usr/lib/go/src/runtime/asm_amd64.s:1998

@nsf
Copy link
Owner

nsf commented Mar 9, 2016

@dominikh Judging by the stack trace, it probably existed for years there.

@nsf
Copy link
Owner

nsf commented Mar 9, 2016

@dominikh fixed

@nhooyr
Copy link
Contributor Author

nhooyr commented Mar 9, 2016

@nsf you're welcome

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants