Rustfmt: Audit exit codes

[nrc]

Currently:

    0 = No errors
    1 = Encountered operational errors e.g. an IO error
    2 = Failed to reformat code because of parsing errors
    3 = Code is valid, but it is impossible to format it properly
    4 = Formatted code differs from existing code (write-mode diff only)

Can we do better? What is optimal for CI? (There are some issues filed about this sort of thing, would be good to look them up).

[michaelsproul]

I did a bit of digging in the related issues and found https://github.com/rust-lang-nursery/rustfmt/issues/1832. Peter (@scode)'s suggestion seems to be to exit with code 0 where we currently exit with 3 (i.e. for impossible-to-format code), and also to combine 1,2,4 into just 1... This seems to kind of make sense, but I'll have to think on it some more...

[nrc]

The error code summary in OP is still correct for rustfmt. For cargo fmt, we just return 0 or 1 (1 for any non-0 rustfmt exit code).

Relevant issues are #1080 and #1832 - which are both dealing with the long lines problem.

There's also #1232 (and I believe there have been other issues) which are about having a 'check' mode for rustfmt where we only check whether a program would be reformatted by rustfmt, without actually reformatting anything. #1028 and #1976 are related since they both touch on write modes ('check' would be a new one).

It seems to me that for exit codes, there are two use cases - regular use of rustfmt where the exit code is informational and CI usage where we want to know whether the program 'passes' rustfmt.

Here is a proposal:

• We add a check write mode which checks formatting without writing anything out. It emits 0 if everything is OK, and 1 if running rustfmt would change anything or there was a bug running rustfmt. Output to stdout (which I think should be the same output as diff mode) would distnguish the two cases.
• In every other mode, we output 0 if rustfmt executed OK, no matter whether formatting was performed or not, or 1 if there was an internal error. Running with --verbose will indicate to stdout exactly whether there was a problem formatting, parsing, etc. (i.e., current exit codes 2, 3, and 4 turn into 0).
  
  
  
  • diff mode will have the same exit statuses as other modes
  
  
• cargo fmt will have the same exit code as rustfmt
• in check mode, a line longer than max width will cause a 1 exit code unless it is explicitly ignored (i.e., a comment or string lit would be considered a failure). In other modes, it is not considered an internal error (i.e., would emit 0). (To close #1080, we also need to make sure the error message is clear now).
• semi-related - when outputting diffs, we will not include the end of line character unless it is enabled with an option - should make the diffs more easily usable

[nrc]

Follow-up: we should remove the error_on_line_overflow option - I feel like it is mostly redundant under the above proposal. The only question is if people running on CI might like a way to ignore cases where comments or string lits overrun the max width? Personally I wouldn't want that, but I can understand if people did.

Also, what about cases where Rustfmt tried and failed to format a line under the max width (i.e., not explicitly ignored and not ignored because it is a comment or macro, etc.)? I think in check mode, that should give exit 1, (i.e., exit 0 implies every line <= max width or is explicitly ignored). In other modes, is this an internal error or not? I think it must be a bug, and therefore probably ought to be considered an error, but maybe not?

Do we also want to distinguish between code which is ignored and allowed to overrun and code which is ignored but should be manually formatted to not overrun? (I expect trailing whitespace to be treated similarly).

[nrc]

cc @topecongiro on the above - sound good?

[topecongiro]

Sounds good to me, thank you for summarizing!

Follow-up: we should remove the error_on_line_overflow option - I feel like it is mostly redundant under the above proposal. The only question is if people running on CI might like a way to ignore cases where comments or string lits overrun the max width? Personally I wouldn't want that, but I can understand if people did.

If I understand correctly, those who want to ignore errors from comments or string lits could use diff mode instead. This seems a bit confusing, though, so I see some benefits on keeping error_on_line_overflow option.

Also, what about cases where Rustfmt tried and failed to format a line under the max width (i.e., not explicitly ignored and not ignored because it is a comment or macro, etc.)? I think in check mode, that should give exit 1, (i.e., exit 0 implies every line <= max width or is explicitly ignored). In other modes, is this an internal error or not? I think it must be a bug, and therefore probably ought to be considered an error, but maybe not?

I agree that in both cases they should be considered as an error. On a side note, I think a very long ident should be considered as a non-error as well (cc #1448).

Do we also want to distinguish between code which is ignored and allowed to overrun and code which is ignored but should be manually formatted to not overrun? (I expect trailing whitespace to be treated similarly).

Personally I see no benefits on distinguishing those two.

[nrc]

in check mode, a line longer than max width will cause a 1 exit code unless it is explicitly ignored (i.e., a comment or string lit would be considered a failure). In other modes, it is not considered an internal error (i.e., would emit 0). (To close #1080, we also need to make sure the error message is clear now).

This is the only remaining work here

[nrc]

Also #2707

[nrc]

This is the only remaining work here

I'm not sure exactly what to do about "by default" I think the user probably should opt-in with the config option. Otherwise, this is now implemented.