JSON output for removing blank lines is wrong #4259

Timmmm · 2020-06-17T12:14:55Z

Rustfmt version: rustfmt 2.0.0-rc.2-nightly (e9b039ea 2020-06-03)

If Rustfmt decides to remove a blank line, the JSON output says that actually nothing needs to change. Trivial repro:

 ~ cat foo.rs
fn foo() {

    let i = 0;
}
 ~ rustfmt --emit json foo.rs | jq
[
  {
    "name": "/Users/me/foo.rs",
    "mismatches": [
      {
        "original_begin_line": 2,
        "original_end_line": 2,
        "expected_begin_line": 2,
        "expected_end_line": 2,
        "original": "",
        "expected": ""
      }
    ]
  }
]

It seems fine at removing newlines in other cases:

 ~ cat foo.rs
fn foo() {
    let i =
    0;
}
 ~ rustfmt --emit json foo.rs | jq
[
  {
    "name": "/Users/timh/foo.rs",
    "mismatches": [
      {
        "original_begin_line": 2,
        "original_end_line": 3,
        "expected_begin_line": 2,
        "expected_end_line": 2,
        "original": "    let i =\n    0;",
        "expected": "    let i = 0;"
      }
    ]
  }
]

So I think the output should be something like this:

[
  {
    "name": "/Users/me/foo.rs",
    "mismatches": [
      {
        "original_begin_line": 2,
        "original_end_line": 3,
        "expected_begin_line": 2,
        "expected_end_line": 2,
        "original": "\n",
        "expected": ""
      }
    ]
  }
]

The text was updated successfully, but these errors were encountered:

Timmmm · 2020-06-17T13:11:05Z

Here's another case that causes issues. It seems like there should be newlines that get stripped.

 ~ cat foo.rs
mod a;
mod c;
mod b;
mod d;
 ~ rustfmt --emit json foo.rs | jq
[
  {
    "name": "/Users/timh/foo.rs",
    "mismatches": [
      {
        "original_begin_line": 2,
        "original_end_line": 2,
        "expected_begin_line": 2,
        "expected_end_line": 2,
        "original": "mod c;",
        "expected": ""
      },
      {
        "original_begin_line": 4,
        "original_end_line": 4,
        "expected_begin_line": 3,
        "expected_end_line": 3,
        "original": "",
        "expected": "mod c;"
      }
    ]
  }
]

I think it's trying to reformat it alphabetically, like

mod a;
mod b;
mod c;
mod d;

But what you actually get is

mod a;
mod b;
mod c;mod d;

I think both instances of "mod c;" should actually be "mod c;\n".

calebcartwright · 2020-06-19T03:05:23Z

Thanks for the report @Timmmm, and for giving the rustfmt 2.0-rc a test drive!

I think it's trying to reformat it alphabetically, like

Yup, the default behavior is to reorder mods alphabetically, and this can controlled by the reorder_modules configuration option.

If Rustfmt decides to remove a blank line, the JSON output says that actually nothing needs to change.

Not exactly. The fact that there's mismatches in the output is indicative that something does need to be changed, it's just ambiguous what those changes are. I feel the question is really about how newline changes should be represented in the output, whether implicit/assumed given the line numbers, or explicitly.

It's clearly not explicit right now, and the scenarios you've shared here raise some good points about why IMO something should change. In addition to the removal of newline chars, it'd also be tricky to know from the json output when a newline is inserted, especially since rustfmt could be emitting different newline styles depending on the config.

The json emit mode (which is also used with --message-format json from cargo fmt) is still unstable, so we'll want to get this sorted before stabilizing the json emit mode. Within the rustfmt maintainer team we've got a lot of higher priority items on our plates right now so it'd probably be a while before we can get around to this.

If anyone's interested and willing to try updating this that would be fantastic and we'd be happy to offer assistance!

The mismatch specification logic for the json emit mode can be found here:

rustfmt/src/emitter/json.rs

