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.

Sign up for GitHub

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

Jump to bottom

x/tools/gopls: nil Type crash in staticcheck #54762

Closed
adonovan opened this issue Aug 29, 2022 · 11 comments
Closed

x/tools/gopls: nil Type crash in staticcheck #54762

adonovan opened this issue Aug 29, 2022 · 11 comments
Assignees
@adonovan
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
gopls/v0.11.0

Comments

@adonovan
Copy link
Member

This crash was observed on one builder run when the panic/recover was removed from analysis in gopls:

panic: nil type [recovered]
	panic: /workdir/gopath/src/golang.org/x/tools/internal/lsp/bug/bug.go:56: analysis SA4016 for package golang.org/x/tools/internal/lsp/rename/generics panicked: nil type

goroutine 93005 [running]:
golang.org/x/tools/internal/lsp/bug.Report({0xc009028900, 0x5e}, 0x0)
	/workdir/gopath/src/golang.org/x/tools/internal/lsp/bug/bug.go:71 +0x594
golang.org/x/tools/internal/lsp/bug.Errorf({0x1488298, 0x27}, {0xc00cdc6d38, 0x3, 0x3})
	/workdir/gopath/src/golang.org/x/tools/internal/lsp/bug/bug.go:56 +0x73
golang.org/x/tools/internal/lsp/cache.runAnalysis.func8.1()
	/workdir/gopath/src/golang.org/x/tools/internal/lsp/cache/analysis.go:368 +0x165
panic({0x12f23a0, 0x17aee00})
	/workdir/go/src/runtime/panic.go:890 +0x262
golang.org/x/exp/typeparams.computeTermSetInternal({0x0, 0x0}, 0x1385b00?, 0x0)
	/workdir/gopath/pkg/mod/golang.org/x/exp/[email protected]/normalize.go:111 +0x1058
golang.org/x/exp/typeparams.NormalTerms({0x0?, 0x0})
	/workdir/gopath/pkg/mod/golang.org/x/exp/[email protected]/normalize.go:78 +0x16e
honnef.co/go/tools/go/types/typeutil.NewTypeSet({0x0, 0x0})
	/workdir/gopath/pkg/mod/honnef.co/go/[email protected]/go/types/typeutil/typeparams.go:16 +0x3b
honnef.co/go/tools/go/types/typeutil.All({0x0, 0x0}, 0xc0021c1ce0?)
	/workdir/gopath/pkg/mod/honnef.co/go/[email protected]/go/types/typeutil/typeparams.go:99 +0x3c
honnef.co/go/tools/staticcheck.CheckSillyBitwiseOps.func1({0x17b3b48?, 0xc0021c1ce0})
	/workdir/gopath/pkg/mod/honnef.co/go/[email protected]/staticcheck/lint.go:2902 +0xc5
golang.org/x/tools/go/ast/inspector.(*Inspector).Preorder(0xc00a1fe600, {0xc00243d5b0?, 0x1, 0x431a70?}, 0xc00cdc75c0)
	/workdir/gopath/src/golang.org/x/tools/go/ast/inspector/inspector.go:77 +0x103
honnef.co/go/tools/analysis/code.Preorder(...)
	/workdir/gopath/pkg/mod/honnef.co/go/[email protected]/analysis/code/visit.go:16
honnef.co/go/tools/staticcheck.CheckSillyBitwiseOps(0xc008345110)
	/workdir/gopath/pkg/mod/honnef.co/go/[email protected]/staticcheck/lint.go:2966 +0xe5
golang.org/x/tools/internal/lsp/cache.runAnalysis.func8(0xc008b3e9a0, 0xc000487a00, 0xc001c5d440, 0xc008345110)
	/workdir/gopath/src/golang.org/x/tools/internal/lsp/cache/analysis.go:371 +0x117
FAIL	golang.org/x/tools/gopls/test	47.513s

The apparent cause is TypeOf(BinaryExpr) returning nil, which is then passed to NewTypeSet. The BinaryExpr in question is a "|" union operator over type constraints. Questions:

  • why isn't this deterministic? Is it a race against process exit, hence it only shows up on slower builders?
  • Is it a go/types bug? The analysis has RunDespiteErrors=false, so every expression should have a type.
@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Aug 29, 2022
@gopherbot gopherbot added this to the Unreleased milestone Aug 29, 2022
@adonovan
Copy link
Member Author

adonovan commented Aug 29, 2022 •
edited
Loading

Another crash (also in staticcheck) reported by another single builder run:

