Fix skip range miscalculation

[matthewhughes934]

As well as a bit of code tidying

• Idempotent test: avoid redundant file reads

  We read the 'significant comments' in check_files, so just pass it
  down rather than re-reading the test file.

• Fix skip range miscalculated on nested blocks

  Fix a bug in FmtVisitor::push_skipped_with_span that assumed
  line_number was always absolute (within a given file) and instead set
  the end of the range to be the end of the span with the skip attribute.

  Found while investigating issue rustfmt::skip sometimes ignored when using error_on_line_overflow and error_on_unformatted #6954 (but this commit doesn't address
  that issue).

[ytmimi]

I've wondered for a while if using the spans from the source makes sense when it comes to determining the skipped_range. Wouldn't we have to figure out the range relative to what's already been formatted in the file?

For example:

fn main() {
   let some.long().method().call().chain().that().will().wrap();

   #[rustfmt::skip] // line 4 before formatting;
   let x = 100;
}

fn main() {
   let some
        .long()
        .method()
        .call()
        .chain()
        .that()
        .will()
        .wrap();

   #[rustfmt::skip] // line 11 after formatting;
   let x = 100;
}

View changes since the review

[matthewhughes934]

Wouldn't we have to figure out the range relative to what's already been formatted in the file?

yes 🙃, I can break my test with your change: on the first run it adds a 'skip' range that ends before the long string after formatting the method chain (no errors on subsequent runs, once the chain is formatted)

diff --git i/tests/target/skip_with_overflow.rs w/tests/target/skip_with_overflow.rs
index a3a03b80..89cd7a3e 100644
--- i/tests/target/skip_with_overflow.rs
+++ w/tests/target/skip_with_overflow.rs
@@ -12,8 +12,10 @@ fn foo() {
         // the bug: we'd mark the region (lo=16, hi=10) as skipped
         // the lo value is correct, but the hi value is the offset of the line after the end of the
         // 'if' block relative to the start of the '|| {' block
-        #[rustfmt::skip]
         if true {
+            some.very().very().very().very().very().very().long().method().call().chain().that().will().wrap();
+
+            #[rustfmt::skip]
             println!(
                 "this is a very long string, it goes over max_width. This is just padding to push it over"
             );

[matthewhughes934]

which leaves me with the question: do we fix the bug in the current approach (writing skips w.r.t unformatted source). Or, do we try and move to some more robust approach? I have no idea how the latter would look, some ideas that come to mind (I have not thought about them much, neither of them feel very good):

• checking for overflows on each line we push (where we have the context of the current span we're working on)
• Append some marker comment like // _RUSTFMT_SKIP_HERE to all lines in a skip block, so when looking at the formatted source we know what's skip (then strip out those comments before writing things out)

[ytmimi]

Might make more sense to turn this into a snapshot test and show that stderr is empty. Alternatively you can write a unit test like this one.

View changes since the review

[ytmimi]

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.