Cosmetic changes to compiler comments and docs

[oli-obk]

@rust-lang/compiler discussed these kind of PRs (notably #58524 and #58036) in the weekly compiler meeting (zulip link).

Summary:

We do not have a style guide for such comments and having one is undesirable. In that light it must be assumed that all changes are subjective and do not offer a clear (as in uncontested) benefit. Additionally both developer and reviewer time can be spent on much more productive endavours. Other communities (e.g. python) have adopted similar policies.

In order to come to a decision on this, we're going to run an FCP.

[oli-obk]

@rfcbot merge

Do we want to close existing and future cosmetic PRs without reviewing them and actively discourage them from being created in the first place?

[rfcbot]

Team member @oli-obk has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Zoxc
• @cramertj
• @eddyb
• @estebank
• @michaelwoerister
• @nagisa
• @nikomatsakis
• @oli-obk
• @petrochenkov
• @pnkfelix
• @varkor

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[alexreg]

I appreciate you discussing this, but I don't agree that all changes are subjective. I mean, there's a spectrum, and many fall pretty close to the "objective" end. (Backticks around inline code and variable names, for example.) Indeed, I proposed the start of a style guide on the Internals forum, and it seemed there was nearly-universal agreement on many (albeit certainly not all) points. There seemed to also be a moderate amount of support for the effort. Cleaning up a codebase and making comments more readable, intelligible, and easy to scan is in my view a real benefit, and I believe that the great majority of software engineers would agree. What those style points are is of course more contentious, but I've not seen evidence most of the changes in my PRs have been contentious.

I think there are less drastic measures than the one you propose. What about a simple flagging system, where someone posts a comment on a cosmetic PR if they strongly disagree with a style point, and if it gets (say) two upvotes from compiler team members, all such changes are disallowed? This promotes clean-up, and encourages higher-quality comments, while not creating too much discord around the matter.

Finally, I'm not expecting massive gratefulness for my work (which most people would consider tedious and unglamorous), but I do want to make it known that I expected a more "friendly and welcoming" response, as the Rust Community code would suggest, and have been left a little disconcerted by this experience.

[oli-obk]

This issue is not about the style we want to have in comments. This issue is about procedure. Doing this manually will just cause the code base to regress again. Also it costs a lot of reviewer time, which is tight anyway.

Manual work and reviewing cost could be solved via automation (although developing said automation costs impl work and reviewing time). In the linked compiler team discussion there was talk about automating this via a rustfmt-like tool for comments/docs. (There was some resistance to even forcing rustfmt on every PR, but that's a tangent not relevant for this issue). There was some desire for such a tool, but a precondition would be a style guide (and the process to create such a guide via community consensus). It was unanimous that we do not have the capacity to create or review the creation of such a tool.

I think there are less drastic measures than the one you propose. What about a simple flagging system, where someone posts a comment on a cosmetic PR if they strongly disagree with a style point, and if it gets (say) two upvotes from compiler team members, all such changes are disallowed? This promotes clean-up, and encourages higher-quality comments, while not creating too much discord around the matter.

Isn't this essentially what we tried out over the last few weeks and it didn't work? Multiple compiler team members disagreeing with a point and then that disagreement causing discord and a lot of discussion?

[alexreg]

Isn't this essentially what we tried out over the last few weeks and it didn't work? Multiple compiler team members disagreeing with a point and then that disagreement causing discord and a lot of discussion?

Kind of, but several of the comments were more like, "I wouldn't write it this way myself, but I'm not really that bothered about it. I may just continue doing it my way."

In any case, I'm happy to make this big PR (or rather, the smaller PRs forked off from it) the last codebase-wide cleanup, with the proviso that any future clean-ups will be highly uncontentious (grammatical fixes, adding backticks), and limited to code that a PR directly concerns.

[nikomatsakis]

@alexreg can you link to the style guide that you created on internals?

[alexreg]

@nikomatsakis More like a draft style guide -- very informal. But it could probably be turned into a proper one without too much effort. :-)

https://internals.rust-lang.org/t/style-guide-for-comments/8995

[nikomatsakis]

@rfcbot fcp reviewed

My feeling on the matter:

First, I feel bad that @alexreg feels bad. =) I think this is a tricky matter. @alexreg, I think I would say that -- for my part, and I'm sure I speak for many others -- I appreciate the work you've been putting into the improving the codebase, and I know your intentions are good. That's not really in question.

That said, I also think that @oli-obk and @petrochenkov etc have a point that, in practice, reviewing "style change" PRs seems to generate more trouble than it's worth. Even looking past the bad feelings, large PRs with lots of small diffs spread across the codebase are rather difficult to land, and generate a lot of merge conflicts. Given what a pain the bors queue can be, that alone might be mildly disqualifying.

I do agree with @alexreg that some recommendations are very close to "objective". The example of enclosing local variable names in backticks is an interesting one, as it is both something that I try to do (and indeed find mildly helpful) and something that would be very difficult or impossible to automate via a tidy-like tool. I think it's a good example for showing why the idea of writing an automated tool isn't so great.

Putting it all together, I think I still lean towards the team consensus that it would generally be better to avoid "sweeping changes" that touch a lot of files just to apply some style guideline. I think updating comments "in place" when you're doing other things that touch the surrounding code seems good.

I think in the future I might be open to revisiting this question, but I think that, when it comes to consistency in the codebase, I would prefer to work towards (a) rustfmt and (b) getting good rustdoc coverage.

---

As a side note, I could see trying to include some kind of "style suggestions" section in the rustc-guide with a few lightweight suggestions (though I'd probably phrase them as "suggestions" and not "rules"). Looking at the style guide that @alexreg posted to internals, I spotted these two which I think are particularly useful (especially the first one):

• Prefer /// doc-comments when applicable, since they can appear in rustdoc
• Prefer to use x for local variables and other references to "code" things

In contrast, though, I'd probably not be inclined to make rules about eg vs e.g., or whether to require full sentences, and so forth. (I was intrigued, I admit, by the number of linters and other suggestions in there.)

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