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

Allow FieldsOf to provide a pointer type #208

Closed
alethenorio opened this issue Aug 26, 2019 · 5 comments · Fixed by #209 or #210
Closed

Allow FieldsOf to provide a pointer type #208

alethenorio opened this issue Aug 26, 2019 · 5 comments · Fixed by #209 or #210
Assignees

Comments

@alethenorio
Copy link

alethenorio commented Aug 26, 2019

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

I am trying to wire a non-pointer field from a struct to a provider which requires a pointer of the same type

Consider the following

type Config struct {
    UserConfig UserConfig
}

func initialize(ctx context.Context, cfg *Config) (*st, func(), error) {
    panic( // panic to be replaced by generated code
        wire.Build(
            InitializeUser,
            wire.FieldsOf(new(Config), "UserConfig"),
        ),
    )
}

func InitializeUser(config *UserConfig) *User {
    // initialize user from user config
}

I am unable to wire the UserConfig field in my Config for the InitializeUser provider without writing a wrapper provider by hand.

Describe the solution you'd like

FieldsOf should provide both a non-pointer and a pointer type of that field. This seems to already be done today by the Struct Providers as per the documentation. It would be useful (and consistent) if FieldsOf would do the same

Describe alternatives you've considered

The alternative it to write your own simple wrapper provider such as

func InitializeUserConfig(cfg *Config) *UserConfig {
   return &cfg.UserConfig
}
@vangent
Copy link
Contributor

vangent commented Aug 26, 2019

Seems reasonable to me.

One option would be to make FieldsOf always provide both the base field type and a pointer to it.
This is simpler to use, but might result in type conflicts, i.e., type Foo was provided by a FieldsOf before, and *Foo was provided from somewhere else, now FieldsOf provides both and results in an error. This seems unlikely but possible.

Another option would be to add some syntax, e.g., wire.FieldsOf(new(Config), "&UserConfig") to provide a *UserConfig instead of a UserConfig.

@shantuo @zombiezen thoughts?

@vangent vangent self-assigned this Aug 26, 2019
@alethenorio
Copy link
Author

The issue with FieldsOf providing a pointer which is provided somewhere else is no different than attempting to provide Foo both through a FieldsOf provider and a "normal" provider right?
Coincidentally enough I also tried using the "&UserConfig" notation when trying to figure out why I wouldn't get a pointer with no luck so the idea is valid as well

@shantuo
Copy link
Contributor

shantuo commented Aug 27, 2019

Option 1) SGTM, which is what we do in the struct provider now.

@zombiezen
Copy link
Collaborator

(Sorry, meant to get to this over the weekend but I fell ill.)

I think either of the options is fine, but I think that is important that the pointer variant is only allowed if the provided struct is of a pointer type. So this would work:

// Provides *UserConfig given a *Config
wire.FieldsOf(new(*Config), "UserConfig")

but this shouldn't:

// Does not provide *UserConfig.
// Provides a UserConfig given a Config.
wire.FieldsOf(new(Config), "UserConfig")

Reason being, it would be incorrect for the address of the field to be nondeterministic (addressing from a temporary), versus having a stable address expression.

@vangent
Copy link
Contributor

vangent commented Sep 4, 2019

Good point @zombiezen . I'll send a PR to limit the pointer-to-field to cases where the struct type is a pointer to a struct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment