Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add track_caller to iterator methods #102024

Open
xxchan opened this issue Sep 19, 2022 · 5 comments
Open

Add track_caller to iterator methods #102024

xxchan opened this issue Sep 19, 2022 · 5 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-error-handling Area: Error handling A-iterators Area: Iterators C-discussion Category: Discussion or questions that doesn't represent real issues. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@xxchan
Copy link
Contributor

xxchan commented Sep 19, 2022

I'm trying to add track_caller to Itertools::zip_eq, but:

zip_eq(&a, &b).count();

reports Iterator::fold

thread 'zip_eq_panic1' panicked at 'itertools: .zip_eq() reached end of one iterator before the other', /rustc/9d1b2106e23b1abd32fce1f17267604a5102f57a/library/core/src/iter/traits/iterator.rs:2170:34
stack backtrace:
   0: std::panicking::begin_panic
             at /rustc/9d1b2106e23b1abd32fce1f17267604a5102f57a/library/std/src/panicking.rs:525:12
   1: <itertools::zip_eq_impl::ZipEq<I,J> as core::iter::traits::iterator::Iterator>::next
             at ./src/zip_eq_impl.rs:51:13
   2: core::iter::traits::iterator::Iterator::fold
             at /rustc/9d1b2106e23b1abd32fce1f17267604a5102f57a/library/core/src/iter/traits/iterator.rs:2170:29
   3: core::iter::traits::iterator::Iterator::count
             at /rustc/9d1b2106e23b1abd32fce1f17267604a5102f57a/library/core/src/iter/traits/iterator.rs:201:9
   4: zip::zip_eq_panic1
             at ./tests/zip.rs:66:5
   5: zip::zip_eq_panic1::{{closure}}
             at ./tests/zip.rs:61:1
   6: core::ops::function::FnOnce::call_once
             at /rustc/9d1b2106e23b1abd32fce1f17267604a5102f57a/library/core/src/ops/function.rs:227:5
   7: core::ops::function::FnOnce::call_once
             at /rustc/9d1b2106e23b1abd32fce1f17267604a5102f57a/library/core/src/ops/function.rs:227:5
zip_eq(&a, &b).collect_vec();

reports alloc::vec::...

thread 'zip_eq_panic1' panicked at 'itertools: .zip_eq() reached end of one iterator before the other', /rustc/9d1b2106e23b1abd32fce1f17267604a5102f57a/library/alloc/src/vec/mod.rs:2638:44
stack backtrace:
   0: std::panicking::begin_panic
             at /rustc/9d1b2106e23b1abd32fce1f17267604a5102f57a/library/std/src/panicking.rs:525:12
   1: <itertools::zip_eq_impl::ZipEq<I,J> as core::iter::traits::iterator::Iterator>::next
             at ./src/zip_eq_impl.rs:51:13
   2: alloc::vec::Vec<T,A>::extend_desugared
             at /rustc/9d1b2106e23b1abd32fce1f17267604a5102f57a/library/alloc/src/vec/mod.rs:2638:35
   3: <alloc::vec::Vec<T,A> as alloc::vec::spec_extend::SpecExtend<T,I>>::spec_extend
             at /rustc/9d1b2106e23b1abd32fce1f17267604a5102f57a/library/alloc/src/vec/spec_extend.rs:18:9
   4: <alloc::vec::Vec<T> as alloc::vec::spec_from_iter_nested::SpecFromIterNested<T,I>>::from_iter
             at /rustc/9d1b2106e23b1abd32fce1f17267604a5102f57a/library/alloc/src/vec/spec_from_iter_nested.rs:37:9
   5: <alloc::vec::Vec<T> as alloc::vec::spec_from_iter::SpecFromIter<T,I>>::from_iter
             at /rustc/9d1b2106e23b1abd32fce1f17267604a5102f57a/library/alloc/src/vec/spec_from_iter.rs:33:9
   6: <alloc::vec::Vec<T> as core::iter::traits::collect::FromIterator<T>>::from_iter
             at /rustc/9d1b2106e23b1abd32fce1f17267604a5102f57a/library/alloc/src/vec/mod.rs:2541:9
   7: core::iter::traits::iterator::Iterator::collect
             at /rustc/9d1b2106e23b1abd32fce1f17267604a5102f57a/library/core/src/iter/traits/iterator.rs:1745:9
   8: itertools::Itertools::collect_vec
             at ./src/lib.rs:1985:9
   9: zip::zip_eq_panic1
             at ./tests/zip.rs:66:5

Is it possible to improve it?

@the8472
Copy link
Member

the8472 commented Sep 19, 2022

Closures can be executed pretty deep inside iterators, plumbing #[track_caller] through all possible call paths seems a bit much. It is very cheap, but not entirely free of cost since it sort of introduces an inlined wrapper function that passes the location information under the hood which makes life a tiny bit harder for the optimizers. It can lead to regressions when you put it on very hot code, e.g. #83359

@cuviper
Copy link
Member

cuviper commented Sep 20, 2022

Stated another way: the zip_eq function itself is not panicking yet, only returning the lazy ZipEq iterator. The actual panic won't be raised until the iterator is advanced that far, which may happen under any arbitrary combination of iterator methods.

The similar change proposed in rayon-rs/rayon#977 does work because IndexedParallelIterator already knows its length, so its zip_eq can immediately assert that the two iterators have the same length.

@cuviper
Copy link
Member

cuviper commented Sep 20, 2022

It would be interesting if the #[track_caller] fn zip_eq could directly access and store something like panic::Location within ZipEq, so that could be reported in the later panic message.

@cuviper
Copy link
Member

cuviper commented Sep 20, 2022

Oh, I think you can do that with Location::caller() -- have you tried that?

@the8472
Copy link
Member

the8472 commented Sep 20, 2022

The similar change proposed in rayon-rs/rayon#977 does work because IndexedParallelIterator already knows its length, so its zip_eq can immediately assert that the two iterators have the same length.

It might be possible to do this on a best-effort basis by checking whether there's any overlap between the size_hints.

@inquisitivecrystal inquisitivecrystal added A-error-handling Area: Error handling C-discussion Category: Discussion or questions that doesn't represent real issues. labels Sep 20, 2022
@jyn514 jyn514 added A-diagnostics Area: Messages for errors, warnings, and lints A-iterators Area: Iterators T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-error-handling Area: Error handling A-iterators Area: Iterators C-discussion Category: Discussion or questions that doesn't represent real issues. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants