EscapeAscii no longer panics due to incorrect ExactSizeIterator #99913

softdevca · 2022-07-29T17:02:20Z

The ExactSizeIterator implementation for EscapeAscii was causing a panic because the length of the iterator was not explicitly available. This commit precalculates the length to use as an exact size_hint.

Supersedes #99880

rustbot · 2022-07-29T17:02:23Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

rust-highfive · 2022-07-29T17:02:23Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

Please see the contribution instructions for more information.

the8472 · 2022-07-29T17:33:39Z

library/core/src/slice/ascii.rs

@@ -174,6 +176,9 @@ impl_fn_for_zst! {
 #[must_use = "iterators are lazy and do nothing unless consumed"]
 pub struct EscapeAscii<'a> {
 inner: iter::FlatMap<super::Iter<'a, u8>, ascii::EscapeDefault, EscapeByte>,
+
+ /// Cached length of the iterator to implement `ExactSizeIterator`.
+ len: usize,


you can't cache the value because len() must be updated every time an item is consumed from the iterator.

https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.size_hint

Returns the bounds on the remaining length of the iterator.

This was also recently clarified for ExactsizeIterator, as you can see in the nightly docs

Returns the exact remaining length of the iterator.

Thank you for pointing out my oversight. I will recommit.

Well, caching may still be cheaper than recomputing it every time, but you'll at least have to update the cached value every time an item is consumed.
But the problem with that is that it makes every call to next() more expensive, so which approach is beneficial depends on how often len() is called.

I feel caching is the better choice even with the extra overhead of an decrement each next() or next_back() and an extra usize for each EscapeAscii. Caching avoids degenerate worst cases like users calling len() or size_hint() after each byte received from a slow source.

It's unfortunately to even have to go through the source iterator more than once but there isn't any other way to implement ExactSizeIterator correctly and it can't be removed because it's been released in a stable version.

and it can't be removed because it's been released in a stable version.

The stability policy does have exceptions for accidental stabilizations and critical bugfixes. So removing it is an option. The crater run in #99880 will tell us whether it's worthwhile taking that option.

clarfonthey · 2022-07-30T22:45:12Z

Hmm, it's a bit weird that ExactSizeIterator was implemented before-- that was definitely something that we should have caught during code review but didn't.

I guess that now that we've done it, we should precompute it and keep track of it. Although personally I should have probably just not implemented it for the slice version of EscapeAscii considering how it requires going through the string linearly upfront.

I'm wondering what we should do here: since len always panicked before, I wonder if we can just remove the implementation and accept the breakages since all code relying on this would have been broken anyway.

softdevca · 2022-07-31T17:23:56Z

I'm wondering what we should do here: since len always panicked before, I wonder if we can just remove the implementation and accept the breakages since all code relying on this would have been broken anyway.

A minor technicality that might not matter: Empty iterators would not panic.

…eiterator

clarfonthey · 2022-08-06T17:08:16Z

Crater answers the age-old question: if a change breaks in the forest and no one is around to notice, is it really a breaking change?

No, says crater.

softdevca · 2022-08-06T17:08:41Z

Pull request #99913 has been updated to remove the implementation of ExactSizeIterator for EscapeAscii based on the crater run.

Also, thank you to those who created and run crater, without it there would be much more uncertainty about a potentially breaking change to stable versions.

clarfonthey · 2022-08-06T17:10:49Z

Will we want to push a patch release for this just to let people know this wasn't supposed to be stable? Or is it fine to let it slide until next version?

softdevca · 2022-08-06T17:18:39Z

Will we want to push a patch release for this just to let people know this wasn't supposed to be stable? Or is it fine to let it slide until next version?

Can it get into 1.63, scheduled for August 11?

clarfonthey · 2022-08-06T18:39:40Z

Would need a beta backport, but that seems reasonable. Not sure who's following this PR who can help coordinate that though!

Mark-Simulacrum · 2022-08-06T19:12:39Z

The timeline is pretty short here, and I think we'd want an FCP or similar before dropping stable support for something like this. If it was about to land on stable we could slip it in, but otherwise I don't think it's worth it.

the8472 · 2022-08-11T20:23:20Z

I think this PR can be closed, in the last libs-api meeting the team decided to try landing #99880

softdevca · 2022-08-11T20:43:02Z

Closing this in favor of pull request #99880. Thanks for everybody's help getting issue #99878 fixed.

EscapeAscii no longer panics due to incorrect ExactSizeIterator

c6340b0

rust-highfive assigned Mark-Simulacrum Jul 29, 2022

rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jul 29, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 29, 2022

rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jul 29, 2022

softdevca mentioned this pull request Jul 29, 2022

EscapeAscii is not an ExactSizeIterator #99880

Merged

This comment has been minimized.

Sign in to view

Add end of file newline for tidy

603d0d6

This comment has been minimized.

Sign in to view

Add comma for tidy

e104c73

the8472 reviewed Jul 29, 2022

View reviewed changes

Length of iterator is remaining

d925efc

Mark-Simulacrum assigned the8472 and unassigned Mark-Simulacrum Jul 30, 2022

softdevca added 2 commits August 6, 2022 09:44

Merge branch 'rust-lang:master' into escape_ascii_implements_exactsiz…

0728ed4

…eiterator

EscapeAscii no longer implements ExactSizeIterator

37a5ee9

softdevca closed this Aug 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EscapeAscii no longer panics due to incorrect ExactSizeIterator #99913

EscapeAscii no longer panics due to incorrect ExactSizeIterator #99913

softdevca commented Jul 29, 2022

rustbot commented Jul 29, 2022

rust-highfive commented Jul 29, 2022

This comment has been minimized.

This comment has been minimized.

the8472 Jul 29, 2022 •

edited

Loading

softdevca Jul 29, 2022

the8472 Jul 29, 2022 •

edited

Loading

softdevca Jul 29, 2022

the8472 Jul 29, 2022

clarfonthey commented Jul 30, 2022 •

edited

Loading

softdevca commented Jul 31, 2022

clarfonthey commented Aug 6, 2022

softdevca commented Aug 6, 2022

clarfonthey commented Aug 6, 2022

softdevca commented Aug 6, 2022

clarfonthey commented Aug 6, 2022

Mark-Simulacrum commented Aug 6, 2022

the8472 commented Aug 11, 2022 •

edited

Loading

softdevca commented Aug 11, 2022

EscapeAscii no longer panics due to incorrect ExactSizeIterator #99913

EscapeAscii no longer panics due to incorrect ExactSizeIterator #99913

Conversation

softdevca commented Jul 29, 2022

rustbot commented Jul 29, 2022

rust-highfive commented Jul 29, 2022

This comment has been minimized.

This comment has been minimized.

the8472 Jul 29, 2022 • edited Loading

Choose a reason for hiding this comment

softdevca Jul 29, 2022

Choose a reason for hiding this comment

the8472 Jul 29, 2022 • edited Loading

Choose a reason for hiding this comment

softdevca Jul 29, 2022

Choose a reason for hiding this comment

the8472 Jul 29, 2022

Choose a reason for hiding this comment

clarfonthey commented Jul 30, 2022 • edited Loading

softdevca commented Jul 31, 2022

clarfonthey commented Aug 6, 2022

softdevca commented Aug 6, 2022

clarfonthey commented Aug 6, 2022

softdevca commented Aug 6, 2022

clarfonthey commented Aug 6, 2022

Mark-Simulacrum commented Aug 6, 2022

the8472 commented Aug 11, 2022 • edited Loading

softdevca commented Aug 11, 2022

the8472 Jul 29, 2022 •

edited

Loading

the8472 Jul 29, 2022 •

edited

Loading

clarfonthey commented Jul 30, 2022 •

edited

Loading

the8472 commented Aug 11, 2022 •

edited

Loading