Lines 57 to 102 in cf1f0cf

 fn add_misformatted_file( 

 &mut self, 

 filename: &FileName, 

 diff: Vec<Mismatch>, 

 ) -> Result<(), EmitterError> { 

 let mut mismatches = vec![]; 

 for mismatch in diff { 

 let original_begin_line = mismatch.line_number_orig; 

 let expected_begin_line = mismatch.line_number; 

 let mut original_end_line = original_begin_line; 

 let mut expected_end_line = expected_begin_line; 

 let mut original_line_counter = 0; 

 let mut expected_line_counter = 0; 

 let mut original_lines = vec![]; 

 let mut expected_lines = vec![]; 

 for line in mismatch.lines { 

 match line { 

 DiffLine::Expected(msg) => { 

 expected_end_line = expected_begin_line expected_line_counter; 

 expected_line_counter = 1; 

 expected_lines.push(msg) 

 } 

 DiffLine::Resulting(msg) => { 

 original_end_line = original_begin_line original_line_counter; 

 original_line_counter = 1; 

 original_lines.push(msg) 

 } 

 DiffLine::Context(_) => continue, 

 } 

 } 

 mismatches.push(MismatchedBlock { 

 original_begin_line, 

 original_end_line, 

 expected_begin_line, 

 expected_end_line, 

 original: original_lines.join("\n"), 

 expected: expected_lines.join("\n"), 

 }); 

 } 

 self.mismatched_files.push(MismatchedFile { 

 name: format!("{}", filename), 

 mismatches, 

 }); 

 Ok(())

It may also be helpful to cross reference the function that generates the visual output found with the --check mode.

This changes the JSON output to be more consistent about where newlines are included. Previously it only included them between lines in a multiline diff. That meant single line changes were treated a bit weirdly. This changes it to append a newline to every line. When feeding the results into `arc lint` this behaves correctly. I have only done limited testing though, in particular there's a possibility it might not work with files with `\r\n` endings (though that would have been the case before too). Fixes rust-lang#4259

Timmmm · 2020-06-19T07:34:28Z

Thanks for the hint! Turns out the fix was quite easy. :-)

* Fix newlines in JSON output This changes the JSON output to be more consistent about where newlines are included. Previously it only included them between lines in a multiline diff. That meant single line changes were treated a bit weirdly. This changes it to append a newline to every line. When feeding the results into `arc lint` this behaves correctly. I have only done limited testing though, in particular there's a possibility it might not work with files with `\r\n` endings (though that would have been the case before too). Fixes #4259 * Update tests

* Fix newlines in JSON output This changes the JSON output to be more consistent about where newlines are included. Previously it only included them between lines in a multiline diff. That meant single line changes were treated a bit weirdly. This changes it to append a newline to every line. When feeding the results into `arc lint` this behaves correctly. I have only done limited testing though, in particular there's a possibility it might not work with files with `\r\n` endings (though that would have been the case before too). Fixes rust-lang#4259 * Update tests # Conflicts: # tests/writemode/target/output.json

* Fix newlines in JSON output This changes the JSON output to be more consistent about where newlines are included. Previously it only included them between lines in a multiline diff. That meant single line changes were treated a bit weirdly. This changes it to append a newline to every line. When feeding the results into `arc lint` this behaves correctly. I have only done limited testing though, in particular there's a possibility it might not work with files with `\r\n` endings (though that would have been the case before too). Fixes #4259 * Update tests # Conflicts: # tests/writemode/target/output.json

calebcartwright added the emitter label Jun 19, 2020

calebcartwright added good first issue Issues up for grabs, also good candidates for new rustfmt contributors help wanted labels Jun 19, 2020

Timmmm mentioned this issue Jun 19, 2020

Fix newlines in JSON output #4262

Merged

calebcartwright closed this as completed in #4262 Jun 23, 2020

calebcartwright mentioned this issue Jun 23, 2020

Attempt to show correct newline style in json emitter #4273

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JSON output for removing blank lines is wrong #4259

JSON output for removing blank lines is wrong #4259

Timmmm commented Jun 17, 2020

Timmmm commented Jun 17, 2020

calebcartwright commented Jun 19, 2020

Timmmm commented Jun 19, 2020

JSON output for removing blank lines is wrong #4259

JSON output for removing blank lines is wrong #4259

Comments

Timmmm commented Jun 17, 2020

Timmmm commented Jun 17, 2020

calebcartwright commented Jun 19, 2020

Timmmm commented Jun 19, 2020