1.x fix for missing comments extra macro body indentation

[davidBar-On]

1.x fix for issue #4603 - infinite macro body indentation because of "missing comment". The fix is per the approach discussed in #4629.

As discussed, the fix includes handling both LostComment and TrailingWhitespace errors, although I am not sure if TrailingWhitespace is needed.

UPDATE: the tests fail because an issue that i cannot understand. When running cargo make test in my local machine it fails on the same errors. However, when running manually the created ./target/debug/rustfmt the output is o.k. I hoped that this is a problem in my local machine, but unfortunately it seems it isn't. I will be glad to get help with this issue.

[calebcartwright]

The cargo make piece was a bit of a red herring (you'll see the same failures running cargo +nightly test with the requisite env vars set). The real problem is that we need error_on_unformatted to be set to true for the session that's formatting the macro def body, otherwise the errors we are looking for get tossed.

When running the rustfmt bin directly, it will pick up on the rustfmt.toml config file in the root of the repository which does set this, whereas our test suites will, by design, override/set explicit config values which is why the tests are failing.

Think this can be resolved by simply setting that on the config:

config.set().emit_mode(config::EmitMode::Stdout);
config.set().verbose(Verbosity::Quiet);
config.set().hide_parse_errors(true);
if is_macro_def {
    config.set().error_on_unformatted(true);
}

[davidBar-On]

Thanks! Setting the config solved the issue.

Note that the changes include a fix of a minor rustfmt 1.x issue: in the following code, moved the closing ) after has_diff() to be after has_macro_format_failure:

rustfmt/src/lib.rs

Lines 485 to 486 in 7de6968

	|| self.has_diff())
	|| self.errors.has_macro_format_failure

[calebcartwright]

Thanks! Few minor things but otherwise LGTM

[calebcartwright]

I think we should bikeshed on the name here, as the way the name currently reads is confusing. This flag is indicator that there was something that rustfmt failed to be able to format successfully (almost always caused by the presence of a comment in a place rustfmt cannot support comments).

It's really more indicative that there were parts of the input code that rustfmt failed to format and reverted to the original formatting, so there are unformatted portions of the code in the output.

[davidBar-On]

I forgot to mention the name in the discussion, as I couldn't find a good name for both lost-comments and trailing-white-spaces errors. What about has_lost_comments_error? has_unformatted_original_code? Another option is to split it to has_lost_comments_error and has_trailing_whitespaces_error.

[calebcartwright]

i'd prefer something that references "failed" or "unformatted"

[davidBar-On]

Renamed has_missing_errors to has_unformatted_original_code. Hope this is o.k.

[calebcartwright]

I don't care too much about the internal name, but do have some reservations about the public facing function given that the rustfmt lib is available for outside consumers (and we can't change it once we put it out there).

I think it's important to keep errors in the name, so would prefer either:

(a) has_unformatted_code_errors
(b) has_unformatted_errors
(c) skip an aggregate helper, and just track/expose whitespace vs. lost comments separately (has_lost_comment_errors and has_trailing_whitespace_errors).

Please be sure to update the function name(s) accordingly too

[davidBar-On]

Changed the name to has_unformatted_code_errors.

[calebcartwright]

Awesome, thank you!