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

spec: allow embedding overlapping interfaces #6977

Closed
adonovan opened this issue Dec 17, 2013 · 99 comments
Closed

spec: allow embedding overlapping interfaces #6977

adonovan opened this issue Dec 17, 2013 · 99 comments
Labels
FrozenDueToAge LanguageChange NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Proposal Proposal-Accepted v2 A language change or incompatible library change
Milestone

Comments

@adonovan
Copy link
Member

If you view an interface as a set of constraints on the implementing type, then
combining two interfaces (that are not mutually incompatible) such as:

  type I interface { f(); String() string }
  type J interface { g(); String() string } 

has a natural interpretation that is equivalent to an interface containing the union of
such constraints.  e.g. these should be equivalent:

  type IJ interface { I; J }
  type IJ interface { f(); g(); String() string }

but in fact the first is an error: "duplicate method: String".  This is
somewhat surprising.  Is there any reason not to permit this?  The set-union behaviour
is easy to understand, describe and implement, and it seems useful in practise when you
have overlapping interfaces describing different aspects of a type.

(I chose String() string since I've seen many users add this constraint to their
interfaces.  It could be any method though.)
@robpike
Copy link
Contributor

robpike commented Dec 18, 2013

Comment 1:

Which String method gets called when I do
var x IJ // using first definition
fmt.Println(x.String())
The resolution of the ambiguity is why the second version of IJ works.

@adonovan
Copy link
Member Author

adonovan commented Jan 2, 2014

Comment 2:

But does that matter?  You could choose one arbitrarily and the effect would be the same.

@griesemer
Copy link
Contributor

Comment 3:

Labels changed: added release-none, languagechange.

Owner changed to @griesemer.

@leo-liu
Copy link

leo-liu commented Jan 24, 2014

Comment 4:

There's no ambiguity at all. IJ is a interface but not a struct, so no real method is
duplicated.

@griesemer
Copy link
Contributor

Comment 5:

Status changed to LongTerm.

@griesemer
Copy link
Contributor

Comment 6:

Labels changed: added repo-main.

@gopherbot
Copy link

Comment 7 by SRabbelier:

Note that even if you factor out the common method into its own interface it doesn't
work:
type C interface { String() string }
type CI interface { C; f()  }
type CJ interface { C; g() } 
type CIJ interface { CI; CJ }
http://play.golang.org/p/r5yOykMn-a
prog.go:8: duplicate method String

@griesemer
Copy link
Contributor

Comment 8:

Re: #7: you didn't factor out the common method in CIJ - both CI and CJ have the String
method and hence the same issue.
Factoring out a common method set will work.

@leventeliu
Copy link

Say struct SI implements I, struct SJ implements J, and struct SIJ implements IJ,

OI := struct SI{} 
OJ := struct SJ{}
foo := struct SIJ{
    I: OI,
    J: OJ,
}

In this case, GO doesn't know which String() is the right method to invoke.
But if you explictly implement the common method(s) of the interfaces, it works.

@griesemer
Copy link
Contributor

@geterns You're example is confusing and syntactically incorrect (did you mean SI := struct { I }, etc.?). Either way, the proposal is about overlapping interfaces and the resulting interface - structs are not related to this except that a struct may implement an interface. How that interface came to be is unrelated at that point.

@leventeliu
Copy link

@griesemer Yes, I'm sorry about the mistask. My example is about combining interfaces with duplicate method(s) in a struct - by explictly implementing the common method(s) of interfaces for the struct, it works. However, combining interfaces with duplicate method(s) to get a new interface is still not working :(
Thanks for your reminding, they're not the same case.

@griesemer
Copy link
Contributor

@geterns This is a long-term issue and a language change (although backward-compatible). It's unimportant (though not hard to implement) and thus likely won't be addressed anytime soon. Regarding your other comments about combining interfaces in a struct: embedding in structs is different from embedding in interfaces - conflicts are only reported if access is ambiguous in the struct case.

@jdef
Copy link

jdef commented Feb 29, 2016

I'd love to see this fixed. Interfaces is Go are pretty awesome and this is a fly in the ointment. I wonder what the original justification was for generating errors under these conditions? As long as the overlapping method signatures are identical I can't imagine why you'd ever want the compiler to flag this as an error.

@josharian
Copy link
Contributor

It's unimportant (though not hard to implement) and thus likely won't be addressed anytime soon.

FWIW, I just filed a dup (#15666) on behalf of a friend for whom this was causing considerable frustration.

@griesemer
Copy link
Contributor

Perhaps we should try to address this for 1.8. It seems pretty straightforward and incontroversial. Anybody having good counter arguments why this might be a mistake?

@hasty
Copy link

hasty commented May 13, 2016

Just to explain how this comes up in real life: I tend to abstract away the data layer from the business layer so a) I can easy make mocks for tests, and b) I can have flexibility in changing data providers, sharding data, etc. Typical interface might be:

package user

type Database interface {
    GetAccount(accountID uint64) (model.Account, error)
}

So then I have some other packages that want to be able to fetch accounts under some circumstances, so they say they require their Database to have all of user's Database methods:

package hardware

type Database interface {
    user.Database
    SaveDevice(accountID uint64, device model.Device) error
}
package wallet

type Database interface {
    user.Database
    ReadWallet(accountID uint64) (model.Wallet, error)
}

Then, I have some package that needs both of those packages, and its Database interface looks like:

package shopping

type Database interface {
    wallet.Database
    device.Database
    Buy(accountID uint64, deviceID uint64) error
}

And then, kablooey: Duplicate method GetAccount(accountID uint64) (model.Account, error)

@mdempsky mdempsky modified the milestones: Go1.8Maybe, Unplanned May 14, 2016
@awfm9
Copy link

awfm9 commented Aug 8, 2016

Would be neat if this was fixed, since it seems straight forward and non controversial. I run into it regularly and have to resort to copy-pasting method signatures.

@robpike
Copy link
Contributor

robpike commented Aug 9, 2016

I think it's OK but I find it all a bit confusing. The clarity of "no duplicates" is comforting.

@splace
Copy link

splace commented Aug 19, 2016

surely if two interfaces include the same sub-set of methods, that’s telling you there is a lower level 'thing' that should have its own interface, having this an error means smaller (flexible) interfaces, I’d have thought this was spot-on for Go, doesn't the problem come from inheritance thinking?

@mcandre
Copy link

mcandre commented Nov 7, 2019

But does that matter? You could choose one arbitrarily and the effect would be the same.

Type-equivalence does not always mean semantic equivalence. There are plenty of interfaces with additional behavior requirements that the compiler is not aware of. In a perfect world, all behavior would be specified in the type system, but this is not the case.

In a pretty good world, developers would register their interfaces through a common query system and immediately deprecate colliding interfaces for distinct interfaces. Maybe something like Hoogle for Go interfaces?

Meanwhile, a mechanism for disambiguation would be nice.

@mdempsky
Copy link
Member

mdempsky commented Nov 7, 2019

Leaving open for a week for final comments.

