Rename List::iter and friends to avoid auto-import conflicts

[MrGVSV]

What problem does this solve or what need does it fill?

It's common to have users write their Bevy code in an IDE that supports auto-import. This is not always helpful as sometimes the wrong trait is pulled in.

A common example of this is accidentally importing List from bevy_reflect when autocompleting .iter().

There are other methods this can happen with too, such as with List::get, Array::is_empty, etc.

What solution would you like?

We should consider making the method names more specific to reflection and/or their trait.

For example, List::iter could become List::iter_elements, List::reflect_iter, or List::list_iter.

We'd probably want to do a similar thing for other traits like Array, Map, and Set.

The main thing we'd need to work out is what the new naming convention should be and which traits/methods need it.

What alternative(s) have you considered?

• We could simply ignore this and just tell users to check their imports when they come across this problem
• We could possibly force users to fully qualify methods on these traits by replacing self with dyn Trait (e.g. my_vec.iter() becomes List::iter(&my_vec))

Ideally, the auto-importers would simply avoid pulling in a trait when the method exists on either the type or the type's deref target. Or even just provide a way for us to hide/discourage certain traits from being auto-imported.

Additional context

See also #15002 for an example of this being an issue.

[alice-i-cecile]

iter_elements is my preference. This should have a comment explaining why it was changed to avoid reversion.

[nixpulvis]

My issue with iter_elements is that it doesn’t help with naming other methods which could conflict. What about len and get, etc? Is something stopping those from being auto-imported incorrectly?

Prefixing everything with reflect_* is attractive to me for this reason, but still makes me think we’re trying too hard to help people fight their own tools.

I don’t use auto-importers, so perhaps I’m not the best case subject.

[entropylost]

I really feel like this is the wrong way of solving it, there should probably be some way to disable importing single traits on the auto importer, and harming the normal experience for this case just seems suboptimal.

intellij-rust/intellij-rust#9123 perhaps this may solve the issue?

[SkiFire13]

As an alternative to the reflect_ prefix we could use dyn_. I agree that iter_elements is not really a general fix since it doesn't cover all the other methods, and I would prefer changing all the methods the same way so it becomes intuitive what is the prefix to use. This would also help with autocomplete, since methods like iter_elements would still come up when autocompleting it, iter_ etc etc, but reflect_ are unique enough that they should be filtered out after a couple of characters.

intellij-rust/intellij-rust#9123 perhaps this may solve the issue?

Unfortunately that's an UI setting the user has to set, moreover this is intellij-rust only and doesn't have an equivalent for rust-analyzer, so I don't think this a reasonable solution for now. Maybe in the future, but IMO this issue needs a fix in the short term because I see lot of users being hit by it.

[alice-i-cecile]

Agreed: I think we should move forward with a rename. It's fine if a niche method gets a slightly less convenient name to avoid splash damage, especially for beginners.

[entropylost]

If you do the renaming, I feel like reflect_ would be better as it's more standardized (contrast to iter_elements just being different than everything else for no reason).

[MrGVSV]

As an alternative to the reflect_ prefix we could use dyn_

dyn_ is pretty good. I'm trying to think of other methods like Array::dyn_is_empty, Set::dyn_insert, etc.

Additionally, I think it might be better prefix for the subtraits (e.g. List, Array, Map, etc.) than reflect_ since they're not necessarily reflection-based operations (unlike Reflect::reflect_partial_eq which often uses reflection operations to perform its duties).

Of course, reflect_ isn't bad if we want to go with that instead. It's at the very least consistent with the rest of reflection.

One potential confusion is with methods like Struct::iter_fields, where it's not really a dynamic version of anything. It's a completely separate concept, and changing that to dyn_iter probably doesn't make sense.

Unfortunately that's an UI setting the user has to set, moreover this is intellij-rust only and doesn't have an equivalent for rust-analyzer, so I don't think this a reasonable solution for now. Maybe in the future, but IMO this issue needs a fix in the short term because I see lot of users being hit by it.

Yeah I don't think this should be something we have to code around. Unfortunately, it's been an issue for at least a year and it's likely users are going to keep running into it—especially newer users or those just learning Rust.

So while I'd rather we not do a rename, it does seem like we should (at least until there's some universal-ish way for a crate to opt-out of auto-importing certain paths).

[Veykril]

Fwiw rust-lang/rust-analyzer#18179 might be helping out here as well

[BenjaminBrienen]

rust-lang/rust-analyzer#18179 is merged

I think this solves this problem. Renaming the items seems to be controversial anyway.

[alice-i-cecile]

I like that RA PR, but it's not clear how Bevy uses it. Do we submit something to RA? Do users have to configure it? What about other language servers?

[BenjaminBrienen]

Looks like it would be a .vscode/settings.json setting (or the like) for devs using bevy.

[alice-i-cecile]

I don't think that's a good enough fix: this is the most dangerous for beginners, who won't have set up their environment for weird footguns like this.

[Veykril]

We are likely going to add tool attributes a la #[rust_analyzer::completions::ignore] or so that one could attach to items instead, but the main issue with that is that it needs a) design work for an extendable API and b) commitment to stability for the attributes. It would also not work for RustRover (or any other rust IDE) unfortunately (unless they follow up with the attributes)

[alice-i-cecile]

Makes sense :) As RA approaches stabilization for that, I'd really like if we could pull in the Rust project and coordinate on something upstream to ensure cross-tooling compatibility. Let me know when you're there and I'll help connect folks!

[almindor]

Just wanted to add anohter +1 to this as I hit this issue recently. Having auto-complete add the trait made it ever so much more confusing. See this gist if you want to see a nice "newbie" example :)

[Veykril]

Fyi, rust-lang/rust-analyzer#19375 will give you a mechanism to properly fix this (for rust-analyzer at least) without having to rename things. Its mainly stuck on initial design considerations (we won't be promising that the design won't change either way though)

[ChayimFriedman2]

FYI rust-lang/rust-analyzer#19375 is now merged, so you can put #[rust_analyzer::completions(ignore_flyimport_methods)] on the traits you want.