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

cmd/vet: warn on 3-clause loops where an iterator variable contains a lock type when GoVersion>=1.22 #66387

Closed
timothy-king opened this issue Mar 18, 2024 · 10 comments

Comments

@timothy-king
Copy link
Contributor

Proposal Details

Extend the copylock analyzer to emit a warning on the copy of the loop variable between iterations that occurs immediately before the loop's Post statement in Go >= 1.22. The requirements for the warning are when:

  • a 3-clause for loop declares a loop scoped variable containing a lock type,
  • the file's GoVersion is >= 1.22,
  • the name of the variable is not _, and
  • the Init statement of the for loop is not otherwise reported.

Example:

for _, mu := 0, (sync.Mutex{}); x < 10; x++ { // want "for loop iteration copies lock value to mu: sync.Mutex"
	_ = mu.TryLock()
}

Relevant part of the spec https://go.dev/ref/spec#For_clause :

The variable used by each subsequent iteration is declared implicitly before executing the post statement and initialized to the value of the previous iteration's variable at that moment.

@timothy-king
Copy link
Contributor Author

Proposed implementation http://go.dev/cl/569955.

Related #66156. This is 'Case 1'.

@timothy-king timothy-king changed the title cmd/vet: warn on 3-clause loops where an iterator variable contains a lock type when GoVersion>1.22 cmd/vet: warn on 3-clause loops where an iterator variable contains a lock type when GoVersion>=1.22 Mar 18, 2024
@ianlancetaylor ianlancetaylor changed the title cmd/vet: warn on 3-clause loops where an iterator variable contains a lock type when GoVersion>=1.22 proposal: cmd/vet: warn on 3-clause loops where an iterator variable contains a lock type when GoVersion>=1.22 Mar 27, 2024
@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals Mar 27, 2024
@gopherbot gopherbot added this to the Proposal milestone Mar 27, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/569955 mentions this issue: go/analysis/passes/copylock: add support for ForStmt

@rsc
Copy link
Contributor

rsc commented Jul 25, 2024

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals Jul 25, 2024
@rsc
Copy link
Contributor

rsc commented Jul 25, 2024

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Accept in Proposals Aug 7, 2024
@rsc
Copy link
Contributor

rsc commented Aug 7, 2024

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

The proposal details are in #66387 (comment).

@rsc rsc moved this from Likely Accept to Accepted in Proposals Aug 14, 2024
@rsc
Copy link
Contributor

rsc commented Aug 14, 2024

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

The proposal details are in #66387 (comment).

@rsc rsc changed the title proposal: cmd/vet: warn on 3-clause loops where an iterator variable contains a lock type when GoVersion>=1.22 cmd/vet: warn on 3-clause loops where an iterator variable contains a lock type when GoVersion>=1.22 Aug 14, 2024
@rsc rsc modified the milestones: Proposal, Backlog Aug 14, 2024
@zigo101
Copy link

zigo101 commented Aug 16, 2024

Will this be merged to the main repo some day?

@timothy-king
Copy link
Contributor Author

This is expected to go into tip during the next x/tools. Probably not more than a month. The first release it is expected to go into is 1.24.

@dmitshur dmitshur modified the milestones: Backlog, Go1.24 Nov 26, 2024
@dmitshur
Copy link
Contributor

This vet change seems like something that needs to be mentioned in Go 1.24 release notes, and it's not mentioned there yet. Reopening as a release blocker to track that.

@dmitshur dmitshur reopened this Nov 26, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/632315 mentions this issue: doc/next: document copylock changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted
Development

No branches or pull requests

6 participants