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: crash upon creating empty linked list #45510

Closed
soypat opened this issue Apr 11, 2021 · 15 comments
Closed

x/tools/gopls: crash upon creating empty linked list #45510

soypat opened this issue Apr 11, 2021 · 15 comments

Comments

@soypat
Copy link

@soypat soypat commented Apr 11, 2021

Here's a hard one for you.

What version of Gopls are you using ?

$ gopls  version
golang.org/x/tools/gopls v0.6.9
    golang.org/x/tools/gopls@v0.6.9 h1:LBBcE2y3Tb4bp79JVLWCQBbvYCFEs5ADGWsQDoSLk1Q=

What did you do?

Create new program from scratch, write:

type a *a
var x a = a{}

or

type a *a
var x a = x

What did you expect to see?

gopls ask me why I would want to use this crazy datastructure

What did you see instead?

gopls crashes

@AlexRouSg
Copy link
Contributor

@AlexRouSg AlexRouSg commented Apr 11, 2021

I am unable to get any indication that gopls has crashed with the given examples.
Please provide the full file (examples are missing the package line at least) and what indicates that gopls has crashed.

Does not crash with:

package main

type a *a
var x a = x

func main() {

}

Still does not crash with a invalid file containing exactly:

type a *a
var x a = x
@soypat
Copy link
Author

@soypat soypat commented Apr 11, 2021

It is not a gopls bug then. I am testing on vscode and this exact same program causes vscode to hang while "Getting actions from 'Go'"

@AlexRouSg
Copy link
Contributor

@AlexRouSg AlexRouSg commented Apr 12, 2021

I too am running vscode and got no such thing.

However what you are describing sounds a lot like vscode is still starting up, when you do a fresh launch of vscode it is known to sometimes take a while to startup all the plugins and related tools so if you do a edit and save too quickly then it has to wait for all of that to finish loading.

Try waiting a few minutes after oppening vscode and see if it still hangs.

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Apr 12, 2021

@soypat: Can you please share your Go version? This type of bug may have been fixed with later versions of Go.

@stamblerre stamblerre changed the title gopls: crash upon creating empty linked list x/tools/gopls: crash upon creating empty linked list Apr 12, 2021
@gopherbot gopherbot added this to the Unreleased milestone Apr 12, 2021
@heschi heschi modified the milestones: Unreleased, gopls/unplanned Apr 12, 2021
@ShoshinNikita
Copy link

@ShoshinNikita ShoshinNikita commented Apr 12, 2021

I could reproduce this behavior:

package main

type a *a

var x a = <> // try to complete

func main() {}

gopls doesn't crash but stops responding. It is caused by a loop in golang.org/x/tools/internal/lsp/source.Deref():

func Deref(typ types.Type) types.Type {
	for {
		p, ok := typ.Underlying().(*types.Pointer)
		if !ok {
			return typ
		}
		// log.Printf("current typ:            %#v\n", typ)
		// log.Printf("new typ (pointer elem): %#v\n", p.Elem())
		typ = p.Elem()
	}
}

typ is equal to typ.Underlying().(*types.Pointer).Elem():

current typ:            &types.Named{info:0x2, obj:(*types.TypeName)(0xc005e04dc0), orig:(*types.Pointer)(0xc00559db60), underlying:(*types.Pointer)(0xc00559db60), methods:[]*types.Func(nil)}
new typ (pointer elem): &types.Named{info:0x2, obj:(*types.TypeName)(0xc005e04dc0), orig:(*types.Pointer)(0xc00559db60), underlying:(*types.Pointer)(0xc00559db60), methods:[]*types.Func(nil)}

So, this loop is infinite.

Stack trace
golang.org/x/tools/internal/lsp/source.Deref()
  github.com/golang/tools/internal/lsp/source/util.go:233

golang.org/x/tools/internal/lsp/source/completion.(*completer).lexical()
  github.com/golang/tools/internal/lsp/source/completion/completion.go:1352

golang.org/x/tools/internal/lsp/source/completion.(*completer).collectCompletions()
  github.com/golang/tools/internal/lsp/source/completion/completion.go:643

golang.org/x/tools/internal/lsp/source/completion.Completion()
  github.com/golang/tools/internal/lsp/source/completion/completion.go:554 +0xe56
@AlexRouSg
Copy link
Contributor

@AlexRouSg AlexRouSg commented Apr 12, 2021

Ok I can reproduce this now but the steps to reproduce wasn't clear.

First create a file and fill it with and save:

package main

type a *a

var x a = <> // try to complete

func main() {}

Next edit this line and save again:
var x a = nil

@soypat
Copy link
Author

@soypat soypat commented Apr 12, 2021

Sorry. I'll make sure steps are clearer next time. Did not think saving the file was crucial part of steps

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Apr 12, 2021

Thanks for figuring out the cause of this bug, @ShoshinNikita! Do you want to send the CL to fix it or would you rather a member of the gopls team take it on?

@ShoshinNikita
Copy link

@ShoshinNikita ShoshinNikita commented Apr 13, 2021

I would be happy to send a CL but I am not sure about the fix. Should we just return the current typ in such situations?

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Apr 13, 2021

Yeah I think that would work.

ShoshinNikita added a commit to ShoshinNikita/tools that referenced this issue Apr 14, 2021
Return a pointer type if the type refers to itself (for example, type a *a).

Fixes golang/go#45510
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 14, 2021

Change https://golang.org/cl/310050 mentions this issue: internal/lsp/source: fix an infinite loop in Deref function

@muirdm
Copy link

@muirdm muirdm commented Apr 15, 2021

I don't think this fix is sufficient for cyclic types like:

type (
	foo *bar
	bar *foo
)
@ShoshinNikita
Copy link

@ShoshinNikita ShoshinNikita commented Apr 15, 2021

Yeah, the current fix doesn't work for such cyclic types. I think we can use map[types.Type]struct{} with all previous underlying types to detect cycles.

@muirdm
Copy link

@muirdm muirdm commented Apr 15, 2021

One slight optimization is that you only have to worry about named pointer types (i.e. you only have to allocate/insert into your map when you see a named pointer type).

ShoshinNikita added a commit to ShoshinNikita/tools that referenced this issue Apr 16, 2021
The previous fix (d1362d7) is not sufficient for all cyclic types.
This change updates Deref function to support more complex cases.
We use a map with underlying types to detect cycles.

Fixes golang/go#45510
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 16, 2021

Change https://golang.org/cl/310311 mentions this issue: internal/lsp/source: fix Deref function for cyclic types

gopherbot pushed a commit to golang/tools that referenced this issue Apr 16, 2021
The previous fix (d1362d7) is not sufficient for all cyclic types.
This change updates Deref function to support more complex cases.
We use a map with underlying types to detect cycles.

Fixes golang/go#45510

Change-Id: I28f655a9c1d4f363cb7ae3f47db3e8567fe6e80a
GitHub-Last-Rev: 4c89874
GitHub-Pull-Request: #305
Reviewed-on: https://go-review.googlesource.com/c/tools/+/310311
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Trust: Rebecca Stambler <rstambler@golang.org>
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants