Stream: t-compiler/wg-learning

Topic: Source formatting guidance in Rustc Dev Guide


Chris Simpkins (Mar 11 2020 at 23:24, on Zulip):

We have documentation in the Rustc Dev Guide that states the following:

Only run rustfmt on new content. One day, we might enforce formatting for the rust-lang/rust repo. Meanwhile, we prefer that rustfmt not be run on existing code as that will generate large diffs and will make git blame harder to sift through. However, running rustfmt on new content, e.g. a new file or a largely new part of a file is ok. Small formatting adjustments nearby code you are already changing for other purposes are also ok.

I reached out to the Rustfmt WG on Discord today and learned that rustfmt nightly has a --file-lines option for explicit source file line range formatting. It is listed as being unstable with the following help docs:

If you want to restrict reformatting to specific sets of lines, you can
use the `--file-lines` option. Its argument is a JSON array of objects
with `file` and `range` properties, where `file` is a file name, and
`range` is an array representing a range of lines like `[7,13]`. Ranges
are 1-based and inclusive of both end points. Specifying an empty array
will result in no files being formatted. For example,

rustfmt --file-lines '[
    {"file":"src/lib.rs","range":[7,13]},
    {"file":"src/lib.rs","range":[21,29]},
    {"file":"src/foo.rs","range":[10,11]},
    {"file":"src/foo.rs","range":[15,15]}]'

would format lines `7-13` and `21-29` of `src/lib.rs`, and lines `10-11`,
and `15` of `src/foo.rs`. No other files would be formatted, even if they
are included as out of line modules from `src/lib.rs`.

Thoughts about our current Rustc Dev Guide fmt documentation for contributors and whether this unstable, nightly only rustfmt approach should be mentioned?

mark-i-m (Mar 12 2020 at 01:48, on Zulip):

oh my, that is quite outdated

mark-i-m (Mar 12 2020 at 01:48, on Zulip):

The correct thing to do is run ./x.py fmt

mark-i-m (Mar 12 2020 at 01:49, on Zulip):

And the CI does enforce (via tidy) that ./x.py fmt --check passes

Chris Simpkins (Mar 12 2020 at 01:55, on Zulip):

Should the recommendation be ./x.py fmt [file you are working on] or without an argument?

simulacrum (Mar 12 2020 at 02:05, on Zulip):

x.py fmt does not take a file argument

simulacrum (Mar 12 2020 at 02:05, on Zulip):

you should run x.py fmt as-is

Chris Simpkins (Mar 12 2020 at 02:05, on Zulip):

simulacrum said:

you should run x.py fmt as-is

ty!

simulacrum (Mar 12 2020 at 02:05, on Zulip):

I would also (very) strongly recommend setting up rustfmt on save in your editor

simulacrum (Mar 12 2020 at 02:06, on Zulip):

a handy trick is to define a folder-local or project-local or w/e config to point rustfmt at the local downloaded rustfmt binary, e.g., I have let g:rustfmt_command = "/Users/mark/Edit/rust/build/x86_64-apple-darwin/stage0/bin/rustfmt" in my project-specific .vimrc

simulacrum (Mar 12 2020 at 02:07, on Zulip):

(This is not strictly necessary, but can be helpful, as rustfmt does diverge a little usually over time. I expect if you ask, @nikomatsakis may have a emacs config to share, and there's probably some level of crowd sourcing for others :)

Chris Simpkins (Mar 12 2020 at 02:08, on Zulip):

This is great. Thank you. I'll try to update the documentation with this information.

simulacrum (Mar 12 2020 at 02:09, on Zulip):

(FWIW, I had hoped that it was quite obvious what to do if CI failed, as it tells you to run x.py fmt, but it may be worth reading over the recent internals thread on this topic as well.)

simulacrum (Mar 12 2020 at 02:10, on Zulip):

er, that sentence diverged a bit there :)

I meant that there seems to possibly be some confusion on this topic, so reading that internals thread may help guide what we should try to clarify in docs

Chris Simpkins (Mar 12 2020 at 02:11, on Zulip):

This didn't come up during a fmt fail on CI. I wandered across this comment in the Guide and looked into whether it is possible to do line level fmt'ing

Chris Simpkins (Mar 12 2020 at 02:13, on Zulip):

This one? https://internals.rust-lang.org/t/forced-rustfmt-is-a-roadblock-to-contributing/11913

mark-i-m (Mar 12 2020 at 03:38, on Zulip):

yes that thread

mark-i-m (Mar 12 2020 at 03:39, on Zulip):

and i agree, there seems to be a lot of questions around this. perhaps after we update the chapter we can make a blog/internals post

Chris Simpkins (Mar 22 2020 at 04:41, on Zulip):

Collecting some of the feedback from the Internals thread...

