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 · 97 comments

Comments

@adonovan
Copy link

@adonovan adonovan commented Dec 17, 2013

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Author

@adonovan adonovan commented Jan 2, 2014

Comment 2:

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

This comment has been minimized.

Copy link
Contributor

@griesemer griesemer commented Jan 8, 2014

Comment 3:

Labels changed: added release-none, languagechange.

Owner changed to @griesemer.

@leo-liu

This comment has been minimized.

Copy link

@leo-liu 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

This comment has been minimized.

Copy link
Contributor

@griesemer griesemer commented Feb 27, 2014

Comment 5:

Status changed to LongTerm.

@griesemer

This comment has been minimized.

Copy link
Contributor

@griesemer griesemer commented Apr 21, 2014

Comment 6:

Labels changed: added repo-main.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented May 21, 2014

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

This comment has been minimized.

Copy link
Contributor

@griesemer griesemer commented May 21, 2014

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

This comment has been minimized.

Copy link

@leventeliu leventeliu commented Jun 30, 2015

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

This comment has been minimized.

Copy link
Contributor

@griesemer griesemer commented Jun 30, 2015

@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

This comment has been minimized.

Copy link

@leventeliu leventeliu commented Jul 1, 2015

@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

This comment has been minimized.

Copy link
Contributor

@griesemer griesemer commented Jul 1, 2015

@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

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Contributor

@josharian josharian commented May 12, 2016

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

This comment has been minimized.

Copy link
Contributor

@griesemer griesemer commented May 13, 2016

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

This comment has been minimized.

Copy link

@hasty 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
@awishformore

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link

@splace 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?

t4n6a1ka added a commit to t4n6a1ka/go that referenced this issue Sep 5, 2019
Updates golang#6977.

Change-Id: I6eda4be550e7c7ea1e1bac3222850002d90a81a8
Reviewed-on: https://go-review.googlesource.com/c/go/+/190378
Reviewed-by: Rob Pike <r@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
t4n6a1ka added a commit to t4n6a1ka/go that referenced this issue Sep 5, 2019
Move checkdupfields call into expandiface, and inline/simplify offmod.
More prep work for implementing golang#6977.

Change-Id: I958ae87f761ec25a8fa7298a2a3019eeca5b25ba
Reviewed-on: https://go-review.googlesource.com/c/go/+/187518
Reviewed-by: Robert Griesemer <gri@golang.org>
t4n6a1ka added a commit to t4n6a1ka/go that referenced this issue Sep 5, 2019
Quietly drop duplicate methods inherited from embedded interfaces if
they have an identical signature to existing methods.

Updates golang#6977.

Change-Id: I144151cb7d99695f12b555c0db56207993c56284
Reviewed-on: https://go-review.googlesource.com/c/go/+/187519
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
@mikeweilgart

This comment has been minimized.

Copy link

@mikeweilgart mikeweilgart commented Sep 30, 2019

Reading through this entire thread, it seems to me that the proposed solution is simple enough, but the problems it seeks to solve are somewhat contrived. (Which is to say, some of the examples which have been presented in favor of this proposal seem to me personally to result from earlier engineering decisions in need of repair, rather than from the lack of the proposed language feature/behavior.)

The idea of being able to compose interfaces without worrying about overlapping methods, providing the signatures match, does seem to be a good one.

Below I'll respond to some of the examples that have been given.


First, replying to @tombergan 's comment (not a real example) #6977 (comment)

...consider:

package p
type A interface {
  Foo()
}

package q
type B interface {
  Bar()
}

package main
import ("p", "q")
type C interface {
  p.A
  q.B
}

Then someone changes p:

package p
type A interface {
+  Bar()  // incompatible with B.Bar()
   Foo()
}

Now a change to an imported package has silently made the definition of C ambiguous.

I wish to point out that adding an additional method to an exported interface is by definition backward incompatible, so this could not possibly happen "silently" no matter how overlapping interfaces were handled. Package p must have changed major versions in order to add a new method to an exported interface.

Actually, I can't imagine a single real-world scenario that would ever call for the expansion of an exported interface. Imagine if fmt.Stringer had a method added. It wouldn't matter what that method were. Even if it were DoNothing() with no return value, it would break all code that used fmt.Stringer. Adding Bar() to p.A above is comparable. Surely the very point of defining interface types in the first place is to avoid the need for such breakage; you can instead define additional methods on concrete types without modifying the interface types fulfilled by existing code. If you have ever encountered a real-world case where an exported interface type had to be expanded after it was already in use by other packages/developers, I would be very interested to see it.

(Besides which, any concrete type fulfilling main.C could only have one Bar() method defined on it regardless of the contents of p and q, so there would never be any ambiguity anyway. But I think someone else pointed that out already.)


