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

MrGVSV commented 1 week ago

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 commented 1 week ago

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

nixpulvis commented 1 week ago

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 commented 1 week ago

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.

https://github.com/intellij-rust/intellij-rust/pull/9123 perhaps this may solve the issue?

SkiFire13 commented 1 week ago

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.

https://github.com/intellij-rust/intellij-rust/pull/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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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).

bevyengine / bevy