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: position-independent type checking #69673

Open
adonovan opened this issue Sep 27, 2024 · 5 comments
Open

go/types: position-independent type checking #69673

adonovan opened this issue Sep 27, 2024 · 5 comments

Comments

@adonovan
Copy link
Member

The following go/types APIs all use Pos in semantically significant ways:

Scope.LookupParent(name, pos)
Scope.Contains(pos)
Scope.Innermost(pos)
CheckExpr(fset, pkg, pos, expr, info)
Eval(fset, pkg, pos)

As the doc comment on Innermost says, "The result is guaranteed to be valid only if the type-checked AST has complete position information." This restriction applies equally to all these operations, and should probably be made explicit for all of them.

For example, Scope.LookupParent uses the position of the reference to compute the set of declarations that are in scope. This assumes the position information is accurate, which it is for trees produced by the parser, but not for ones that have been modified by refactoring algorithms or synthesized directly. It should be possible to type-check any syntax tree correctly, even without accurate position information.

In each case, the position is used as a shorthand to indicate the portion of the environment that is accessible at a given point. It would be possible to provide parallel APIs for these 5 functions, without the restriction, that replaces pos with a different parameter that indicates the environment. For example, it could be something like an []ast.Node indicating a path from the root of the tree to the designated node.

Internally, the tree of Scopes could record the environment in a form that is independent of position, similar to my lazy type checker, which mapped each Node to a pair of a Scope and an int that represents the length of the prefix of Scope symbols that are in scope at that point. Ignoring efficiency concerns, I imagine the existing Pos-based functions could become wrappers around a function that resolves a Pos to a []Node (assuming valid pos info), followed by a call to the []Node-based API.

This would open the door to composable refactorings that mutate the tree in a sequence of passes, invalidating position info but still allowing the type checker to be reinvoked after each mutation, before finally formatting the tree to produce the updated output. There are still many open questions about how to build such refactorings: we rely heavily on pos for debugging and error reporting; re-typechecking may require additional packages that were not needed before; and so on. But it is both viable and desirable to make the type checker itself fully independent of syntax positions, so they are used only for error messages, or passed through to Object.Pos, but have no semantics.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/616259 mentions this issue: go/types, types2: replace 2 uses of Scope.LookupParent with Checker.lookup

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/616337 mentions this issue: go/types, types2: remove remaining internal use of Scope.LookupParent

gopherbot pushed a commit that referenced this issue Sep 27, 2024
…ookup

A step towards removing reliance on Scope.LookupParent.

Updates #69673.

Change-Id: I9fdd4b08ea600b531b90895ac779fdc580ff00e5
Reviewed-on: https://go-review.googlesource.com/c/go/+/616259
LUCI-TryBot-Result: Go LUCI <[email protected]>
Auto-Submit: Robert Griesemer <[email protected]>
Commit-Queue: Robert Griesemer <[email protected]>
Reviewed-by: Robert Griesemer <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/616260 mentions this issue: go/types, types2: remove need for Scope.LookupParent from TestObjectString

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/616261 mentions this issue: go/types, types2: move go/types-only Scope methods into scopes2.go

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/616316 mentions this issue: go/types, types2: remove Checker.pos from types2 code - not needed anymore

gopherbot pushed a commit that referenced this issue Sep 27, 2024
This moves the implementation of Scope.LookupParent into
environment.lookupScope where it encapsulates the use of
the current environment's position. At least in types2,
that position can be removed, because it is never set.

With this, the type checker doesn't rely on position
information anymore for looking up objects during type
checking.

LookupParent is still called from tests and some go/types
code.

Updates #69673.

Change-Id: I7159ba95b71cf33cc3b16058aa19327e166224b1
Reviewed-on: https://go-review.googlesource.com/c/go/+/616337
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Robert Griesemer <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>
Auto-Submit: Robert Griesemer <[email protected]>
gopherbot pushed a commit that referenced this issue Sep 27, 2024
…tring

Updates #69673.

Change-Id: I0ce5f009c1e95a2722a50d79a74fef83d2547b47
Reviewed-on: https://go-review.googlesource.com/c/go/+/616260
Auto-Submit: Robert Griesemer <[email protected]>
Reviewed-by: Robert Griesemer <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
gopherbot pushed a commit that referenced this issue Sep 27, 2024
Remove them them from types2.

Updates #69673.

Change-Id: I7843f6da1edf3a19f85c61706104d173e04088d3
Reviewed-on: https://go-review.googlesource.com/c/go/+/616261
Reviewed-by: Alan Donovan <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Auto-Submit: Robert Griesemer <[email protected]>
Reviewed-by: Robert Griesemer <[email protected]>
gopherbot pushed a commit that referenced this issue Sep 27, 2024
…ymore

In go/types, move field down in environment struct, rename it to
exprPos, and document use.

Updates #69673.

Change-Id: I355af1237f8cd731ad9706e6a5fce34b314978cc
Reviewed-on: https://go-review.googlesource.com/c/go/+/616316
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>
Auto-Submit: Robert Griesemer <[email protected]>
Reviewed-by: Robert Griesemer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants