8000 Permit Multibinds with MembersInjector values by damianw · Pull Request #4459 · google/dagger · GitHub
[go: up one dir, main page]
Skip to content

Repository navigation

Permit Multibinds with MembersInjector values - #4459

Closed
damianw wants to merge 1 commit into
google:masterfrom
damianw:allow-multibinds-membersinjector
Closed

Permit Multibinds with MembersInjector values#4459
damianw wants to merge 1 commit into
google:masterfrom
damianw:allow-multibinds-membersinjector

Conversation

@damianw
@damianw damianw commented Sep 25, 2024
Copy link
Copy Markdown

The MultibindsMethodValidator prevents Multibinds methods from using Producer, Produced, Lazy, Provider, and MembersInjector as a Map value type or a Set element type. However, out of all of these, MembersInjector does not actually produce or access any instances; instead, it operates on an instance provided as an argument. Therefore, using it as a Map or Set value type is actually quite different than any of the rest.

It's actually already possible multi-bind a Set or Map with MembersInjector values, but not to declare such a binding with Multibinds. This PR changes the validator to allow such binding declarations, and adds additional tests to verify (and demonstrate) that this works as intended.

@Chang-Eric
Chang-Eric commented Oct 10, 2024
Copy link
Copy Markdown
Member

Thanks for the PR and sorry for the delay here. Looking into this, I think we're actually just completely missing validation on the @IntoSet and @IntoMap types. I tried doing an @Provides @IntoMap that returns a Provider and it worked (or got farther than it should have), even though it really shouldn't be allowed.

I think it should be okay to allow MembersInjector though for maps, so what I'll do is I'll merge your PR in for the tests as disabled and then probably send a follow up to add the checking and enablement in?

@damianw
damianw commented Oct 10, 2024
Copy link
Copy Markdown
Author

Sounds great, thanks! Appreciate your looking into it.

copybara-service Bot pushed a commit that referenced this pull request Oct 21, 2024
Damian Wieczorek Dagger Team
Permit @Multibinds with values that are also allowed by @IntoSet/@Int…
6882f52 
…omap.

Closes #4459.

RELNOTES=Permit @Multibinds with values that are also allowed by @IntoSet/@Intomap.
PiperOrigin-RevId: 684189777
@copybara-service copybara-service Bot mentioned this pull request Oct 21, 2024
Merged
@Chang-Eric
Chang-Eric commented Oct 22, 2024
Copy link
Copy Markdown
Member

Also note the preceding change 15a30ca that adds validation for @IntoSet/@IntoMap bindings

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@damianw @Chang-Eric
0