Skip to content

go/analysis/passes/loopclosure: recursively check last statements in statements like if, switch, and for #415

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
Closed
wants to merge 11 commits into from
Closed

go/analysis/passes/loopclosure: recursively check last statements in statements like if, switch, and for #415

wants to merge 11 commits into from

Conversation

thepudds
Copy link
Contributor

@thepudds thepudds commented Nov 18, 2022 •
edited
Loading

In golang/go#16520, there was a suggestion to extend the
current loopclosure check to check more statements.

The current loopclosure flags patterns like:

for k, v := range seq {
	go/defer func() {
		... k, v ...
	}()
}

For this CL, the motivating example from golang/go#16520 is:

var wg sync.WaitGroup
for i := 0; i < 10; i++ {
	for j := 0; j < 1; j++ {
		wg.Add(1)
		go func() {
			fmt.Printf("%d ", i)
			wg.Done()
		}()
	}
}
wg.Wait()

The current loopclosure check does not flag this because of the
inner for loop, and the checker looks only at the last statement
in the outer loop body.

The suggestion is we redefine "last" recursively. For example,
if the last statement is an if, then we examine the last statements
in both of its branches. Or if the last statement is a nested loop,
then we examine the last statement of that loop's body, and so on.

A few years ago, Alan Donovan sent a sketch in CL 184537.

This CL attempts to complete Alan's sketch, as well as integrates
with the ensuing changes from golang/go#55972 to check
errgroup.Group.Go, which with this CL can now be recursively "last".

Updates golang/go#16520
Updates golang/go#55972
Fixes golang/go#30649
Fixes golang/go#32876

thepudds and others added 2 commits November 18, 2022 15:11
@thepudds
go/analysis/passes/loopclosure: recursively check last statements in …
259b411 
…statements like if, switch, and for

In golang/go#16520, Allan Donovan suggested the current loopclosure
check could be extended to check more statements.

The current loopclosure flags patterns like:

	for k, v := range seq {
		go/defer func() {
			... k, v ...
		}()
	}

The motivating example for this CL from golang/go#16520 was:

	var wg sync.WaitGroup
	for i := 0; i < 10; i++ {
		for j := 0; j < 1; j++ {
			wg.Add(1)
			go func() {
				fmt.Printf("%d ", i)
				wg.Done()
			}()
		}
	}
	wg.Wait()

The current loopclosure check does not flag this because of the
inner for loop, and the checker looks only at the last statement
in the outer loop body.

Allan suggested we redefine "last" recursively. For example,
if the last statement is an if, then we examine the last statements
in both of its branches. Or if the last statement is a nested loop,
then we examine the last statement of that loop's body, and so on.

A few years ago, Allan sent a sketch in CL 184537.

This CL attempts to complete Allan's sketch, as well as integrates
with the ensuing changes from golang/go#55972 to check errgroup.Group.Go,
which with this CL can now be recursively "last".

Updates golang/go#16520
Updates golang/go#55972
@thepudds
Merge branch 'golang:master' into vet-loopclosure
a220948
@gopherbot
Copy link
Contributor

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

Please visit https://go-review.googlesource.com/c/tools/+/452155 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
Copy link
Contributor

Message from Gopher Robot:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps:
A maintainer will review your change and provide feedback. See
https://go.dev/doc/contribute#review for more info and tips to get your
patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.

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

@gopherbot
Copy link
Contributor

Message from Daniel Martí:

Patch Set 1: Run-TryBot+1

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

@gopherbot
Copy link
Contributor

Message from Gopher Robot:

Patch Set 1:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from kokoro:

Patch Set 1:

Kokoro presubmit build starting for golang/tools/gopls-legacy/presubmit
Logs at:
https://source.cloud.google.com/results/invocations/316735ec-06b5-49d9-aac2-0e678592c421

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

@gopherbot
Copy link
Contributor

Message from kokoro:

Patch Set 1: gopls-CI+1

Kokoro presubmit build finished with status: SUCCESS
Logs at: https://source.cloud.google.com/results/invocations/316735ec-06b5-49d9-aac2-0e678592c421

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

@gopherbot
Copy link
Contributor

Message from Gopher Robot:

Patch Set 1: TryBot-Result+1

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from Robert Findley:

Patch Set 1:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from Tim King:

Patch Set 1:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from thepudds:

Patch Set 2:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from kokoro:

Patch Set 2:

Kokoro presubmit build starting for golang/tools/gopls-legacy/presubmit
Logs at:
https://source.cloud.google.com/results/invocations/1754968d-5aac-44ba-80a4-a3bd02c69134

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

@gopherbot
Copy link
Contributor

Message from kokoro:

Patch Set 2: gopls-CI+1

Kokoro presubmit build finished with status: SUCCESS
Logs at: https://source.cloud.google.com/results/invocations/1754968d-5aac-44ba-80a4-a3bd02c69134

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

@gopherbot
Copy link
Contributor

Message from Tim King:

Patch Set 1:

(6 comments)

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

@gopherbot
Copy link
Contributor

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

Please visit https://go-review.googlesource.com/c/tools/+/452155 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
Copy link
Contributor

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

Please visit https://go-review.googlesource.com/c/tools/+/452155 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
Copy link
Contributor

Message from thepudds:

Patch Set 6:

(7 comments)

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

@gopherbot
Copy link
Contributor

Message from thepudds:

Patch Set 6:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from thepudds:

Patch Set 6:

