Skip to content

go/analysis/passes/loopclosure: allow certain well-understood statements to trail problematic go and defer statements #417

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 17 commits into from
Closed

go/analysis/passes/loopclosure: allow certain well-understood statements to trail problematic go and defer statements #417

wants to merge 17 commits into from

Conversation

thepudds
Copy link
Contributor

@thepudds thepudds commented Dec 5, 2022 •
edited
Loading

WIP.

Additional commits and longer commit message coming...

For now, see description in the opening comment of golang/go#57173.

This change allows the loopclosure analyzer to examine
go, defer, and errgroup.Group.Go statements that are not the
last statement in a compound statement body.

For example, this allows flagging captured loop variables such as
the following, which is a real-world example from golang/go#21412:

func handleBatch(ctx context.Context, batch *Batch) error {
    var chans []chan error
    for _, request := range batch.Requests {
        c := make(chan error)
        go func(c chan error) {
            c <- handle(ctx, request) // vet now flags: "loop variable request captured by func literal"
        }(c)
        chans = append(chans, c)
    }
    // ... read chans ...
}

For performance sanity checking, we ran a few flavors of the standalone
'loopclosure' analyzer command in this CL against the entire
github.com/hashicorp/terraform module via 'loopclosure ./...'.

The 'loopclosure' command uses go/analysis/singlechecker. Comparing
a 1.19 'loopclosure' command to this CL on the terraform module, it
might be ever so slightly faster now,
presumably due to other changes in 1.20:

name old time/op new time/op delta
Loopclosure 4.34s ± 3% 4.30s ± 4% -1.00% (p=0.005 n=27+30)

The performance difference between 1.20rc1 and this CL seems to be in
the noise for a single 'loopclosure' command run on the terraform module:

name old time/op new time/op delta
Loopclosure 4.30s ± 4% 4.30s ± 2% ~ (p=0.892 n=30+27)

Benchmarking the 'loopclosure' command seems to indicate that other
costs outside this CL heavily dominate (e.g., cmd/go getting package
config info, the follow-on type checking, building of the inspector, and
so on).

If we instead use go/analysis/multichecker to repeatedly run the
loopclosure analyzer pass against the terraform module 100 times or
1000 times in a single invocation of the analysis driver binary, and
then use a simple linear model to estimate the per pass cost of the
loopclosure analysis:

go 1.20rc1: 10.5 millisec/pass for entire terraform module
this CL: 15.1 millisec/pass for entire terraform module

Fixes golang/go#21412
Fixes golang/go#57173
Updates golang/go#16520

@thepudds
go/analysis/passes/loopclosure: refactor to add a visitor type replac…
9f2680c 
…ing forEachLastStmt; no external behavior change
@gopherbot
Copy link
Contributor

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

Please visit https://go-review.googlesource.com/c/tools/+/455195 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 1: Run-TryBot+1

Please don’t reply on this GitHub thread. Visit golang.org/cl/455195.
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/455195.
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/be2765d8-fb8e-449f-ad7e-f21acefc69cb

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

@thepudds
go/analysis/passes/loopclosure: add filtering to allow certain well-u…
2a84b3d 
…nderstood statements to trail problematic go and defer statements
@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/be2765d8-fb8e-449f-ad7e-f21acefc69cb

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

@gopherbot
Copy link
Contributor

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

Please visit https://go-review.googlesource.com/c/tools/+/455195 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: TryBot-Result+1

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from thepudds:

Patch Set 2: Run-TryBot+1

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

@gopherbot
Copy link
Contributor

Message from Gopher Robot:

Patch Set 2:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/455195.
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/1226ec48-c6bf-48f7-8782-3f79b775cb09

Please don’t reply on this GitHub thread. Visit golang.org/cl/455195.
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/1226ec48-c6bf-48f7-8782-3f79b775cb09

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

@gopherbot
Copy link
Contributor

Message from Gopher Robot:

Patch Set 2: TryBot-Result+1

(1 comment)

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

@thepudds
go/analysis/passes/loopclosure: restore exact Go 1.20 logic, with fut…
e8e1fa9 
…ure logic now only accessible via internal flag
@gopherbot
Copy link
Contributor

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

Please visit https://go-review.googlesource.com/c/tools/+/455195 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 3: Run-TryBot+1

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

@gopherbot
Copy link
Contributor

Message from Gopher Robot:

Patch Set 3:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from kokoro:

Patch Set 3:

Kokoro presubmit build starting for golang/tools/gopls-legacy/presubmit
Logs at:
https://source.cloud.google.com/results/invocations/2fe94ef2-618a-46cb-9ce1-ea25e4bae658

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

@gopherbot
Copy link
Contributor

Message from Gopher Robot:

Patch Set 3: TryBot-Result+1

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from thepudds:

Patch Set 3: -Run-TryBot

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from kokoro:

Patch Set 3: gopls-CI+1

Kokoro presubmit build finished with status: SUCCESS
Logs at: https://source.cloud.google.com/results/invocations/2fe94ef2-618a-46cb-9ce1-ea25e4bae658

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

@gopherbot
Copy link
Contributor

Message from thepudds:

Patch Set 3:

(1 comment)

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

@gopherbot
Copy link
Contributor

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

Please visit https://go-review.googlesource.com/c/tools/+/455195 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 4: Run-TryBot+1

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

@gopherbot
Copy link
Contributor

Message from Gopher Robot:

Patch Set 4:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from kokoro:

Patch Set 4:

Kokoro presubmit build starting for golang/tools/gopls-legacy/presubmit
Logs at:
https://source.cloud.google.com/results/invocations/6b614a64-e0aa-4682-9f2f-47af40f27fb7

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

@gopherbot
Copy link
Contributor

Message from Gopher Robot:

Patch Set 4: TryBot-Result+1

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from kokoro:

Patch Set 4: gopls-CI+1

Kokoro presubmit build finished with status: SUCCESS
Logs at: https://source.cloud.google.com/results/invocations/6b614a64-e0aa-4682-9f2f-47af40f27fb7

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

@gopherbot
Copy link
Contributor

Message from thepudds:

Patch Set 17: Run-TryBot+1

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

@gopherbot
Copy link
Contributor

Message from Gopher Robot:

Patch Set 17:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from kokoro:

Patch Set 17:

Kokoro presubmit build starting for golang/tools/gopls-legacy/presubmit
Logs at:
https://source.cloud.google.com/results/invocations/1e0d4525-ca2d-450d-9273-f629804183b0

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

@gopherbot
Copy link
Contributor

Message from kokoro:

Patch Set 17: gopls-CI-1

Kokoro presubmit build finished with status: FAILURE
Logs at: https://source.cloud.google.com/results/invocations/1e0d4525-ca2d-450d-9273-f629804183b0

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

@gopherbot
Copy link
Contributor

Message from Gopher Robot:

Patch Set 17: TryBot-Result+1

(1 comment)

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

@gopherbot
Copy link
Contributor

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

Please visit https://go-review.googlesource.com/c/tools/+/455195 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 18: Run-TryBot+1

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

@gopherbot
Copy link
Contributor

Message from Gopher Robot:

Patch Set 18:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from kokoro:

Patch Set 18:

Kokoro presubmit build starting for golang/tools/gopls-legacy/presubmit
Logs at:
https://source.cloud.google.com/results/invocations/d890e42b-1c0c-4a9b-8f7c-7c61caba34a1

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

@gopherbot
Copy link
Contributor

Message from kokoro:

Patch Set 18: gopls-CI-1

Kokoro presubmit build finished with status: FAILURE
Logs at: https://source.cloud.google.com/results/invocations/d890e42b-1c0c-4a9b-8f7c-7c61caba34a1

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

@gopherbot
Copy link
Contributor

Message from Gopher Robot:

Patch Set 18: TryBot-Result+1

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from thepudds:

Patch Set 18:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from kokoro:

Patch Set 18:

Kokoro presubmit build starting for golang/tools/gopls-legacy/presubmit
Logs at:
https://source.cloud.google.com/results/invocations/ca0bd682-6505-4dab-b827-09fae549106d

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

@gopherbot
Copy link
Contributor

Message from kokoro:

Patch Set 18:

Kokoro presubmit build finished with status: FAILURE
Logs at: https://source.cloud.google.com/results/invocations/ca0bd682-6505-4dab-b827-09fae549106d

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

@gopherbot
Copy link
Contributor

Message from Tim King:

Patch Set 18:

(3 comments)

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

@gopherbot
Copy link
Contributor

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

Please visit https://go-review.googlesource.com/c/tools/+/455195 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 20:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from thepudds:

Patch Set 20: Run-TryBot+1

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

@gopherbot
Copy link
Contributor

Message from Gopher Robot:

Patch Set 20:

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from kokoro:

Patch Set 20:

Kokoro presubmit build starting for golang/tools/gopls-legacy/presubmit
Logs at:
https://source.cloud.google.com/results/invocations/b89ff042-50ca-4d42-a2cf-79d66f23465a

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

@gopherbot
Copy link
Contributor

Message from Gopher Robot:

Patch Set 20: TryBot-Result+1

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from kokoro:

Patch Set 20: gopls-CI+1

Kokoro presubmit build finished with status: SUCCESS
Logs at: https://source.cloud.google.com/results/invocations/b89ff042-50ca-4d42-a2cf-79d66f23465a

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

@gopherbot
Copy link
Contributor

Message from kokoro:

Patch Set 21:

Kokoro presubmit build starting for golang/tools/gopls-legacy/presubmit
Logs at:
https://source.cloud.google.com/results/invocations/d2aa91d3-c6f4-4c37-b613-508652c45135

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

@gopherbot
Copy link
Contributor

Message from kokoro:

Patch Set 21: gopls-CI+1

Kokoro presubmit build finished with status: SUCCESS
Logs at: https://source.cloud.google.com/results/invocations/d2aa91d3-c6f4-4c37-b613-508652c45135

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

@gopherbot
Copy link
Contributor

Message from thepudds:

Patch Set 21: Run-TryBot+1

(1 comment)

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

@gopherbot
Copy link
Contributor

Message from Alan Donovan:

Patch Set 21:

(1 comment)

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

@thepudds thepudds closed this Jan 31, 2024
@gopherbot
Copy link
Contributor

Message from t hepudds:

Patch Set 21:

(1 comment)

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

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