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: incorrect scope positioning for variables defined by RangeStmt #15686

Closed
mdempsky opened this issue May 14, 2016 · 4 comments
Closed

go/types: incorrect scope positioning for variables defined by RangeStmt #15686

mdempsky opened this issue May 14, 2016 · 4 comments
Assignees
Milestone

Comments

@mdempsky
Copy link
Member

@mdempsky mdempsky commented May 14, 2016

In go/types/stmt.go:818,819, there's this code to handle declaring for a RangeStmt's Key and Value variables:

scopePos := s.End()
check.declare(check.scope, nil /* recordDef already called */, obj, scopePos)

This is wrong though, because s refers to the RangeStmt itself, so s.End() is the closing brace. The consequence of this is if you try to use scope.LookupParent or types.Eval with a position inside the RangeStmt's block, it can't find the Key or Value variables.

I think the correct fix is to use s.X.End() instead.

Reviewing other check.declare calls, I think type switches are technically mishandled too. The spec says "the variable is declared at the beginning of the implicit block in each clause". I believe that should be the position of the "case" or "default" keywords, or at least at or immediately after the colon; not at the start of the first statement within the body.

These are preexisting issues, but fixing them should be very non-intrusive, so I'm hopeful it can still make 1.7.

/cc @griesemer @alandonovan

@mdempsky mdempsky added this to the Go1.7Maybe milestone May 14, 2016
@rsc
Copy link
Contributor

@rsc rsc commented May 17, 2016

Too late for Go 1.7.

Loading

@rsc rsc added this to the Go1.8 milestone May 17, 2016
@rsc rsc removed this from the Go1.7Maybe milestone May 17, 2016
@griesemer griesemer self-assigned this May 18, 2016
@griesemer
Copy link
Contributor

@griesemer griesemer commented May 18, 2016

@mdempsky I agree that this looks non-intrusive, but I believe @alandonovan has code that looks at scope ranges for various analyses. There may be subtle follow-up effects/errors that we don't foresee at the moment. I think it's better to leave for 1.8.

Loading

@mdempsky
Copy link
Member Author

@mdempsky mdempsky commented May 18, 2016

@griesemer Fair enough. I found a reasonably robust and non-intrusive workaround anyway, so I'm fine with deferring this issue to 1.8.

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 17, 2016

CL https://golang.org/cl/27290 mentions this issue.

Loading

@gopherbot gopherbot closed this in fd8028d Aug 18, 2016
mdempsky added a commit to mdempsky/gocode that referenced this issue Apr 19, 2017
Go 1.8 included the fix for golang/go#15686, so no need to workaround
it anymore.

However, we also now have position information for imported objects,
which interferes with scoping rules. go/types doesn't export
Object.scopePos, so switch to using go/types.(*Scope).LookupParent
instead.
@golang golang locked and limited conversation to collaborators Aug 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants