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

Formatting instability with --file-lines #5340

Open
gigaroby opened this issue May 11, 2022 · 1 comment
Open

Formatting instability with --file-lines #5340

gigaroby opened this issue May 11, 2022 · 1 comment
Labels
only-with-option requires a non-default option value to reproduce p-low

Comments

@gigaroby
Copy link

At work we've been using --file-lines and we just ran into what we think is a bug. Take for example a snippet like (simplified and does not compile):

fn GenerateBindingsImpl() {
    catch_unwind(|| {
        // It is ok to abort here.
        let Bindings { rs_api, rs_api_impl } = generate_bindings(json, **foobar_support_path**, &rustfmt_config_path).unwrap();
        FfiBindings {
            rs_api: FfiU8SliceBox::from_boxed_slice(rs_api.into_bytes().into_boxed_slice()),
            rs_api_impl: FfiU8SliceBox::from_boxed_slice(
                rs_api_impl.into_bytes().into_boxed_slice(),
            ),
        }
    })
}

The only change we make to this snippet is to add the foobar_support_path argument and so the version control system generates a --file-lines covering that line only: rustfmt --config-path /tmp/rustfmt.toml --unstable-features --file-lines '[{"file": "stdin", "range": [4,4]}]'

This does nothing to the snippet but by playing around with the range we can find out that changing the end line has no effect (easy to try by setting it to 100) but altering the start line to 2 or less will cause it to break the line as follows:

rustfmt --config-path /tmp/rustfmt.toml --unstable-features --file-lines '[{"file": "stdin", "range": [2,4]}]'

fn GenerateBindingsImpl() {
    catch_unwind(|| {
        // It is ok to abort here.
        let Bindings { rs_api, rs_api_impl } =
            generate_bindings(json, foobar_support_path, &rustfmt_config_path).unwrap();
        FfiBindings {
            rs_api: FfiU8SliceBox::from_boxed_slice(rs_api.into_bytes().into_boxed_slice()),
            rs_api_impl: FfiU8SliceBox::from_boxed_slice(
                rs_api_impl.into_bytes().into_boxed_slice(),
            ),
        }
    })
}

The main problem with this is that editing the surrounding lines later will randomly cause these changes to appear, causing spurious diffs. This is the specific commit where we just re-formatted the entire file: google/crubit@3623489

The rustfmt.toml config we use is as follows:

edition = "2021"
version = "Two"
# The "Default" setting has a heuristic which splits lines too aggresively.
# We are willing to revisit this setting in future versions of rustfmt.
# Bugs:
#   * https://github.com/rust-lang/rustfmt/issues/3119
#   * https://github.com/rust-lang/rustfmt/issues/3120
use_small_heuristics = "Max"

newline_style = "Unix"

wrap_comments = true
@ytmimi ytmimi added the only-with-option requires a non-default option value to reproduce label May 11, 2022
@ytmimi
Copy link
Contributor

ytmimi commented May 11, 2022

Thanks for the report!

Confirming that I'm able to reproduce the behavior on rustfmt 1.4.38-nightly (17003ce6 2022-05-09). I only see changes when setting the range to [2,4], but not when setting the range to [3,4] or [4,4]. I've tested out some different inputs and I think the reason for the behavior is that the closure starts on line 2, so in order to format those lines the enclosing AST node needs to be included in the file line range. This is just a guess.

Building off your example, add a few comments before catch_unwind. If you try to set the range to [7,7], you'll see that nothing is formatted, but setting the range from [5,7] will produce formatting.

fn GenerateBindingsImpl() {
    // more comments
    // more comments
    // more comments
    catch_unwind(|| {
        // It is ok to abort here.
        let Bindings { rs_api, rs_api_impl } = generate_bindings(json, foobar_support_path, &rustfmt_config_path).unwrap();
        FfiBindings {
            rs_api: FfiU8SliceBox::from_boxed_slice(rs_api.into_bytes().into_boxed_slice()),
            rs_api_impl: FfiU8SliceBox::from_boxed_slice(
                rs_api_impl.into_bytes().into_boxed_slice(),
            ),
       }
   })
}
Snippet with line numbers
1.  fn GenerateBindingsImpl() {
2.      // more comments
3.      // more comments
4.      // more comments
5.      catch_unwind(|| {
6.          // It is ok to abort here.
7.          let Bindings { rs_api, rs_api_impl } = generate_bindings(json, foobar_support_path, &rustfmt_config_path).unwrap();
8.          FfiBindings {
9.              rs_api: FfiU8SliceBox::from_boxed_slice(rs_api.into_bytes().into_boxed_slice()),
10.             rs_api_impl: FfiU8SliceBox::from_boxed_slice(
11.                 rs_api_impl.into_bytes().into_boxed_slice(),
12.             ),
13.        }
14.    })
15. }

Linking the file-lines tracking issue so we can document this as a problem for stabilization (#3397)

@ytmimi ytmimi added the p-low label Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
only-with-option requires a non-default option value to reproduce p-low
Projects
None yet
Development

No branches or pull requests

2 participants