Regarding @npc-g 's database use case here: #6977 (comment)

If there are multiple database implementations each with methods unique to those databases, and you want to deal with a single interface which only contains the methods in common between the database, then why not use an interface type that only contains the common methods (and define it centrally)?

If instead you have an interface type in each separate database package (as you illustrate), then by definition you must have all of those methods to fulfill any particular DB interface type, and if you were to define a single interface as a UNION of all those interface types (as your code snippet would appear to illustrate), then you would have to implement ALL the methods from ALL the packages' DB interface types before you would have any single concrete type which would fulfill the union interface (initsrv.DB in your example). So, I don't see how that could possibly work, even with the proposed feature, because none of the DB types from the other packages would actually fulfill initsrv.DB.

You seem to want an intersection between method sets, rather than a union, which is not what is discussed in this proposal at all. Please let me know if I have misunderstood your example.


Regarding @svenkanna 's example here: #6977 (comment)

Despite the appearance, this example is not actually an illustrative real-world use case, both because of the peculiar interface type names (which do not clearly communicate their purpose—not illustrative) and especially because the overlapping part of the method sets, interface type IOrder, is shown as the empty interface (therefore there would be no conflict from any method name).

@networkimprov

This comment has been minimized.

Copy link

@networkimprov networkimprov commented Sep 30, 2019

I can't imagine a single real-world scenario that would ever call for the expansion of an exported interface.

I think you're imagining library packages, meant for import by numerous, unknown clients. Most packages in the wild have very few clients, and are confined to a single project.

@svenkanna

This comment has been minimized.

Copy link

@svenkanna svenkanna commented Oct 2, 2019

Despite the appearance, this example is not actually an illustrative real-world use case, both because of the peculiar interface type names (which do not clearly communicate their purpose—not illustrative) and especially because the overlapping part of the method sets, interface type IOrder, is shown as the empty interface (therefore there would be no conflict from any method name).

The example is related to order management domain. IOrder is not an empty interface in the real use case. You can imagine IOrder definition something like the below:

type IOrderItem interface {
Product() IProduct
Quantity() float64
Rate() floatt64
}
type IOrder interface{
ID() int64
Items() []IOrderItem
}

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 10, 2019

Change https://golang.org/cl/200221 mentions this issue: design/6977-overlapping-interfaces.md: various updates

@griesemer

This comment has been minimized.

Copy link
Contributor

@griesemer griesemer commented Oct 10, 2019

This is a bit late, but @adonovan the example you have given (or at least the explanation for the need of overlapping interfaces) is not very strong: @adg has pointed out in a code review that one could easily solve the problem you're trying to address with a sequence of type assertions.

Anyway, I have removed that example again from the design doc in favor of this one which seems a bit stronger (see https://golang.org/cl/200221).

gopherbot pushed a commit to golang/proposal that referenced this issue Oct 11, 2019
- Updated proposed spec changes to match current spec changes at tip.
- Added update on implementation status.
- Per comments in https://go-review.googlesource.com/c/proposal/+/188197,
  revised larger example at the end.
- Also, added some more small examples illustrating basic behavior
  and corner cases.

Updates golang/go#6977.

Change-Id: Ie3906e1a7379b0b15714512e4cafa15c98b22a11
Reviewed-on: https://go-review.googlesource.com/c/proposal/+/200221
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@griesemer

This comment has been minimized.

Copy link
Contributor

@griesemer griesemer commented Oct 14, 2019

@skyscooby

This comment has been minimized.

Copy link

@skyscooby skyscooby commented Oct 16, 2019

@griesemer looking at L157 here: https://go-review.googlesource.com/c/proposal/+/200221/3/design/6977-overlapping-interfaces.md#157

I think this should say package device based on device.Database being aggregated in shopping.Database

@griesemer

This comment has been minimized.

Copy link
Contributor

@griesemer griesemer commented Oct 16, 2019

@skyscooby Indeed. Looks like device and hardware are used instead of just one of them in the text. Will fix. Thank you.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Nov 6, 2019

As a reminder, we introduced a new process for these Go 2-related language changes in our blog post https://blog.golang.org/go2-here-we-come. The Go 1.14 development cycle is now over and it’s time for the final decision.

The feedback here has been almost entirely positive. For example, see the emoji on #6977 (comment).

@adg asked for stronger examples, which have been added to the design doc.

Given the uniformly positive feedback and how small a change this is, it seems like this is a likely accept for Go 1.14. Leaving open for a week for final comments.

If accepted, we'll just need to close the issue - the implementation is landed already per the language change process.

- rsc for proposal review

@mcandre

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Nov 13, 2019

Already implemented, so closing.

@rsc rsc closed this Nov 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.