Something I thought of: what's the expected behavior when go 1.13 module A depends on go 1.14 module B, and B makes use of overlapping interfaces or changes its exported interfaces in a way that cause A to make use of them? Two specific cases:

  1. Is it okay for B to export a defined interface type that uses overlapping interfaces, and for A to consume that type? (I assume yes, though I think this currently doesn't work.)

  2. Initial state: module B exports type X interface{}; type Y interface{} and module A has type I interface { B.X; B.Y }. If B upgrades to go 1.14 and adds M() to both B.X and B.Y, should module A build even if it stays on go 1.13? (I'm leaning towards no, but not sure.)

@bcmills
Copy link
Contributor

bcmills commented Nov 7, 2019

  1. Is it okay for B to export a defined interface type that uses overlapping interfaces, and for A to consume that type?

It should be, yes: since the method sets are identical and the go version for B permits overlapping interfaces, it should not be a breaking change for the maintainer of B to replace

interface FooBarrer {
	Foo()
	Bar()
}
interface BarBagger {
	Bar()
	Bag()
}
interface FooBarBagger {
	Foo()
	Bar()
	Bag()
}

with

interface FooBarrer {
	Foo()
	Bar()
}
interface BarBagger {
	Bar()
	Bag()
}
interface FooBarBagger {
	FooBarrer
	BarBagger
}

@bcmills
Copy link
Contributor

bcmills commented Nov 7, 2019

  1. […] If B upgrades to go 1.14 and adds M() to both B.X and B.Y, should module A build even if it stays on go 1.13? […]

No. The package combining the two interfaces is A, and the go version for A does not permit that package to embed overlapping interfaces.

The maintainer of B has made a breaking change by adding a method to an exported interface, so the fact that this breaks A should not come as a surprise.

Specifically, the change to either interface would also break A if they had previously defined:

type I interface {
	B.X
	B.Y
	M() someOtherType
}

@mdempsky
Copy link
Member

mdempsky commented Nov 7, 2019

What about:

package a // go 1.14

type a1 { interface A() }
type a2 { interface A() }
type A = interface { a1; a2 }
package b // go 1.13

import "./a"

var _ a.A

@bcmills
Copy link
Contributor

bcmills commented Nov 7, 2019

That specific example won't work, because the go 1.13 directive is in the go.mod file and relative imports aren't allowed in module mode. 😛

But putting that aside, the var _ a.A should be allowed, because the package that actually combines the two interfaces is a, not b.

@mdempsky
Copy link
Member

mdempsky commented Nov 7, 2019

That specific example won't work, because the go 1.13 directive is in the go.mod file and relative imports aren't allowed in module mode. stuck_out_tongue

Ack. I don't understand Go modules well enough to know how to concisely write a test case that uses them directly, so I resorted to pseudocode.

But putting that aside, the var _ a.A should be allowed, because the package that actually combines the two interfaces is a, not b.

This is my inclination as well.

@mdempsky
Copy link
Member

mdempsky commented Nov 8, 2019

I've submitted b7d097a. As of master, cmd/compile applies these rules for overlapping interfaces:

  • An interface type literal is allowed to embed overlapping interfaces if its source form appears in a package compiled with -lang=go1.14 or newer.
  • Between packages, types are only distinguishable up to identity. Since for any interface type literal that uses overlapping interfaces, there's always an identical type literal that does not require overlapping interfaces, importers are unaffected by those internal choices of the other package.

@rsc
Copy link
Contributor

rsc commented Nov 13, 2019

No objections raised, so accepting.

@rsc rsc changed the title proposal: spec: allow embedding overlapping interfaces spec: allow embedding overlapping interfaces Nov 13, 2019
@rsc
Copy link
Contributor

rsc commented Nov 13, 2019

Already implemented, so closing.

@gopherbot
Copy link

Change https://golang.org/cl/214240 mentions this issue: compiler: permit duplicate methods from embedded interfaces

gopherbot pushed a commit to golang/gofrontend that referenced this issue Jan 10, 2020
This is a language change for Go 1.14.

Updates golang/go#6977

Change-Id: Ia8b8dc446c4dd700caccf59ca0444a380c49ba15
Reviewed-on: https://go-review.googlesource.com/c/gofrontend/+/214240
Reviewed-by: Than McIntosh <thanm@google.com>
marxin pushed a commit to marxin/gccold that referenced this issue Jan 10, 2020
    
    This is a language change for Go 1.14.
    
    Updates golang/go#6977
    
    Reviewed-on: https://go-review.googlesource.com/c/gofrontend/+/214240


git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@280109 138bc75d-0d04-0410-961f-82ee72b054a4
kraj pushed a commit to kraj/gcc that referenced this issue Jan 11, 2020
@gopherbot
Copy link

Change https://golang.org/cl/216997 mentions this issue: doc/go1.14: document overlapping interfaces change (update release notes)

gopherbot pushed a commit that referenced this issue Jan 30, 2020
…tes)

Updates #6977.
Updates #36878.

Change-Id: I40594be85ee0a0d4b35bacc90104568d2b8a4761
Reviewed-on: https://go-review.googlesource.com/c/go/+/216997
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/217134 mentions this issue: go/types: unexport Checker.LookupFieldOrMethod

gopherbot pushed a commit that referenced this issue Jan 31, 2020
Implementation changes in go/types for #6977 required that internal
LookupFieldOrMethod calls had access to the current *Checker. In
order to make quick progress, I added a *Checker receiver to the
function LookupFieldOrMethod (thus making it a method), and added
a new function LookupFieldOrMethod. The plan was always to rename
that function (Checker.LookupFieldOrMethod) such that it wouldn't
be exported; with the obvious name being Checker.lookupFieldOrMethod.
But that name was already in use which is why I postponed the rename.
Eventually I forgot to clean it up. This CL fixes that with the
following renames:

Checker.lookupFieldOrMethod => Checker.rawLookupFieldOrMethod
Checker.LookupFieldOrMethod => Checker.lookupFieldOrMethod

Updates #6977.
Fixes #36916.

Change-Id: Icfafd0de9a19841ba5bd87142730fe7323204491
Reviewed-on: https://go-review.googlesource.com/c/go/+/217134
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@golang golang locked and limited conversation to collaborators Feb 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge LanguageChange NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Proposal Proposal-Accepted v2 A language change or incompatible library change
Projects
None yet
Development

No branches or pull requests