Chris Simpkins (Mar 22 2020 at 04:42, on Zulip):

Source

To stick another voice on it, you want ./x.py fmt. This will use the pinned, in-tree version of rustfmt, which is both a) guaranteed to work (as CI is gated on it working) and b) won't disagree with ./x.py tidy, like a local version of rustfmt might.

When working in the rustc tree, the current state is that you have to use ./x.py test (which always runs tidy, even when narrowed) rather than cargo test. Even if you can/do get away with cargo-driven tests rather than bootstrap-driven tests, when in the rustc tree you should get into the habit of using ./x.py tidy.

Chris Simpkins (Mar 22 2020 at 04:45, on Zulip):

Source

Yes there are all sorts of ways to avoid forgetting to rustfmt before committing

Chris Simpkins (Mar 22 2020 at 04:47, on Zulip):

Source

I also personally have rustfmt in my editor on every save, which makes me never forget formatting.

For now, I think we should strive to make x.py fmt more discoverable (or x.py test --bless). I would appreciate PRs improving any docs or error messages from folks who were lost in this thread.

Chris Simpkins (Mar 22 2020 at 04:51, on Zulip):

Source

Re: don't use cargo +[toolchain] fmt...

either we should say directly using cargo isn't supported in the rustc tree and push people harder to use x.py, or we should say using cargo to test a single component is supported and push to make ./x.py test libfoo and cargo test -p foo execute the same tests (including tidy).

Chris Simpkins (Mar 22 2020 at 04:56, on Zulip):

Source

Re: the need for more prominent guidance on fmt approach

I wasn't aware [x.py fmt] existed!

Chris Simpkins (Mar 22 2020 at 05:03, on Zulip):

Source

Re: definition of a contributor workflow with fmt'ing

I think it would be useful to split out two threads from this discussion: rustfmt being too aggressive, perhaps, in formatting code, and improvements we can make in rust-lang/rust specifically to user's workflow. I personally would suspect that making the latter easier would be where we can most easily make the largest wins -- I'm open to suggestions and reviewing patches to the fmt integration if that would be helpful to folks.

Chris Simpkins (Mar 22 2020 at 14:41, on Zulip):

Other reference material...

Chris Simpkins (Mar 22 2020 at 14:42, on Zulip):

This is the Rust code formatting RFC GitHub repository: https://github.com/rust-dev-tools/fmt-rfcs

Chris Simpkins (Mar 22 2020 at 14:43, on Zulip):

that describes a RFC based formatting process: https://github.com/rust-dev-tools/fmt-rfcs#the-formatting-rfc-process

Chris Simpkins (Mar 22 2020 at 14:44, on Zulip):

The Guide currently indicates:

rustc is slowly moving towards the Rust standard coding style; at the moment, however, it follows a rather more chaotic style.

where Rust standard coding style = style defined in the Rust code formatting RFC repository IIUC

Chris Simpkins (Mar 22 2020 at 14:48, on Zulip):

There is a "style team" that acts as rustfmt approval decision makers:

The style team will make the ultimate decision on accepting or closing a style issue. Decisions should be by consensus.

Chris Simpkins (Mar 22 2020 at 14:56, on Zulip):

The fmt guiding principles are defined in https://github.com/rust-dev-tools/fmt-rfcs#guiding-principles as:

(decision thread is https://github.com/rust-dev-tools/fmt-rfcs/issues/4)

Chris Simpkins (Mar 22 2020 at 15:00, on Zulip):

Notable comments in those principles from the compiler fmt PoV:

Chris Simpkins (Mar 22 2020 at 15:49, on Zulip):

Question: Are there a different (subset, superset, different set) of principles required for compiler source code base fmt’ing?

Enter x.py fmt and tidy...

Chris Simpkins (Mar 22 2020 at 16:14, on Zulip):

The Guide indicates the following about tidy:

We do have some mandatory formatting conventions, which are automatically enforced by a script we affectionately call the "tidy" script. The tidy script runs automatically when you do ./x.py test and can be run in isolation with ./x.py test tidy.

Question: Re: the "mandatory formatting conventions" statement, are the conventions listed in https://rustc-dev-guide.rust-lang.org/conventions.html?highlight=tidy#formatting-and-the-tidy-script current and comprehensive guidance on what tidy enforces for compiler contributors?

Chris Simpkins (Mar 22 2020 at 16:17, on Zulip):

tidy entry point: https://github.com/rust-lang/rust/blob/master/src/tools/tidy/src/main.rs

Chris Simpkins (Mar 22 2020 at 16:51, on Zulip):

Tidy style definitions

Style Errors

Tidy opt out

Chris Simpkins (Mar 22 2020 at 17:02, on Zulip):
//! Note that some of these rules are excluded from Rust files because we enforce rustfmt. It is
//! preferable to be formatted rather than tidy-clean.

(src)

mark-i-m (Mar 22 2020 at 22:43, on Zulip):

@Chris Simpkins Wow, thanks for doing all that research

mark-i-m (Mar 22 2020 at 22:49, on Zulip):

Question: Is "style team" the rustfmt WG? (#wg-rustfmt on Discord)

Tbh, I didn't know this was a distinct team. Perhaps @simulacrum knows?

Question: Are there different (subset, superset, different set) principles required, or different priority levels for those defined, for compiler source code base fmt’ing?

AFAICT, it varies from one compiler hacker to another what they value the most. Personally for me, readability and consistency across the (very large) codebase are the most important, but others may prioritize other things, I guess.

Question: Re: the "mandatory formatting conventions" statement, are the conventions listed in https://rustc-dev-guide.rust-lang.org/conventions.html?highlight=tidy#formatting-and-the-tidy-script current and comprehensive guidance on what tidy enforces for compiler contributors?

I don't think we document them anywhere. But generally, we use nightly rustfmt + this rustfmt.toml. I don't personally know how important it is to document the specific things. I just run ./x.py fmt before committing and don't really think about it again. Do you feel that it is an obstacle to new developers?

simulacrum (Mar 22 2020 at 22:50, on Zulip):

It is no longer an active team, to my knowledge - used to be a WG (under, IIRC, @nrc).

simulacrum (Mar 22 2020 at 22:51, on Zulip):

We basically only support x.py fmt (and you can do what it does yourself, e.g. on save).

Chris Simpkins (Mar 22 2020 at 22:53, on Zulip):

I don't think we document them anywhere. But generally, we use nightly rustfmt + this rustfmt.toml. I don't personally know how important it is to document the specific things. I just run ./x.py fmt before committing and don't really think about it again. Do you feel that it is an obstacle to new developers?

Not necessarily an obstacle, but the Internals thread points to a good deal of questioning about why it is necessary at all. I wonder if helps to address why the formatting is used and how to easily accomplish it. There were active (IIUC reasonably long-term contributrors to rust) developers in the Internals thread who knew nothing of the ./x.py formatting approaches. The arguments veered off into why make developers do it at all, just do it after they push land. Maybe we try to address these things?

mark-i-m (Mar 22 2020 at 22:54, on Zulip):

Hmm... fair point

mark-i-m (Mar 22 2020 at 22:55, on Zulip):

Personally, I would rather get a statement on this from t-compiler or something

mark-i-m (Mar 22 2020 at 22:56, on Zulip):

That is, since it does appear to have a fair amount of controversy I would rather get guidance from project leaders on this topic

mark-i-m (Mar 22 2020 at 22:57, on Zulip):

I think you are right that it would be good to address in the guide at some point though

Chris Simpkins (Mar 22 2020 at 22:59, on Zulip):

mark-i-m said:

That is, since it does appear to have a fair amount of controversy I would rather get guidance from project leaders on this topic

Definitely makes sense. I would be happy to pull together documentation for the Guide and/or a blog post on the topic once we understand if/how documentation might help to address the concerns expressed there. There is more to it than docs, but we may be able to help.

Chris Simpkins (Mar 22 2020 at 23:19, on Zulip):

@simulacrum

We basically only support x.py fmt (and you can do what it does yourself, e.g. on save).

And the recommended approach as I understand it, is to run ./x.py test -i --stage 1 [file | dir] before committing. This runs tidy so that you get the tidy lints and know that you need to fmt if you don't use a fmt on save approach.

simulacrum (Mar 22 2020 at 23:23, on Zulip):

No, that's not generally how to get formatting. I would recommend just x.py fmt before committing if you don't have it set up on save.

simulacrum (Mar 22 2020 at 23:24, on Zulip):

(That is the right approach for testing, though)

Chris Simpkins (Mar 22 2020 at 23:25, on Zulip):

simulacrum said:

No, that's not generally how to get formatting. I would recommend just x.py fmt before committing if you don't have it set up on save.

Sorry, I wasn't clear. That lints the files IIUC and tidy informs you as part of the local test run that you need to fmt correct? The Internals thread suggests that there are those who are surprised to find that (1) they didn't fmt; (2) their CI build fails minutes later and they don't know pass/fail status of build for their changes because the lack of fmt blocked completion of the build. Local .x.py test run does run tidy?

mark-i-m (Mar 23 2020 at 03:02, on Zulip):

@Chris Simpkins there is also ./x.py fmt --check which is run by ./x.py test ...

simulacrum (Mar 23 2020 at 13:44, on Zulip):

x.py test does run tidy, but generally speaking most people should not be running x.py test locally (it's long). I would personally recommend that most people just run UI tests after a compiler change, if that

Last update: Apr 03 2020 at 16:50UTC