go/types, types2: always set the receiver type of interface methods to the defining interface type #49906
Comments
griesemer
added
the
NeedsInvestigation
|
I'd like to suggest that the Func's receiver parameter always points to the For example, I suggest https://go.dev/play/p/weOZB-AO-w2 should print: rather than like it does currently. I think this would be more consistent and also simplify importers. |
|
Change https://go.dev/cl/386003 mentions this issue: |
|
Change https://go.dev/cl/386004 mentions this issue: |
|
Marking for Go 1.19 so this stays on the radar. |
griesemer
changed the title
|
Change https://go.dev/cl/396516 mentions this issue: |
|
I am ok with this change. As far as I can tell, it's important to have a non-nil receiver (so we know that we have a method), but for interface methods it doesn't matter whether the receiver is the declaring interface proper or the named type if one was given to the interface. CL 396516 makes the relevant changes in @findleyr Do you see any problems with tools (backward-compatibility)? |
Now that there's a native go/types importer for unified IR, the compiler no longer needs to stay backwards compatible with old iexport importers. This CL also updates the go/types and go/internal/gcimporter tests to expect that the unified IR importer sets the receiver parameter type to the underlying Interface type, rather than the Named type. This is a temporary workaround until we make a decision on #49906. Notably, this makes `GOEXPERIMENT=unified go test` work on generics code without requiring `-vet=off` (because previously cmd/vet was relying on unified IR's backwards-compatible iexport data, which omitted generic types). Change-Id: Iac7a2346bb7a91e6690fb2978fb702fadae5559d Reviewed-on: https://go-review.googlesource.com/c/go/+/386004 Trust: Matthew Dempsky <mdempsky@google.com> Run-TryBot: Matthew Dempsky <mdempsky@google.com> Reviewed-by: Robert Griesemer <gri@golang.org> Reviewed-by: Robert Findley <rfindley@google.com> TryBot-Result: Gopher Robot <gobot@golang.org>
|
We have now pending CL 396516 that implements the suggestion by @mdempsky , but for the failures in the @mdempsky mentions that the chance would simplify importers. That's true, and it also simplifies the type checker slightly. But leaving the requirements as is doesn't seem particularly difficult either. |
|
Agreed we shouldn't make this change if it negatively impacts end-user experience. I'd like to take a deeper look at the x/tools failures though, before ruling out this change. My intuition is the defined receiver type information should still be available via the go/types API, just the consumer code isn't worrying about getting it, because in the common case (e.g., |
|
Agreed that there are scenarios (https://go.dev/play/p/weOZB-AO-w2) where the reported receiver type is not ideal. The "correct" solution might be to adjust the various tests that are failing. Postponing for now as this is neither urgent nor a major issue at the moment. |
|
Change https://go.dev/cl/421115 mentions this issue: |
|
Change https://go.dev/cl/421355 mentions this issue: |
For a type definition like `type I interface{ M() }`, the go/types API
traditionally sets `M`'s receiver parameter type to `I`, whereas
Unified IR was (intentionally) leaving it as `interface{ M() }`.
I still think `interface{ M() }` is the more consistent and
semantically correct type to use in this scenario, but I concede that
users want `I` instead, as evidenced by existing tooling and tests.
Updates #49906.
Change-Id: I74ba5e8b08e4e98ed9dc49f72b7834d5b552058b
Reviewed-on: https://go-review.googlesource.com/c/go/+/421355
Reviewed-by: David Chase <drchase@google.com>
Auto-Submit: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
|
Did the updating CL perhaps fix this? |
|
@dr2chase Not entirely. It brought the unified IR importer's behavior closer to what go/types currently implements itself, but there are still some minor inconsistencies. For example, in https://go.dev/play/p/weOZB-AO-w2 (the test case from above), importing the package from unified IR will now report I doubt the discrepancy affects users in practice, and it would be awkward to exactly emulate go/types's current behavior without bumping the unified IR export format. If necessary, we can certainly do that though. |
For a type definition like `type I interface{ M() }`, the go/types API
traditionally sets `M`'s receiver parameter type to `I`, whereas
Unified IR was (intentionally) leaving it as `interface{ M() }`.
I still think `interface{ M() }` is the more consistent and
semantically correct type to use in this scenario, but I concede that
users want `I` instead, as evidenced by existing tooling and tests.
Updates golang#49906.
Change-Id: I74ba5e8b08e4e98ed9dc49f72b7834d5b552058b
Reviewed-on: https://go-review.googlesource.com/c/go/+/421355
Reviewed-by: David Chase <drchase@google.com>
Auto-Submit: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
|
Change https://go.dev/cl/425365 mentions this issue: |
For "type T interface{ M() }", go/types users expect T's underlying
interface type to specify T as the receiver parameter type (#49906).
The unified importer handles this by cloning the interface to rewrite
the receiver parameters before calling SetUnderlying.
I missed in CL 425360 that these interfaces would need to have
Complete called too.
Manually tested to confirm that this actually fixes "go test -race
golang.org/x/tools/go/analysis/internal/checker" now (when both CLs
are ported to the x/tools importer).
Updates #54653.
Change-Id: I51e6db925db56947cd39dbe880230f14734ca01c
Reviewed-on: https://go-review.googlesource.com/c/go/+/425365
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
For "type T interface{ M() }", go/types users expect T's underlying
interface type to specify T as the receiver parameter type (golang#49906).
The unified importer handles this by cloning the interface to rewrite
the receiver parameters before calling SetUnderlying.
I missed in CL 425360 that these interfaces would need to have
Complete called too.
Manually tested to confirm that this actually fixes "go test -race
golang.org/x/tools/go/analysis/internal/checker" now (when both CLs
are ported to the x/tools importer).
Updates golang#54653.
Change-Id: I51e6db925db56947cd39dbe880230f14734ca01c
Reviewed-on: https://go-review.googlesource.com/c/go/+/425365
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
|
@mdempsky Is there anything left to do here? I think we want to keep pointing to the defined type for better error messages. |
|
No, I think we can close this. |
When setting up methods for interfaces, we set the recv to the interface type (its definition if we have one). We are (probably - I don't remember for sure) doing this so we can tell that those
Funcobjects are methods; there may be other reasons. Investigate if this is something we can or might want to change.See also #49888 and the respective CL which would be affected.
cc: rfindley
The text was updated successfully, but these errors were encountered: