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

go/types, types2: CheckExpr / Eval may mutate type checked objects (=> data race) [1.24 backport] #72826

Open
gopherbot opened this issue Mar 12, 2025 · 5 comments
Assignees
Labels
CherryPickCandidate Used during the release process for point releases
Milestone

Comments

@gopherbot
Copy link
Contributor

@findleyr requested issue #71817 to be considered for backport to the next 1.24 minor release.

@gopherbot please backport this to 1.24. It is causing test flakes.

@findleyr
Copy link
Member

@griesemer what do you think about this backport?

It will not be a clean cherry-pick, since the current implementation relies on VarKind. It also introduces map reads/writes replacing struct field access, which may have performance implications (but seemed not to matter significantly in benchmarking).

The problem is that a recent use of CheckExpr within gopls is causing a lot of test flakes on arbitrary tests, and there's no easy way to eliminate those flakes.

@adonovan one possibility is to live with this, writing a suitable watchflakes expression to at least consolidate the flakes into a single issue. Not great, but possible.

@griesemer
Copy link
Contributor

Let's do the backport CL and see how bad the diffs are.
I'm not worried about the map access time.
The respective 1.25 change was very controlled and limited in complexity (fairly easy to believe that it was correct).
If the backport CL is equally limited (with amendments for VarKind), I am fine with it as it seems low risk.

@findleyr findleyr self-assigned this Mar 13, 2025
@gopherbot
Copy link
Contributor Author

Change https://go.dev/cl/657695 mentions this issue: [release-branch.go1.24] go/types,types2: allocate the used* maps in initFiles

@gopherbot
Copy link
Contributor Author

Change https://go.dev/cl/657675 mentions this issue: [release-branch.go1.24] go/types,types2: externalize used objects

@findleyr
Copy link
Member

@golang/release if this is approved for cherry-pick, we'll need to merge BOTH of the CLs above (the second fixes a bug in the first). If you'd prefer, I can squash them into a single CL. Please let me know what's best.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CherryPickCandidate Used during the release process for point releases
Projects
None yet
Development

No branches or pull requests

3 participants