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, types2: always set the receiver type of interface methods to the defining interface type #49906

Closed
griesemer opened this issue Dec 1, 2021 · 16 comments
Assignees
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@griesemer
Copy link
Contributor

griesemer commented Dec 1, 2021

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 Func objects 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

@griesemer griesemer added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 1, 2021
@griesemer griesemer added this to the Backlog milestone Dec 1, 2021
@griesemer griesemer self-assigned this Dec 1, 2021
@mdempsky
Copy link
Member

mdempsky commented Feb 1, 2022

I'd like to suggest that the Func's receiver parameter always points to the types.Interface that represents the underlying interface type literal that declared the method, and never to the types.Named for defined interface types.

For example, I suggest https://go.dev/play/p/weOZB-AO-w2 should print:

func (interface).M()
func (interface).M()
func (interface).M()

rather than

func (p.A).M()
func (p.A).M()
func (interface).M()

like it does currently. I think this would be more consistent and also simplify importers.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/386003 mentions this issue: go/types: disable TestCheckExpr for unified IR

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/386004 mentions this issue: cmd/compile: switch to final unified IR export format

@griesemer griesemer modified the milestones: Backlog, Go1.19 Mar 29, 2022
@griesemer
Copy link
Contributor Author

Marking for Go 1.19 so this stays on the radar.

@griesemer griesemer changed the title go/types, types2: consider not setting the receiver type of interface methods go/types, types2: always set the receiver type of interface methods to the defining interface type Mar 29, 2022
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/396516 mentions this issue: types2: use declaring interface as receiver for interface methods

@griesemer
Copy link
Contributor Author

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 types2.

@findleyr Do you see any problems with tools (backward-compatibility)?

gopherbot pushed a commit that referenced this issue Apr 4, 2022
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>
@griesemer
Copy link
Contributor Author

We have now pending CL 396516 that implements the suggestion by @mdempsky , but for the failures in the x/tools repo. However, these failures give me pause: it does seem that we are going to lose quite a bit of information that's useful by tools when reporting errors. It's not clear to me that this is a change/simplification worth doing.

@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.

@mdempsky @findleyr for comments.

@mdempsky
Copy link
Member

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., type A interface { M() } from the https://go.dev/play/p/weOZB-AO-w2) it just works already. But for the same reason that the tests are failing for CL 396516, I suspect they'd similarly provide non-ideal end-user results for types B and C.

@griesemer
Copy link
Contributor Author

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.

@griesemer griesemer modified the milestones: Go1.19, Go1.20 May 17, 2022
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/421115 mentions this issue: [dev.unified] go/internal/gcimporter: rewrite interface receiver parameters

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/421355 mentions this issue: go/internal/gcimporter: rewrite interface receiver parameters

gopherbot pushed a commit that referenced this issue Aug 4, 2022
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>
@dr2chase
Copy link
Contributor

dr2chase commented Aug 4, 2022

Did the updating CL perhaps fix this?

@mdempsky
Copy link
Member

mdempsky commented Aug 4, 2022

@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 func (p.C).M() for the last line, rather than func (interface).M() like go/types still does.

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.

jproberts pushed a commit to jproberts/go that referenced this issue Aug 10, 2022
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>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/425365 mentions this issue: go/internal/gcimporter: call Complete on cloned Interfaces too

gopherbot pushed a commit that referenced this issue Aug 24, 2022
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>
rajbarik pushed a commit to rajbarik/go that referenced this issue Sep 1, 2022
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>
@griesemer
Copy link
Contributor Author

@mdempsky Is there anything left to do here? I think we want to keep pointing to the defined type for better error messages.

@mdempsky
Copy link
Member

No, I think we can close this.

@golang golang locked and limited conversation to collaborators Nov 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants