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

Consider a mechanism to permit automatically binding a type to interfaces it implements #242

Open
bradleypeabody opened this issue Mar 14, 2020 · 10 comments
Labels
enhancement New feature or request needs info Further discussion or clarification is necessary

Comments

@bradleypeabody
Copy link

bradleypeabody commented Mar 14, 2020

Is your feature request related to a problem? Please describe.

If I am wiring a complex application which uses interfaces to describe dependencies, it is cumbersome to have to provide wire.Bind to describe mappings from interface to implementation even when the method names are clearly designed to be unambiguous.

If I have a LoginController which needs a TryLogin(u, p string) error method, there is only going to be one type in the system which implements such a call and it will be available via some function in wire.Build(...). I don't see why we need a wire.Bind to indicate what implements that TryLogin method when there is only one.

Describe the solution you'd like

For interfaces and implementations that are unambiguous (there is only one concrete type from the provider list that matches the interface being resolved), a wire.Bind should not be needed. I understand the use of wire.Bind to clarify ambiguous matches but requiring it in all cases adds tedium for no benefit.

Describe alternatives you've considered

I don't see any alternative other than writing out wire.Bind for each mapping, which is the tedium I'm trying to avoid.

Additional context

The Go language itself resolves interfaces based solely on method signatures, not an explicit implements mechanism - I don't see why wire wouldn't follow the same behavior. wire.Bind can still be used to resolve ambiguous cases.

Example

Basically, I feel like this example should just work:

type LoginStore interface {
	TryLogin(u, p string) error
}

type LoginCtrl struct {
	LoginStore LoginStore
}

func NewLoginCtrl(store LoginStore) *LoginCtrl { return &LoginCtrl{LoginStore: store} }

type UserDataStore struct{}

// TryLogin makes UserDataStore implement LoginStore
func (uds *UserDataStore) TryLogin(u, p string) error { panic("TODO") }

func NewUserDataStore() *UserDataStore { return &UserDataStore{} }

// ... {
	wire.Build(NewUserDataStore, NewLoginCtrl)
// }

If there is agreement on the feature I can dig into the code and start seeing what it would take to put together a PR.

@zombiezen
Copy link
Collaborator

Thanks for the report, and I'm sorry to hear it's been a source of friction for you.

I do think the explicitness adds benefit, but we may not have communicated the benefit well (might warrant an FAQ). The reason the binding is explicit is to avoid scenarios where adding a new type to the provider graph that implements the same interface causes the graph to break, because that can be surprising. While this does result in more typing, the end-effect is that the developer's intent is more explicit in the code, which we felt was most consistent with the Go philosophy.

I'm closing this issue out because we won't accept PRs for your proposal as written, but if you have other ideas on how to reduce tedium, please feel free to open a new issue or ask questions here.

@bradleypeabody
Copy link
Author

Thanks @zombiezen, I get where you are coming from. It is certainly a trade-off and I certainly get that adding items to a wire set causing other earlier items to fail could be unexpected. The only thing I would point out is that Go does something very similar regarding method name matching when embedding structs: You can call T.B() and B can be on an embedded struct as long as it's unambiguous; adding another embed to T with another B method will cause previously working T.B() references to fail. I do understand the scope is different as we're talking about structs and code that references these directly whereas in the topic of this issue interfaces will tend to connect more disparate parts of code.

But fair enough, if this has already been thought through and decided then I understand.

I have an alternate proposal, which is to allow this feature optionally through something like: wire.AutoBind(new(LoginStore)). In this case it flags LoginStore as a candidate for automatically satisfying interfaces. This would not change the behavior of any existing code but would allow people who wanted automatic interface matching to use it. It puts the decision in the hands of the wirer.

What do you think? Is that a possibility?

@zombiezen
Copy link
Collaborator

I think that would work, but would likely need to take lower precedence than explicit bindings. My instinct here is that there may be some complexity for larger provider graphs, but the explicit AutoBind marker would make it easier for static analysis to detect the behavior is occurring.

In the interest of trying to set realistic expectations and unblocking you, you should know that I and the other Wire maintainers are constrained on bandwidth right now due to the COVID-19 situation. I encourage you to prototype and experiment with this idea on a fork, but I won't have time to review over the next few weeks. Hearing your experience with the experiment in a realish-world scenario would be helpful in weighing whether or not we upstream this. Does that work for you?

@bradleypeabody
Copy link
Author

Okay great.

  • Agreed on the lower precedence - explicit bindings should always be honored.
  • Understood on the possible difficulties. I'm willing to give it a shot.
  • Completely understood on the timeline and no worries. I'm in no rush at all, I just mainly wanted to ensure that if I do work on this that there's a reasonable possibility it would get merged. I think we're good on that for now. I'll fork and get together a prototype of this feature when I can and let you know.

@bradleypeabody
Copy link
Author

Would you be okay with re-opening this issue? Or should I just make a PR when I have one and we can do any further discussion around the actual code?

@zombiezen zombiezen changed the title Do not require wire.Bind if interface to implementation matching is unambiguous Consider a mechanism to permit automatically binding a type to interfaces it implements Mar 28, 2020
@zombiezen zombiezen added enhancement New feature or request needs info Further discussion or clarification is necessary labels Mar 28, 2020
@zombiezen
Copy link
Collaborator

Retitled to match new scope and reopened.

@zombiezen zombiezen reopened this Mar 28, 2020
@ProximaB
Copy link

ProximaB commented Sep 4, 2020

@bradleypeabody How it's going? 😄

@bradleypeabody
Copy link
Author

Unfortunately I have not yet gotten to this. I was hoping to run into a specific case where I need it for Vugu (https://github.com/vugu/vugu but I haven't run into it again, yet). This is definitely still on my radar and I hope to look into it further soon.

@dabbertorres
Copy link

@bradleypeabody have you gotten to this by chance? If not, would you mind if I shared my attempt at adding this feature?

@bradleypeabody
Copy link
Author

@dabbertorres I have not had a chance to work on this. Definitely no objection to someone else taking it on. I'll try to provide feedback/suggestions to whatever extent possible/needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs info Further discussion or clarification is necessary
Projects
None yet
Development

No branches or pull requests

4 participants