Remove IntoIterator bound from KeyedSubfield#4605
Open
tysen wants to merge 1 commit intoleptos-rs:mainfrom
Open
Remove IntoIterator bound from KeyedSubfield#4605tysen wants to merge 1 commit intoleptos-rs:mainfrom
tysen wants to merge 1 commit intoleptos-rs:mainfrom
Conversation
The `KeyedSubfield` struct previously had a struct-level bound:
```
pub struct KeyedSubfield<Inner, Prev, K, T>
where
for<'a> &'a T: IntoIterator,
{ ... }
```
This causes infinite trait resolution when used alongside, for example, `bevy_ecs`.
The root cause is that `bevy_ecs` has a blanket implementation:
```
impl<'w, 's, T: Resource> IntoIterator for &Res<'w, 's, T> { ... }
```
During compilation we end up recursing infinitely checking `Res<..>`,
`Res<Res<..>>`, `Res<Res<Res<..>>>` and so on.
This commit replaces that bound w/ a new trait `KeyedIterable` and moves
the `IntoIterator` bound to the impl blocks where it's actually needed.
The downside of this is that `reactive_stores` must now explicitly impl
this trait for supported collections, so support for 3rd-party
collections (e.g. `DashMap`) will need to be added here as requested
(because of the orphan rule).
Contributor
Author
|
Fixes #4612 |
Collaborator
|
I think I'm fine with this in principle, although I haven't read through the PR in detail. In practice I think we already need support within the crate for collection types to be used as KeyedSubfield because we need KeyedAccess implemented anyway. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
KeyedSubfieldstruct previously had a struct-level bound:This causes infinite trait resolution when used alongside, for example,
bevy_ecs. The root cause is thatbevy_ecshas a blanket implementation:During compilation we end up recursing infinitely checking
Res<..>,Res<Res<..>>,Res<Res<Res<..>>>and so on.This commit replaces that bound w/ a new trait
KeyedIterableand moves theIntoIteratorbound to the impl blocks where it's actually needed.The downside of this is that
reactive_storesmust now explicitly impl this trait for supported collections, so support for 3rd-party collections (e.g.DashMap) will need to be added here as requested (because of the orphan rule).This is of course a big breaking change for this crate. I think it's warranted as bevy is pretty popular, there is plenty of bevy + leptos interest (see 1, 2), and this problem is not unique to
bevy_ecsbut would happen when combiningreactive_storesw/ any crate that has a blanketIntoIteratorimpl on a wrapper type (likeRes).Were this to be merged, impls of
KeyedIterablefor 3rd party collections will have to live here, gated behind feature flags. This is a fairly common practice but if that's too big of a change and/or you'd rather not, feel free to close aswontfixand I'll keep using my fork.