Skip to content

go/types: scopePos is unset for parameter variables #64292

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
adonovan opened this issue Nov 20, 2023 · 3 comments
Closed

go/types: scopePos is unset for parameter variables #64292

adonovan opened this issue Nov 20, 2023 · 3 comments
Assignees
@adonovan
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.22

Comments

@adonovan
Copy link
Member

adonovan commented Nov 20, 2023 •
edited
Loading

go/types associates with each Object the position at which its scope effectively begins (scopePos). For a package-level decl or an import, it's NoPos. For a type parameter, it's the 'func' keyword. For each declaration local to a function body, it's the end of the ValueSpec or TypeSpec or := statement that declared it. For a parameter or named result variable, it should be the end of the FuncType, but in fact it is NoPos.

This is the root cause of #60752 in gopls, though it took me a long time to realize it.

The fix is

diff --git a/src/go/types/signature.go b/src/go/types/signature.go
index ed9fcfe58e..03e0facc9c 100644
--- a/src/go/types/signature.go
+++ b/src/go/types/signature.go
@@ -176,10 +176,13 @@ func (check *Checker) funcType(sig *Signature, recvPar *ast.FieldList, ftyp *ast
                }
        }
 
-       // Value (non-type) parameters' scope starts in the function body. Use a temporary scope for their
-       // declarations and then squash that scope into the parent scope (and report any redeclarations at
-       // that time).
-       scope := NewScope(check.scope, nopos, nopos, "function body (temp. scope)")
+       // Value (non-type) parameters' scope starts in the function
+       // body, if any: in other words, after the FuncType.
+       //
+       // Use a temporary scope for their declarations and then
+       // squash that scope into the parent scope (and report any
+       // redeclarations at that time).
+       scope := NewScope(check.scope, ftyp.End(), nopos, "function body (temp. scope)")
@adonovan adonovan self-assigned this Nov 20, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/544035 mentions this issue: go/types: set correct scopePos for parameters/results

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/543720 mentions this issue: gopls/internal/lsp/source: fix rename spurious shadowing error

@adonovan adonovan mentioned this issue Nov 27, 2023
Closed
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/545316 mentions this issue: gopls/internal/lsp/source/extract: don't assume Scope.Pos

gopherbot pushed a commit to golang/tools that referenced this issue Nov 28, 2023
@adonovan @gopherbot
gopls/internal/lsp/source/extract: don't assume Scope.Pos

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
72ee4f8 
This change updates the extract logic to no longer assume
that the Scope.Pos for a local variable declared directly within
a function is FuncDecl.Body. This is necessary to prevent
the extract logic (and its tests) from breaking when
go1.22 changes the Scope.Pos to FuncDecl.Type.Func,
i.e. the "func" token.

Updates golang/go#64292

Change-Id: Ia7edd2d5576fd6d1cd97607f3b4ed1302e477057
Reviewed-on: https://go-review.googlesource.com/c/tools/+/545316
Auto-Submit: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
gopherbot pushed a commit to golang/tools that referenced this issue Dec 4, 2023
@adonovan
gopls/internal/lsp/source: add test for rename spurious shadowing

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
5e672e2 
(Second attempt at CL 509560, rolled back in a5e8bb0, because
it broke tests that had been added since this CL was tested.
It was the wrong fix; the right fix is CL 544035 to go/types.)

This change documents and tests the go1.22-only fix for a bug
in the rename lexical shadowing check due to a bug in go/types.

Updates golang/go#60752
Updates golang/go#64292

Change-Id: I9bc96807fdd2ee5c4712268de16cf2d04513773a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/543720
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Commit-Queue: Alan Donovan <adonovan@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Alan Donovan <adonovan@google.com>
@dmitshur dmitshur added the NeedsFix The path to resolution is known, but the work has not been done. label Dec 20, 2023
@dmitshur dmitshur added this to the Go1.22 milestone Dec 20, 2023
teddywing added a commit to teddywing/gocapturedrefrace that referenced this issue Mar 10, 2024
@teddywing
Clean up Go 1.22 tests and fix broken analysis under Go 1.22

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
1926d13 
* Remove my debugging tests from trying to understand the Go 1.22
  problem.
* Describe the problem caused by the change in 'go/types' starting in Go
  1.22.0.

The problem was caused by
golang/go@a27a525,
which resolved the following two issues related to 'gopls':

* golang/go#64292
* golang/go#64295
@adonovan adonovan mentioned this issue Jun 6, 2024
Open
@golang golang locked and limited conversation to collaborators Dec 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@adonovan adonovan

Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.22
Development

No branches or pull requests
3 participants
@dmitshur @adonovan @gopherbot