Closed VorpalBlade closed 1 month ago
It's a classic macro hygiene issue in strum
's expansion. strum
is generating an inherent method called get
:
impl AIter {
fn get(&self, idx: usize) -> ::core::option::Option<A> {
match idx {
0usize => ::core::option::Option::Some(A::B),
1usize => ::core::option::Option::Some(A::C),
_ => ::core::option::Option::None,
}
}
}
...and calling it without using the fully-qualified syntax; e.g.:
impl DoubleEndedIterator for AIter {
fn next_back(&mut self) -> ::core::option::Option<<Self as Iterator>::Item> {
let back_idx = self.back_idx + 1;
if self.idx + back_idx > 2usize {
self.back_idx = 2usize;
::core::option::Option::None
} else {
self.back_idx = back_idx;
self.get(2usize - self.back_idx)
}
}
}
If my understanding of method resolution is correct, rustc first searches for an applicable get
with a &mut self
receiver in the inherent impl of AIter
, then in the trait impls, and then — failing that — repeats the process for get
s with a &self
receiver. Consequently, the get(&mut self)
from Itertools
is resolved before the fn get(&self)
inherent method.
The fix is for strum
to use fully qualified invocations instead; e.g.:
AIter::get(self, 2usize - self.back_idx)
Fair enough, as a side note, get
there is a very generic name. Wouldn't having prefixed names or at least somewhat less common words be better? Then you would also avoid users getting warnings like this:
warning: a method with this name may be added to the standard library in the future
--> chezmoi_modify_manager/src/transforms.rs:62:18
|
62 | docs.intersperse("\n\n".to_string()).collect::<String>()
| ^^^^^^^^^^^
|
= warning: once this associated item is added to the standard library, the ambiguity may cause an error or change in behavior!
= note: for more information, see issue #48919 <https://github.com/rust-lang/rust/issues/48919>
= help: call with fully qualified syntax `itertools::Itertools::intersperse(...)` to keep using the current method
= note: `#[warn(unstable_name_collisions)]` on by default
The only way to get rid of that warning is to use a fully qualified form, which of course breaks method chaining (.a().b().c()...
). With that in mind it seems a bit odd to add new functions that risks running into that.
We opted for consistency with slice::get
. Since there is no risk of slices implementing Iterator
, there's no risk of a name collision risk there. There is some risk of a name collision with Iterator::get
(if it's ever added), but we did our due diligence by pitching the feature to the Rust libs team first (https://github.com/rust-lang/rust/issues/73482). Even so, if they do someday decide to adopt Iterator::get
, breakage there will be avoided by the implementation of https://github.com/rust-lang/rfcs/pull/3624.
Especially given that there is a language-level solution to the problem of super trait method collisions on the horizon, I think it's very unlikely that we'd take the step of prefixing the names of all itertools methods. That would be a tremendous breaking change to pass on to our users.
Feel free to close this issue (or maybe you want to keep it open for others to find over the next few days?) as there is nothing to do on the itertools side of thing.
Itertools::get(self)
(it takes ownership) and not get(&mut self)
but that probably does not change the result of method resolution (I'm not familiar with it though).
It's a classic macro hygiene issue in
strum
's expansion.
It would be nice if there was a clippy lint that would help authors of libraries like strum
with macro hygiene, suggesting to qualify calls (here to AIter::get
). Might be tricky to do though.
It would be nice if there was a clippy lint that would help authors of libraries like
strum
with macro hygiene, suggesting to qualify calls (here toAIter::get
). Might be tricky to do though.
Good idea, though this is the wrong place to pursue that. Either the rust issue tracker or https://internals.rust-lang.org would be appropriate places for this.
I hope you don't mind me adding to this. I don't think the following warrants a separate issue but I wanted to document this somewhere and figured this would be a good place if people come across this quirk:
Range
, RangeFrom
and RangeInclusive
implement Iterator
(and therefore Itertools
), as well as core::slice::SliceIndex
.
The latter has an unstable trait method get()
.
So this example also requires qualified paths:
#![feature(slice_index_methods)]
use core::slice::SliceIndex;
use itertools::Itertools;
fn main() {
let r = 0..10;
r.get(1..3);
// Disambiguation:
//Itertools::get(r, 1..3);
}
I do think it's slightly unfortunate. However, slice_index_methods
is unstable and my impression was that it's not really likely to get stabilized.
Yes it's a bit unfortunate. However, I heard that ranges are discussed to not be iterators anymore but only IntoIterator
for them to be Copy
in which case there would be no conflict in recent Rust.
EDIT: The fix has been merged in strum_macros 0.26.3. Unpinning this.
This leads to the very strange following error if the mentioned line is uncommented:
I have no idea if this is a strum or itertools bug. As such I have reported this to both, see https://github.com/Peternator7/strum/issues/358
Using rustc 1.78.0 (9b00956e5 2024-04-29).