Replace deprecated Range::step_by() with Iterator::step_by()

[jdm]

https://doc.servo.org/core/ops/struct.Range.html#method.step_by
https://doc.servo.org/core/iter/trait.Iterator.html#method.step_by

warning: use of deprecated item: replaced by `Iterator::step_by`
   --> /Users/jdm/src/master-servo/components/layout/inline.rs:969:66
    |
969 |                 (range.end().get() - 1..range.begin().get() - 1).step_by(-1)
    |                                                                  ^^^^^^^
    |
    = note: #[warn(deprecated)] on by default

warning: use of deprecated item: replaced by `Iterator::step_by`
   --> /Users/jdm/src/master-servo/components/layout/inline.rs:971:58
    |
971 |                 (range.begin().get()..range.end().get()).step_by(1)
    |                                                          ^^^^^^^
    |
    = note: #[warn(deprecated)] on by default

This will require replacing the existing #[feature(step_by)] with #[feature(iterator_step_by)] at the very least.

[highfive]

Hi! If you have any questions regarding this issue, feel free to make a comment here, or ask it in the #servo channel in IRC.

If you intend to work on this issue, then add @highfive: assign me to your comment, and I'll assign this to you. 😄

[jklepatch]

@highfive: assign me

[highfive]

Hey @jklepatch! Thanks for your interest in working on this issue. It's now assigned to you!

[jklepatch]

I did a first try but didnt succeed. I posted a related question on the Rust issue tracker:
rust-lang/rust#27741

[jdm]

You may need to use something like .iter().step_by(..) now.

[jklepatch]

• In lib.rs, I have replaced #![feature(step_by)] with #![feature(iterator_step_by)]
• in layout/inline.rs, I have added use std::iter::Iterator; at the top

Successful Attempt:

     let fragment_indices = if reverse {
                (range.end().get() - 1..range.begin().get() - 1)
      } else {
                (range.begin().get()..range.end().get())
      };

     for fragment_index in Iterator::step_by(fragment_indices, 1) {
        // ...
     }

I followed this example: https://github.com/servo/servo/blob/master/components/net_traits/image/base.rs#LC53

Although it compiles, It's still not correct because iterating over a range defined with the higher bound first and lower bound after does not work as expected (first branch of the if statement). It actually does not iterate at all. A solution is to execute rev() on the range, then it will iterate backwards as desired. But then I don't see the point of using step_by() anymore. We could just call rev() on the range directly.

I have created this rust playground to illustrate my point:
https://play.rust-lang.org/?gist=f6d1b37d27dbb1881ebabd6de8630f7d&version=nightly&backtrace=0

FYI, below are all my unsuccesful attempts

Unsuccessful Attempt #1: Use directly step_by():

error: use of unstable library feature 'step_by': recent addition (see issue #27741)
   --> /home/jklepatch/os/servo/components/layout/inline.rs:971:66
       |
971 |                 (range.end().get() - 1..range.begin().get() - 1).step_by(-1)
       |                                                                                          ^^^^^^^
       |
       = help: add #![feature(step_by)] to the crate attributes to enable

Unsuccessful Attempt #2: Use into_iter():

error: use of unstable library feature 'step_by': recent addition (see issue #27741)
   --> /home/jklepatch/os/servo/components/layout/inline.rs:971:78
       |
971 |                 (range.end().get() - 1..range.begin().get() - 1).into_iter().step_by(-1)
       |                                                                                                          ^^^^^^^
       |
       = help: add #![feature(step_by)] to the crate attributes to enable

Unsuccessful Attempt #3: Use iter():

error[E0599]: no method named `iter` found for type `core::ops::Range<isize>` in the current scope
   --> /home/jklepatch/os/servo/components/layout/inline.rs:971:66
       |
971 |                 (range.end().get() - 1..range.begin().get() - 1).iter().step_by(-1)
       |                                                                                         ^^^^

Unsuccessful Attempt #4: Use into_iter() + iter():

error[E0599]: no method named `iter` found for type `core::ops::Range<isize>` in the current scope
   --> /home/jklepatch/os/servo/components/layout/inline.rs:971:78
       |
971 |                 (range.end().get() - 1..range.begin().get() - 1).into_iter().iter().step_by(-1)
       |                                                                                                         ^^^^

[jdm]

I agree, I don't think that step_by adds anything useful here. Calling rev() will result in more readable code.

[jklepatch]

I tried the below solution, but it didnt work because the 2 branches of the if statement return a different type:

    let fragment_indices = if reverse {
                 (range.begin().get()..range.end().get()).rev()
      } else {
                (range.begin().get()..range.end().get())
      };

I tried to call into_iter() in the second branch, but it still returns a Range.

I was thinking of 2 other solutions:

1. Creating an Enum ReversableRange that can have the iterator returned by rev() or a Range. Not sure it would work in the for statement.
2. Move everything inside the for statement into its own function, and call this function in a match statement. The above-mentioned Enum ReversableRange would probably have to be created for this solution also.

match reverse {
      1 =>  func((range.begin().get()..range.end().get()).rev()),
      _ =>  func((range.begin().get()..range.end().get()));      
      }

Any opinions?

[jdm]

Good points! I think the enum would be the best way forward; if you implement the Iterator trait for it by invoking the next() method on each contained iterator type, I think it should work fine.

[scottmcm]

Another option would be a "conditional reverse" adapter, as described here: rust-lang/rust#27741 (comment)

[SimonSapin]

Fixed in #17605.