internal/lsp/cache: fix leaking wg.Done() and <-ioLimit

[freeformz]

There were several returns in this function that could leak both the
wg.Done() and <-ioLimit. This refactors them into defers close to their
counterparts.

Additionally I refactored the function to favor early returns over
nested if statements.

Fixes golang/go#32368

[freeformz]

PS: This may fix golang/go#32368, but there may be other things going on elsewhere that also need to be addressed.

[gopherbot]

This PR (HEAD: c4c8686) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/tools/+/179878 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

[gopherbot]

Message from Rebecca Stambler:

Patch Set 1: Run-TryBot+1

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/179878.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Gobot Gobot:

Patch Set 1:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=75e6cc47

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/179878.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Ian Cottrell:

Patch Set 1:

(2 comments)

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/179878.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Rebecca Stambler:

Patch Set 1:

(1 comment)

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/179878.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Gobot Gobot:

Patch Set 1: TryBot-Result+1

TryBots are happy.

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/179878.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

This PR (HEAD: cd31933) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/tools/+/179878 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

[gopherbot]

Message from Edward Muller:

Patch Set 1:

Patch Set 1:

(2 comments)

Re nil vs len()==0

The error returned is "No source for %v", which is true in either case. I'm happy to restore the nil check though instead, let me know.

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/179878.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Edward Muller:

Patch Set 2:

Patch Set 1:

(1 comment)

I capitalized AND on purpose to bring attention to it.

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/179878.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Edward Muller:

Patch Set 2:

(3 comments)

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/179878.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Ian Cottrell:

Patch Set 2:

Patch Set 1:

Patch Set 1:

(2 comments)

Re nil vs len()==0

The error returned is "No source for %v", which is true in either case. I'm happy to restore the nil check though instead, let me know.

Where possible we want the errors to match what you would see on the command line, so if the parser can cope it should be left to it.
I suspect it will produce an error about a missing package declaration for an empty file, something like "expected 'package', found 'EOF'"

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/179878.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

This PR (HEAD: bfa160b) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/tools/+/179878 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

[gopherbot]

Message from Edward Muller:

Patch Set 5:

(1 comment)

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/179878.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Ian Cottrell:

Patch Set 5: Run-TryBot+1 Code-Review+2

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/179878.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Gobot Gobot:

Patch Set 5:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=d9336d4c

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/179878.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Ian Cottrell:

Patch Set 5:

Thanks a lot for looking in to this and well found!

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/179878.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Gobot Gobot:

Patch Set 5: TryBot-Result-1

1 of 10 TryBots failed:
Failed on linux-amd64-race: https://storage.googleapis.com/go-build-log/64c134f9/linux-amd64-race_85aaa81c.log

Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/179878.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Edward Muller:

Patch Set 5:

Related to this...

I was looking into some of the methods used inside of this function and noticed that *goFile.read()'s documentation says: "It assumes that the caller is holding the mutex of the file's view.", but clearly this function is not, nor does *importer.typeCheck() (the only caller of *importer.parseFiles() I can find), but *importer.getPkg() does take locks on *importer's view.pcache.mu, but those aren't held during the call to typeCheck.

Does this comment need to be updated?

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/179878.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

Message from Rebecca Stambler:

Patch Set 5:

Patch Set 5:

Related to this...

I was looking into some of the methods used inside of this function and noticed that *goFile.read()'s documentation says: "It assumes that the caller is holding the mutex of the file's view.", but clearly this function is not, nor does *importer.typeCheck() (the only caller of *importer.parseFiles() I can find), but *importer.getPkg() does take locks on *importer's view.pcache.mu, but those aren't held during the call to typeCheck.

Does this comment need to be updated?

loadParseTypecheck is always called from one of the 3 GetToken, GetAST, or GetPackage functions, which grab the file's view's mutex at the start (see

tools/internal/lsp/cache/gofile.go

Line 22 in b3315ee

func (f *goFile) GetToken(ctx context.Context) *token.File {

for an example). That mutex ends up being held for the duration of that function, so it should be held for the read call. Please let me know if I am mistaken and if there is a place where we are calling read without holding that mutex.

---

Please don’t reply on this GitHub thread. Visit golang.org/cl/179878.
After addressing review feedback, remember to publish your drafts!

[gopherbot]

This PR is being closed because golang.org/cl/179878 has been merged.