panic: /workdir/gopath/src/golang.org/x/tools/internal/lsp/bug/bug.go:56: analysis SA1019 for package golang.org/x/tools/internal/lsp/rename/issue42134 panicked: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x38 pc=0xb11e1e]

goroutine 17408 [running]:
golang.org/x/tools/internal/lsp/bug.Report({0xc0098d4640, 0x98}, 0x0)
	/workdir/gopath/src/golang.org/x/tools/internal/lsp/bug/bug.go:71 +0x445
golang.org/x/tools/internal/lsp/bug.Errorf({0xf1dc31?, 0x31?}, {0xc001a98560?, 0xc001dd9ce0?, 0xc001a985c0?})
	/workdir/gopath/src/golang.org/x/tools/internal/lsp/bug/bug.go:56 +0x3f
golang.org/x/tools/internal/lsp/cache.runAnalysis.func8.1()
	/workdir/gopath/src/golang.org/x/tools/internal/lsp/cache/analysis.go:368 +0x10e
panic({0xdd7ec0, 0x16c80f0})
	/workdir/go/src/runtime/panic.go:884 +0x212
honnef.co/go/tools/staticcheck.CheckDeprecated.func3({0x11714b8?, 0xc001b4e2b8?}, 0x14?)
	/workdir/gopath/pkg/mod/honnef.co/go/[email protected]/staticcheck/lint.go:3178 +0x23e
golang.org/x/tools/go/ast/inspector.(*Inspector).Nodes(0xc004b61290, {0x0?, 0x16d4f60?, 0x203001?}, 0xc009d5f900)
	/workdir/gopath/src/golang.org/x/tools/go/ast/inspector/inspector.go:100 +0xad
honnef.co/go/tools/staticcheck.CheckDeprecated(0xc007ebf5f0)
	/workdir/gopath/pkg/mod/honnef.co/go/[email protected]/staticcheck/lint.go:3214 +0x1b1
golang.org/x/tools/internal/lsp/cache.runAnalysis.func8(0xdf0000?, 0xc007ebf5f0?, 0xc009874550, 0x510666?)
	/workdir/gopath/src/golang.org/x/tools/internal/lsp/cache/analysis.go:371 +0x76
golang.org/x/tools/internal/lsp/cache.runAnalysis({0x1175518, 0xc0027bbdc0}, 0xc003d76280, 0xc000152ea0, 0xc00ae68240, 0x5104d4?)
	/workdir/gopath/src/golang.org/x/tools/internal/lsp/cache/analysis.go:372 +0x888
golang.org/x/tools/internal/lsp/cache.(*snapshot).actionHandle.func1({0x1175518, 0xc0027bbdc0}, {0xed0a40?, 0xc003d76280})
	/workdir/gopath/src/golang.org/x/tools/internal/lsp/cache/analysis.go:168 +0xf0
golang.org/x/tools/internal/memoize.(*Promise).run.func2.1()
	/workdir/gopath/src/golang.org/x/tools/internal/memoize/memoize.go:187 +0xa9
runtime/trace.WithRegion({0x1175518?, 0xc0027bbdc0?}, {0xc00325c030, 0x21}, 0xc001a98f90)
	/workdir/go/src/runtime/trace/annotation.go:141 +0xe3
golang.org/x/tools/internal/memoize.(*Promise).run.func2()
	/workdir/gopath/src/golang.org/x/tools/internal/memoize/memoize.go:180 +0x145
created by golang.org/x/tools/internal/memoize.(*Promise).run
	/workdir/gopath/src/golang.org/x/tools/internal/memoize/memoize.go:179 +0x1ea
FAIL	golang.org/x/tools/gopls/test	6.121s

Apparently a nil ObjectOf on a SelectorExpr.Sel. None of these look related to the new generics logic.

@adonovan
Copy link
Member Author

adonovan commented Aug 31, 2022 •
edited
Loading

Re-running the tests shows a great variety of crashes. https://source.cloud.google.com/results/invocations/48b434a2-c9e7-46a9-acde-57b5fc976081/targets/golang%2Ftools%2Fgopls-legacy%2Fpresubmit-116/log contains more evidence that this is a general race condition between two gopls subsystems: an analysis pass has a nil entry in the map of Results of its predecessor pass, Inspector.

panic: interface conversion: interface {} is nil, not *inspector.Inspector

goroutine 65570 [running]:
golang.org/x/tools/go/analysis/passes/structtag.run(0xc00d07f6c0, 0x3, 0xd357a0, 0xc00bc6f701, 0xc00d0b0ef0)
	/src/tools/go/analysis/passes/structtag/structtag.go:37 +0xfe
golang.org/x/tools/internal/lsp/cache.runAnalysis.func8(0xc00d07f6c0, 0xc00bc6f768)
	/src/tools/internal/lsp/cache/analysis.go:376 +0x52
golang.org/x/tools/internal/lsp/cache.runAnalysis(0x1039298, 0xc00d128b40, 0xc0023788c0, 0x149ff00, 0xc008475440, 0xc00d10f680, 0xc00d10f680)
	/src/tools/internal/lsp/cache/analysis.go:377 +0xa56
golang.org/x/tools/internal/lsp/cache.(*snapshot).actionHandle.func1(0x1039298, 0xc00d128b40, 0xe0da60, 0xc0023788c0, 0xc00c133640, 0xc0083d3658)
	/src/tools/internal/lsp/cache/analysis.go:168 +0x159
golang.org/x/tools/internal/memoize.(*Promise).run.func2.1()
	/src/tools/internal/memoize/memoize.go:187 +0xd7
runtime/trace.WithRegion(0x1039298, 0xc00d128b40, 0xc00d122540, 0x21, 0xc0083d3758)
	/usr/local/go/src/runtime/trace/annotation.go:141 +0xf8
golang.org/x/tools/internal/memoize.(*Promise).run.func2(0x1039298, 0xc00d128b40, 0xc00d05d6e0, 0xc00d0b0e80, 0xc00d10e960, 0xe0da60, 0xc0023788c0)
	/src/tools/internal/memoize/memoize.go:180 +0x14c
created by golang.org/x/tools/internal/memoize.(*Promise).run
	/src/tools/internal/memoize/memoize.go:179 +0x19a

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/426802 mentions this issue: internal/lsp/cache: delete misleading Package.IllTyped method

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/426803 mentions this issue: internal/lsp/cache: honor RunDespiteErrors=false

gopherbot pushed a commit to golang/tools that referenced this issue Aug 31, 2022
@adonovan
internal/lsp/cache: delete misleading Package.IllTyped method
3eb8a8f 
This method does not in fact report whether the package is ill-typed.
It actually reports whether any of three fields of pkg are nil.
They never are.
This does rather explain why type-error analyzers are executed
on (genuinely) ill-typed packages, despite the !IllTyped()
condition. And why analyzers that aren't prepared to handle
ill-typed code (!RunDespiteErrors) sometimes crash in production.

Updates golang/go#54762

Change-Id: I95421584bec68fb849b5ed52cc4e6d9b6bb679be
Reviewed-on: https://go-review.googlesource.com/c/tools/+/426802
Auto-Submit: Alan Donovan <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Robert Findley <[email protected]>
gopls-CI: kokoro <[email protected]>
Run-TryBot: Alan Donovan <[email protected]>
gopherbot pushed a commit to golang/tools that referenced this issue Sep 1, 2022
@adonovan
internal/lsp/cache: honor RunDespiteErrors=false
6c10975 
This change prevents analysis from running on a package containing
any kind of error unless the analyzer has indicated that it is robust.
This is presumed to be the cause of several panics in production.

And a test.

Updates golang/go#54762

Change-Id: I9327e18ef8d7773c943ea45fc786991188358131
Reviewed-on: https://go-review.googlesource.com/c/tools/+/426803
Run-TryBot: Alan Donovan <[email protected]>
Auto-Submit: Alan Donovan <[email protected]>
gopls-CI: kokoro <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Robert Findley <[email protected]>
@jamalc jamalc modified the milestones: Unreleased, gopls/v0.10.0 Sep 1, 2022
@findleyr
Copy link
Member

findleyr commented Sep 9, 2022

Moving this to gopls/v0.10.1; I don't think we need to remove our panic recovery just yet.

@findleyr findleyr modified the milestones: gopls/v0.10.0, gopls/v0.10.1 Sep 9, 2022
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/426018 mentions this issue: internal/lsp/cache: report analysis panics during testing

gopherbot pushed a commit to golang/tools that referenced this issue Sep 29, 2022
@adonovan
internal/lsp/cache: report analysis panics during testing
d49f960 
Report a bug when an analysis dependency is found to be
neither "vertical" (for facts) or "horizontal" (for results).
This has been observed in practise: the lookup from a package
ID to a package object is not consistent.

The bug report is suppressed for command-line-arguments
packages to see whether it occurs on ordinary packages too.

Also, add disabled logic to disable recovery and dump all
goroutines, for temporary use when debugging.

Unrelated things found during debugging:
- simplify package key used in analysis cache: the mode is
  always the same constant.
- move TypeErrorAnalyzers logic closer to where needed.

Updates golang/go#54655
Updates golang/go#54762

Change-Id: I0c2afd9ddd4fcc21748a03f8fa0769a2de09a56f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/426018
Reviewed-by: Robert Findley <[email protected]>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/439116 mentions this issue: gopls/internal/lsp/cache: in tests, show all stacks during analyzer crash

gopherbot pushed a commit to golang/tools that referenced this issue Oct 5, 2022
@adonovan
gopls/internal/lsp/cache: in tests, show all stacks during analyzer c…
2d32e15 
…rash

We recently started using bug.Errorf to report analyzer crashes,
causing tests to fail sporadically. Unfortunately the log included
only the "panicked" message but not the relevant stack of the panic
or of the other goroutines, giving limited evidence on which to
investigate the underlying cause, suspected to be in package loading,
not the analyzer itself.

This change causes such crashes to display all stacks so that we
can gather more evidence over the next few days before we suppress
the crashes again.

Updates golang/go#54762
Updates golang/go#56035

Change-Id: I2d29e05b5910540918816e695b458463a05f94d9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/439116
Reviewed-by: Bryan Mills <[email protected]>
Run-TryBot: Alan Donovan <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Robert Findley <[email protected]>
gopls-CI: kokoro <[email protected]>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/440178 mentions this issue: gopls/internal/lsp/cache: suppress crashes in fact_purity analyzer

gopherbot pushed a commit to golang/tools that referenced this issue Oct 7, 2022
@adonovan
gopls/internal/lsp/cache: suppress crashes in fact_purity analyzer
709f108 
It has a known bug, and the purpose of the PanicOnBugs is to
find unknown bugs.

Updates golang/go#54762
Updates golang/go#56035

Change-Id: Id9fdfff478ef52b1d864acee366991808baeb574
Reviewed-on: https://go-review.googlesource.com/c/tools/+/440178
Run-TryBot: Alan Donovan <[email protected]>
Reviewed-by: Robert Findley <[email protected]>
gopls-CI: kokoro <[email protected]>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/440179 mentions this issue: gopls/internal/lsp/cache: suppress panics during analysis

gopherbot pushed a commit to golang/tools that referenced this issue Oct 7, 2022
@adonovan @gopherbot
gopls/internal/lsp/cache: suppress panics during analysis
8b1d1ec 
We now have a plausible model for the cause of the analysis
crashes and are disabling this check until we have submitted
a plausible fix.

(Specifically: actionHandle.promise may be a stale promise
for an older package with the same ID, since snapshot.actions
uses the package ID as a key. This causes us to re-use
stale inspectors, whose syntax nodes won't be found in TypesInfo.)

Updates golang/go#54762
Updates golang/go#56035

Change-Id: I1a7cc1b02b8c322692b1a6bee03f6cd858c08ea0
Reviewed-on: https://go-review.googlesource.com/c/tools/+/440179
TryBot-Result: Gopher Robot <[email protected]>
gopls-CI: kokoro <[email protected]>
Run-TryBot: Alan Donovan <[email protected]>
Reviewed-by: Bryan Mills <[email protected]>
Auto-Submit: Alan Donovan <[email protected]>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/420538 mentions this issue: internal/lsp/cache: invalidate analysis results on packages.Load

gopherbot pushed a commit to golang/tools that referenced this issue Oct 11, 2022
@findleyr
internal/lsp/cache: invalidate analysis results on packages.Load
cd0288f 
Most invalidation happens in snapshot.clone, but to be safe we were also
invalidating package data when new metadata is set via packages.Load. At
the time, I wasn't sure why this was necessary.

Now I understand: with experimentalUseInvalidMetadata it is possible
that we re-compute invalidated data using stale metadata in between the
initial clone and the subsequent reload. I noticed that it was also
possible to have stale analysis results after the Load results arrive.

Fix this by invalidating analysis results on Load, in addition to
packages. Factor out invalidation into a new helper method.

Since we believe this may fix analyzer panics, re-enable strict handling
of analysis panics during tests.

For golang/go#56035
For golang/go#54762
For golang/go#42857

Change-Id: I8c28e0700f8c16c58df4ecf60f6127b1c05d6dc5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/420538
Run-TryBot: Robert Findley <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
gopls-CI: kokoro <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>
@adonovan
Copy link
Member Author

We believe this issue was fixed by https://go.dev/cl/443100. Closing.

@findleyr findleyr modified the milestones: gopls/v0.10.1, gopls/v0.10.2 Nov 1, 2022
@golang golang locked and limited conversation to collaborators Nov 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@adonovan adonovan

Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
gopls/v0.11.0
Development

No branches or pull requests
4 participants
@adonovan @gopherbot @jamalc @findleyr