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: validity of program depends on method declaration order #23203

Closed
griesemer opened this issue Dec 21, 2017 · 7 comments
Closed

go/types: validity of program depends on method declaration order #23203

griesemer opened this issue Dec 21, 2017 · 7 comments
Assignees
Milestone

Comments

@griesemer
Copy link
Contributor

@griesemer griesemer commented Dec 21, 2017

https://play.golang.org/p/WCktUidLyc_ is accepted while the equivalent program https://play.golang.org/p/Z-B9rBhYILd fails with an error. The only difference is the order of the method declarations.

Esoteric case, but may not be too hard to get right. Marking for 1.11 in case we get to it.

See also #23202.

@griesemer griesemer added this to the Go1.11 milestone Dec 21, 2017
@griesemer griesemer self-assigned this Dec 21, 2017
@griesemer
Copy link
Contributor Author

@griesemer griesemer commented Dec 21, 2017

See TODO in go/types/decl.go (Checker.typeDecl). The solution might be to simply add the methods (call addMethodDecls) before we call check.typExpr (so that the methods are present when we look for the type to be declared). To avoid other errors we may need to defer the check for conflicting struct and methods names to the end of type-checking.

Loading

@griesemer
Copy link
Contributor Author

@griesemer griesemer commented May 2, 2018

Investigated a bit more: While it is easy to add methods before type-checking, type-checking those methods at that time may lead to cycles. Type-checking the methods later will lead to crashed because the methods have missing types (but have already been associated with the base type).

This may require more work on how we handle cycles in the first place.

Postponing for now as non-urgent since the (gc) compiler cannot handle this case, either.

Loading

@griesemer griesemer removed this from the Go1.11 milestone May 2, 2018
@griesemer griesemer added this to the Go1.12 milestone May 2, 2018
@griesemer
Copy link
Contributor Author

@griesemer griesemer commented May 2, 2018

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 3, 2018

Change https://golang.org/cl/139422 mentions this issue: go/types: determine hasPtrRecv property from source rather than type

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 4, 2018

Change https://golang.org/cl/139425 mentions this issue: go/types: delay type-checking of methods to when they are used

Loading

gopherbot pushed a commit that referenced this issue Oct 4, 2018
LookupFieldOrMethod needs to know if a method receiver is a pointer
type. Until now this was computed from the the method signature's
receiver, which required the method signature to be type-checked.
Furthermore, it required the receiver to be set before the method
signature was fully type-checked in some cases (see issue #6638).

This CL remembers this property during object resolution, when we
know it from the source.

With this CL, method signatures don't need to be type-checked before
they can be looked up; this is a first step towards separating
type checking of types and type-checking of associated methods.

Updates #23203.
Updates #26854.

Change-Id: Ie3eb7976e8fe8176ea1b284fa7471a4b7000f80b
Reviewed-on: https://go-review.googlesource.com/c/139422
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
@gopherbot
Copy link

@gopherbot gopherbot commented Oct 4, 2018

Change https://golang.org/cl/139897 mentions this issue: go/types: don't type-check method signatures eagerly anymore

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 4, 2018

Change https://golang.org/cl/139721 mentions this issue: go/types: fix recvPtr helper (follow-up on https://golang.org/cl/139422)

Loading

gopherbot pushed a commit that referenced this issue Oct 5, 2018
The prior CL prepared go/types for the situation where methods might
not have a type-checked signature when being looked up. The respective
adjustments to recvPtr were not correct (but because so far method
signatures are type-checked in time, the bug didn't manifest itself).

Updates #23203.
Updates #26854.

Change-Id: I796691d11e6aac84396bdef802ad30715755fcc6
Reviewed-on: https://go-review.googlesource.com/c/139721
Reviewed-by: Alan Donovan <adonovan@google.com>
gopherbot pushed a commit that referenced this issue Oct 5, 2018
…re used

Remove assumption that methods associated to concrete (non-interface)
types have a fully set up signature. Such methods are found through
LookupFieldOrMethod or lookupMethod, or indexed method access from
a Named type. Make sure that the method's signature is type-checked
before use in those cases.

(MethodSets also hold methods but the type checker is not using
them but for internal verification. API clients will be using it
after all methods have been type-checked.)

Some functions such as MissingMethod may now have to type-check a
method and for that they need a *Checker. Add helper functions as
necessary to provide the additional (receiver) parameter but permit
it to be nil if the respective functions are invoked through the API
(at which point we know that all methods have a proper signature and
thus we don't need the delayed type-check).

Since all package-level objects eventually are type-checked through
the top-level loop in Checker.packageObjects we are guaranteed that
all methods will be type-checked as well.

Updates #23203.
Updates #26854.

Change-Id: I6e48f0016cefd498aa70b776e84a48215a9042c5
Reviewed-on: https://go-review.googlesource.com/c/139425
Reviewed-by: Alan Donovan <adonovan@google.com>
@gopherbot gopherbot closed this in 8ae8576 Oct 5, 2018
@golang golang locked and limited conversation to collaborators Oct 5, 2019
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
2 participants