(1 comment)

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

@gopherbot
Copy link
Contributor

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

Please visit https://go-review.googlesource.com/c/tools/+/452155 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
Copy link
Contributor

Message from Alan Donovan:

Patch Set 9:

(6 comments)

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

@gopherbot
Copy link
Contributor

Message from thepudds:

Patch Set 9:

(1 comment)

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

@gopherbot
Copy link
Contributor

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

Please visit https://go-review.googlesource.com/c/tools/+/452155 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
Copy link
Contributor

Message from Tim King:

Patch Set 9:

(5 comments)

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

@gopherbot
Copy link
Contributor

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

Please visit https://go-review.googlesource.com/c/tools/+/452155 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
Copy link
Contributor

Message from thepudds:

Patch Set 12:

(8 comments)

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

@gopherbot
Copy link
Contributor

Message from Alan Donovan:

Patch Set 12:

(2 comments)

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

@gopherbot
Copy link
Contributor

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

Please visit https://go-review.googlesource.com/c/tools/+/452155 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
Copy link
Contributor

Message from thepudds:

Patch Set 14:

(6 comments)

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

@gopherbot
Copy link
Contributor

Message from Alan Donovan:

Patch Set 14: Code-Review+2

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from Robert Findley:

Patch Set 14: Code-Review+1

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

@gopherbot
Copy link
Contributor

Message from Alan Donovan:

Patch Set 14: Run-TryBot+1

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

@gopherbot
Copy link
Contributor

Message from Gopher Robot:

Patch Set 14:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from Daniel Martí:

Patch Set 14: Run-TryBot+1

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

@gopherbot
Copy link
Contributor

Message from kokoro:

Patch Set 14:

Kokoro presubmit build starting for golang/tools/gopls-legacy/presubmit
Logs at:
https://source.cloud.google.com/results/invocations/9a0745d2-63d5-4c2b-97d1-96f660d44c16

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

@gopherbot
Copy link
Contributor

Message from thepudds:

Patch Set 14:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from Gopher Robot:

Patch Set 14:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from Tim King:

Patch Set 14: Code-Review+2

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

@gopherbot
Copy link
Contributor

Message from thepudds:

Patch Set 14:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from Robert Findley:

Patch Set 14:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from kokoro:

Patch Set 14: gopls-CI-1

Kokoro presubmit build finished with status: FAILURE
Logs at: https://source.cloud.google.com/results/invocations/9a0745d2-63d5-4c2b-97d1-96f660d44c16

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

@gopherbot
Copy link
Contributor

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

Please visit https://go-review.googlesource.com/c/tools/+/452155 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
Copy link
Contributor

Message from thepudds:

Patch Set 14:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from Gopher Robot:

Patch Set 14: TryBot-Result-1

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from Tim King:

Patch Set 15: TryBot-Bypass+1 Code-Review+2

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

@gopherbot
Copy link
Contributor

Message from Tim King:

Patch Set 15: Run-TryBot+1 -TryBot-Bypass

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

@gopherbot
Copy link
Contributor

Message from Alan Donovan:

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

(2 comments)

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

@gopherbot
Copy link
Contributor

Message from Gopher Robot:

Patch Set 15:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from kokoro:

Patch Set 15:

Kokoro presubmit build starting for golang/tools/gopls-legacy/presubmit
Logs at:
https://source.cloud.google.com/results/invocations/8b5355db-3930-4cd7-a971-4ddf743d9a54

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

gopherbot pushed a commit that referenced this pull request Nov 21, 2022
@thepudds @timothy-king
go/analysis/passes/loopclosure: recursively check last statements in …
b797704 
…statements like if, switch, and for

In golang/go#16520, there was a suggestion to extend the
current loopclosure check to check more statements.

The current loopclosure flags patterns like:

	for k, v := range seq {
		go/defer func() {
			... k, v ...
		}()
	}

For this CL, the motivating example from golang/go#16520 is:

	var wg sync.WaitGroup
	for i := 0; i < 10; i++ {
		for j := 0; j < 1; j++ {
			wg.Add(1)
			go func() {
				fmt.Printf("%d ", i)
				wg.Done()
			}()
		}
	}
	wg.Wait()

The current loopclosure check does not flag this because of the
inner for loop, and the checker looks only at the last statement
in the outer loop body.

The suggestion is we redefine "last" recursively. For example,
if the last statement is an if, then we examine the last statements
in both of its branches. Or if the last statement is a nested loop,
then we examine the last statement of that loop's body, and so on.

A few years ago, Alan Donovan sent a sketch in CL 184537.

This CL attempts to complete Alan's sketch, as well as integrates
with the ensuing changes from golang/go#55972 to check
errgroup.Group.Go, which with this CL can now be recursively "last".

Updates golang/go#16520
Updates golang/go#55972
Fixes golang/go#30649
Fixes golang/go#32876

Change-Id: If66c6707025c20f32a2a781f6d11c4901f15742a
GitHub-Last-Rev: 04980e0
GitHub-Pull-Request: #415
Reviewed-on: https://go-review.googlesource.com/c/tools/+/452155
Reviewed-by: Tim King <[email protected]>
Run-TryBot: Tim King <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>
gopls-CI: kokoro <[email protected]>
Reviewed-by: Robert Findley <[email protected]>
Run-TryBot: Alan Donovan <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
@gopherbot
Copy link
Contributor

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

@gopherbot gopherbot closed this Nov 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@thepudds @gopherbot