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

x/tools/gopls: completion doesn't suggest items following invalid code #31687

Closed
myitcv opened this issue Apr 25, 2019 · 11 comments
Closed

x/tools/gopls: completion doesn't suggest items following invalid code #31687

myitcv opened this issue Apr 25, 2019 · 11 comments
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.

Comments

@myitcv
Copy link
Member

myitcv commented Apr 25, 2019

What version of Go are you using (go version)?

$ go version
go version go1.12.3 linux/amd64
$ go list -m golang.org/x/tools
golang.org/x/tools v0.0.0-20190425001055-9e44c1c40307

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GOARCH="amd64"
GOBIN="/home/myitcv/gostuff/src/github.com/myitcv/govim/cmd/govim/.bin"
GOCACHE="/home/myitcv/.cache/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/myitcv/gostuff"
GOPROXY=""
GORACE=""
GOROOT="/home/myitcv/gos"
GOTMPDIR=""
GOTOOLDIR="/home/myitcv/gos/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/myitcv/gostuff/src/github.com/myitcv/govim/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build004347189=/tmp/go-build -gno-record-gcc-switches"

What did you do?

This is largely a "port" of mdempsky/gocode#22 to the gopls issue tracker where the issue remains.

// example 1
package main

func main() {
    Do
      ^
}

func DoIt() bool {
    return true
}

If completion is attempted at the position indicated by ^, completion successfully returns the single candidate:

DoIt()

Now consider:

// example 2
package main

func main() {
    if Do
         ^
}

func DoIt() bool {
    return true
}

This time we don't get any candidates.

What did you expect to see?

The single candidate DoIt() returned in example 2.

What did you see instead?

As above


cc @stamblerre @ianthehat

@myitcv myitcv added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. gopls Issues related to the Go language server, gopls. labels Apr 25, 2019
@gopherbot gopherbot added this to the Unreleased milestone Apr 25, 2019
@myitcv
Copy link
Member Author

myitcv commented Apr 29, 2019

Another instance:

package main

func main() {
	var i I
	i.
	  ^
}

type I interface {
	DoIt()
}

The completion request at the cursor position show returns no results.

@stamblerre - is it worth kicking off separate go/parser issues to try and get these issues fixed in parallel?

@stamblerre
Copy link
Contributor

They're both instances of the same issue actually. Basically when something fails to parse at the top of the file, it creates problems at the end, so anything below where you are will not be handled correctly.

@myitcv
Copy link
Member Author

myitcv commented Apr 29, 2019

Yes indeed; I suspect however these are separate cases in go/parser hence my reason for listing the various instances I find.

@stamblerre stamblerre changed the title x/tools/cmd/gopls: completion of items following invalid if statements x/tools/cmd/gopls: completion fails on items following invalid code May 21, 2019
@stamblerre stamblerre changed the title x/tools/cmd/gopls: completion fails on items following invalid code x/tools/cmd/gopls: completion doesn't suggest items following invalid code May 21, 2019
@stamblerre stamblerre changed the title x/tools/cmd/gopls: completion doesn't suggest items following invalid code x/tools/gopls: completion doesn't suggest items following invalid code Jul 2, 2019
@SteelPhase
Copy link

Is there anyway that gopls could fallback to parsing the file on disk in these cases to attempt to return useful autocomplete? Not sure if that's actually the best way to resolve this type of situation.

@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Sep 12, 2019
@myitcv
Copy link
Member Author

myitcv commented Oct 7, 2019

@muirrn as the completion guru, do you think this can be closed now? Or are you using it as a placeholder for future improvements?

@muirdm
Copy link

muirdm commented Oct 7, 2019

I think we can keep it open to track/consolidate this class of problems.

I'm not planning any work in this area but I think the general plan is to expand our AST fixes to include these and other common syntax errors. For example, workarounds for the above cases might look like:

  • Parse if Do\n as if Do {}\n
  • Parse i.\n as i._\n

@gopherbot
Copy link

Change https://golang.org/cl/216482 mentions this issue: internal/lsp/cache: improve completion when missing opening curly

@gopherbot
Copy link

Change https://golang.org/cl/216483 mentions this issue: internal/lsp/cache: improve completion in init statements

@stamblerre stamblerre modified the milestones: gopls unplanned, gopls completion Jan 29, 2020
gopherbot pushed a commit to golang/tools that referenced this issue Feb 13, 2020
In cases like:

func _() {
  if fo<>
}

func foo() {}

Completing at "<>" does not include "foo" because the missing "if"
opening curly brace renders the rest of the file unparseable. "foo"
doesn't exist in the AST, so as far as gopls is concerned it doesn't
exist at all.

To fix this, we detect when a block is missing opening "{" and we
insert stub "{}" to make things parse better. This is a different kind
of fix than our previous *ast.BadExpr handling because we must reparse
the file after tweaking the source. After reparsing we maintain the
original syntax error so the user sees consistent error messages. This
also avoids having the "{}" spring into existence when the user
formats the file.

Fixes golang/go#31907.
Updates golang/go#31687.

Change-Id: I95ba60a11f7dd23dc484c063b4fd7ad77daa4e08
Reviewed-on: https://go-review.googlesource.com/c/tools/+/216482
Run-TryBot: Muir Manders <muir@mnd.rs>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
gopherbot pushed a commit to golang/tools that referenced this issue Feb 13, 2020
In cases like:

  if i := foo<>

we get an *ast.BadExpr because the parser is expecting the condition
expression, but "i := foo" is not a valid expression. Now we move the
statement into the "init" field and add a dummy "cond" expression.

We also needed a slight tweak to our missing curly brace fix. Now we
insert an extra semicolon in cases like:

for i := 0; i < foo

yielding

for i := 0; i < foo;{}

The parser doesn't like having only two clauses in the three clause
"for" statement.

Updates golang/go#31687.

Change-Id: I12d51e0d8af03436741227753f8e71452a463b05
Reviewed-on: https://go-review.googlesource.com/c/tools/+/216483
Run-TryBot: Muir Manders <muir@mnd.rs>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/216484 mentions this issue: internal/lsp/cache: improve completion after dangling selector

gopherbot pushed a commit to golang/tools that referenced this issue Feb 13, 2020
Dangling selectors such as:

    func _() {
      x.
    }
    var x struct { i int }

tend to wreak havoc on the AST. In the above example you didn't used
to get completions because the declaration of "x" was missing from the
AST.

We now work around this issue by inserting a "_" into the source code
before parsing to make the selector valid:

    func _() {
      x._ // <-- insert "_" here
    }
    var x struct { i int }

This makes completion work as expected because the declaration of "x"
is present in the AST.

I had to change fixAST() to be called before fixSrc() because
otherwise this new workaround in fixSrc() breaks the "accidental
keyword" countermeasures in fixAST().

Fixes golang/go#31973.
Updates golang/go#31687.

Change-Id: Ia7ef6c045a9c71502d1b8b36f187ac9b8a85fe21
Reviewed-on: https://go-review.googlesource.com/c/tools/+/216484
Run-TryBot: Muir Manders <muir@mnd.rs>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@muirdm
Copy link

muirdm commented Feb 13, 2020

@myitcv can you test out master and see if it resolves your above issues satisfactorily?

@myitcv
Copy link
Member Author

myitcv commented Feb 14, 2020

Looks good to me! Thank you very much, @muirdm

@myitcv myitcv closed this as completed Feb 14, 2020
@golang golang locked and limited conversation to collaborators Feb 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

5 participants