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: scopePos is unset for parameter variables #64292

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
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@adonovan
Copy link
Member

adonovan commented Nov 20, 2023

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

@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
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
(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
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

3 participants