search

rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
99.07k stars 12.79k forks source link

Cosmetic changes to compiler comments and docs #58619

Closed oli-obk closed 5 years ago

oli-obk commented 5 years ago

@rust-lang/compiler discussed these kind of PRs (notably https://github.com/rust-lang/rust/pull/58524 and https://github.com/rust-lang/rust/pull/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 commented 5 years ago

@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 commented 5 years ago

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

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

alexreg commented 5 years ago

@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 commented 5 years ago

@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):

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 commented 5 years ago

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

nikomatsakis commented 5 years ago

So, to surface this, @alexreg and I chatted a bit in privmsg. @alexreg basically agreed that it probably makes sense to try and avoid big "style PRs" for the time being, but expressed some regret that they had already put in a fair amount of work into the existing PR and -- particularly since a number of the changes are not really controversial -- were interested in landing some of those changes, which they thought were generally uncontroversial. I asked if they could prepare a list of the 'kinds' of changes they're talking about (presuming that's not a lot of work for them to do).

nikomatsakis commented 5 years ago

That said, @alexreg, I'm not sure if you meant to include those two lists in your comment or perhaps as follow-up comments? :)

alexreg commented 5 years ago

Really appreciate your input here, @nikomatsakis. I think all you've said is pretty fair. So, after our brief discussion on chat, here are my two lists of cosmetic changes that I think we should accept insofar as my existing work is concerned, while only the first should really be "recommended practice" in the future. Open to revision here, but hopefully this won't need to many more back-and-forths.

Largely uncontroversial (close to objective improvement)

Somewhat controversial, but already very inconsistent in the codebase, so not worth the effort of taking out of my existing PR – won't enforce in future though.

Other cosmetic changes

--> Remove them!

Oh, and I haven't mentioned it, but it can't possibly annoy anyone – typo/spelling/grammatical fixes should be fine too.

If I've left in any changes (in either list) that still annoy people, I apologise in advance; please just let me know and I'll remove them.

CC @oli-obk @RalfJung @petrochenkov

petrochenkov commented 5 years ago

I'm still against, unless the rules are documented, automated and submitted in one-rule-per-PR fashion (the process, basically).

I think even re-opening this discussion was a mistake, since now it's draining time and energy from participants again. Also, a private chat shouldn't generally turn the course 180 degrees from the team consensus, that undermines the FCP process.

petrochenkov commented 5 years ago

@alexreg

Somewhat controversial ... so not worth the effort of taking out of my existing PR

That's plain impolite. No consensus - no changes. "It's hard to remove the controversial changes because they are done manually, so I won't remove them" is the primary reason why I want to nuke these PRs and migrate (or not migrate) to https://github.com/rust-lang/rust/pull/58524#issuecomment-464863966.

oli-obk commented 5 years ago

Open to revision

So my revision suggestion is

Largely uncontroversial (close to objective improvement)

Enclose variable names / code within all comments in backticks.

will be solved by enabling certain clippy lints (and maybe improving them). Running clippy on rustc is a WIP (https://github.com/rust-lang/rust/pull/56595), but seems on the horizon.

Enclose filenames/paths within all comments in backticks.

can be a new clippy lint, I think we already detect urls

Begin full sentences in comments with a capital letter.

let's wait for doc rules and enforcement tools

Favour doc comments over regular comments where relevant to anyone reading the rustdoc documentation.

Should probably be part of rustfmt. I personally would be ok with a PR just doing this and nothing else. Even if not yet automated, it's a directly visible doc improvement. I've been doing this as drive-by fixes when applicable.

Use present indicative verb rather than imperative or any other form for doc comments on fns/methods. (I believe this is part of the library style guide already.)

Hard to enforce automatically, but maybe look at other languages that have enforcement tools and see how/where to integrate that in a tool.

Code formatting changes to conform with rustfmt (particularly regarding whitespace).

Yes, that is something almost everyone is ok with, but it regresses fast, so needs x.py inclusion and potentially CI automation. Should probably have its own tracking issue.

Somewhat controversial, but already very inconsistent in the codebase

These are exactly what caused the entire controversy. Once the FCP is over, PRs containing just these changes will be closed. Drive-by changes to docs and comments related to the logical changes in a PR are obviously still ok as long as they are just that (i.e. aren't overwhelming or obfuscating the actual changes or otherwise becoming a review burden).

alexreg commented 5 years ago

@oli-obk Thanks for the feedback.

Largely uncontroversial (close to objective improvement)

Enclose variable names / code within all comments in backticks.

will be solved by enabling certain clippy lints (and maybe improving them). Running clippy on rustc is a WIP (#56595), but seems on the horizon.

I think this is very hard to automate, as @nikomatsakis was saying. I'm not sure it will ever be possible.

Enclose filenames/paths within all comments in backticks.

can be a new clippy lint, I think we already detect urls

That's possible to lint automatically yes. Is there really any problem with fixing them now though?

Begin full sentences in comments with a capital letter.

let's wait for doc rules and enforcement tools

Possibly. I'm not sure how well this can be automated though. There are some annoying edge cases.

Favour doc comments over regular comments where relevant to anyone reading the rustdoc documentation.

Should probably be part of rustfmt. I personally would be ok with a PR just doing this and nothing else. Even if not yet automated, it's a directly visible doc improvement. I've been doing this as drive-by fixes when applicable.

Fair enough.

Use present indicative verb rather than imperative or any other form for doc comments on fns/methods. (I believe this is part of the library style guide already.)

Hard to enforce automatically, but maybe look at other languages that have enforcement tools and see how/where to integrate that in a tool.

For the future, I agree, but worth integrating these changes for now, yes?

Code formatting changes to conform with rustfmt (particularly regarding whitespace).

Yes, that is something almost everyone is ok with, but it regresses fast, so needs x.py inclusion and potentially CI automation. Should probably have its own tracking issue.

Yep, for sure. Shall we leave these in though?

Somewhat controversial, but already very inconsistent in the codebase

These are exactly what caused the entire controversy. Once the FCP is over, PRs containing just these changes will be closed. Drive-by changes to docs and comments related to the logical changes in a PR are obviously still ok as long as they are just that (i.e. aren't overwhelming or obfuscating the actual changes or otherwise becoming a review burden).

Okay, so you're saying let's just exclude all of these? I guess I can live with that. :-)

oli-obk commented 5 years ago

Automating backtick additions is hard, that's why the clippy lint errs on the side of only linting camel case words. We might be able to extend this in the future, so i'd leave it to comment authors and lints. One "easy" extension could be file paths, so since there's a path forward, we can just wait until clippy's lint gets such a feature.

Wrt starting sentences with a capital letter, can we make a "good enough" algorithm that finds most situations? Some false negatives are fine by me. Maybe consider a rustfmt/style guide rfc?

Wrt present indicative. This seems like something a spell checker/grammar checker can do. We have a longstanding issue in clippy for doc comments spell check that "just" needs implementation.

Rustfmt changes can be made in PRs that just touch a single crate, just like we have been doing in the past. Though we should setup automation for that as already mentioned.

nikomatsakis commented 5 years ago

@petrochenkov

Also, a private chat shouldn't generally turn the course 180 degrees from the team consensus, that undermines the FCP process.

I don't really feel like the course has turned in particular? I was reporting what @alexreg and I discussed offline precisely because I wanted to bring it here to a more public forum (which we have now done).

I think even re-opening this discussion was a mistake, since now it's draining time and energy from participants again.

I won't deny feeling pretty drained by this discussion!

alexreg commented 5 years ago

I won't deny feeling pretty drained by this discussion!

Yep, you're not alone there! But I think being overly dogmatic and uncompromising about allowing in a core set of changes now is probably not very helpful...

anp commented 5 years ago

But I think being overly dogmatic and uncompromising about allowing in a core set of changes now is probably not very helpful...

Maybe you could clarify which request(s) you view as dogmatic and uncompromising? As a bystander, I am very surprised that this conversation remains ongoing when the opportunity cost of large, cross-cutting, subjective cosmetic PRs is clearly very high. And this high opportunity cost is paid in an environment where "number of things that can land in a day" is a pretty small value.

alexreg commented 5 years ago

That's the whole point: there are a number of points that aren't terribly subjective. Or at worst, only a small amount of subjectivity, that shouldn't cause vehement objection or disagreement. I think there needs to be more appreciation for this sort of work, that cleans up the codebase and makes it more readable...

Anyway, I'll be opening a new much-reduced PR soon, which is split roughly by change type. Then hopefully people can recognise that we're making a good conservative compromise, and things will get merged.

oli-obk commented 5 years ago

I advise against putting additional work into this. When the FCP is over, we will close these PRs unreviewed. I have laid out plans for how to address the style topics you want to address. Please consider working along those lines.

alexreg commented 5 years ago

@oli-obk Did you see @nikomatsakis's proposal above? He seems to be advocating a middle path, which I'm in favour of too.

anp commented 5 years ago

That's the whole point: there are a number of points that aren't terribly subjective. Or at worst, only a small amount of subjectivity, that shouldn't cause vehement objection or disagreement.

A “cosmetic” PR is definitionally subjective unless we’ve collectively defined what “improved appearance” means, ie in some team consensus process, ideally with community input. As a practical example, I do not find myself in agreement with your earlier list of changes under "Largely uncontroversial (close to objective improvement)".

In terms of building that consensus, I did see the irlo thread you linked, and I suspect the lack of engagement could be related to:

I think there needs to be more appreciation for this sort of work, that cleans up the codebase and makes it more readable...

To be blunt: why? I aspire to this standard myself, but I do not accept that it has intrinsic value beyond pleasing my sensibilities. It has value beyond pleasing me when it aids contribution, adoption, understanding, and collaboration, especially in a community. What’s the practical benefit of these sorts of changes? Are rust users experiencing bugs because of confused contributors who read a wrong code comment? Is there a survey somewhere identifying this as an obstacle to onboarding interested rust compiler hackers? Are prose confusions bogging down design conversations or RFCs? Are non-cosmetic PRs being held up by disagreements on prose style? There are potentially many convincing and practically relevant cases to be made here, and if you want you should try to state them more explicitly. However, if you motivate these PRs based on that argument of observable value, you need to be prepared to discuss that concrete value and its cost with the teams, and also to weigh them against other things of value that we’d all like to have. I have not yet seen an analysis or argument with clarity in this regard.

anp commented 5 years ago

As a followup: if a cosmetic PR lands and requires even a single feature PR to handle a merge conflict, why should we consider that cost acceptable? This is the core of what's unaddressed here, IMO.

EDIT: upon reskimming this thread, I wanted to add:

First, I feel bad that @alexreg feels bad.

I do too, but I don't think the solution is to continue discussing which approach to take to wide-reaching cosmetic changes. I worry that @alexreg feeling bad (again, <3 and I'm really sorry that we're ending up opposed in this thread) would have been best avoided with a clearer expression of project priorities earlier on and an invitation to explicitly plan out preferred approaches to codebase readability, if at all.

oli-obk commented 5 years ago

Did you see @nikomatsakis's proposal above? He seems to be advocating a middle path, which I'm in favour of too.

Yes. I don't think changing comments to doc comments is a contended point at all and I'm going to assume that PRs just doing that (and not any other changes) are perfectly alright with everyone and will get rolled up.

Wrt to writing variable names in backticks, as a rule for new comments or drive by fixes these do seem uncontroversial, but doing a sweeping change PR for them hits all the same reviewability and merge conflict problems that we want to avoid. I think once we have the clippy infrastructure in place, adding a style guide and partially enforcing it via clippy, and partially via review of new PRs is a good plan.

alexreg commented 5 years ago

@anp I’m sorry, but I find your first comment both arrogant and insensitive. I won’t reply further, because all the points you memtion have been discussed before.

anp commented 5 years ago

@alexreg i’m really very sorry for hurting your feelings here. i’d definitely be open to discussing this further (no pressure, i hear that you’re not right now). If others here feel similarly about my statements, please feel free to message me about it or post here — I really really value empathy and self awareness and have been in a process of active improvement in these areas for much of my adult life. That process benefits a lot from the good faith feedback of others and while it’s hard I get a lot of value from listening to it.

Regardless of your feelings about me or my comments, I do hope that if you press forward on these kinds of changes that you’ll find a way to more effectively communicate the concrete value of these efforts to those who are reviewing the PRs. While I agree these points have been raised previously, I’m not sure they’ve been resolved or really addressed.

alexreg commented 5 years ago

@anp Thanks for being more sensitive. I'm sorry if my impression was mistaken too. It's just this is very difficult from my position, I hope you appreciate... it feels like there are a bunch of people ganging up on me out there (and coming from a rather dogmatic stance), and it's hard to see why this compromise or "middle way" can't be accepted for the existing subset of work (so as not to waste all the effort) before this FCP is settled (which I would then have no qualms about). This discussion has been going on a while now, and draining a bunch of us.

You're right, all the points have been mentioned and discussed though not entirely settled. I'm not sure if they ever will be, though perhaps that's too pessimistic!

alexreg commented 5 years ago

(And sure, I'm open to a brief friendly chat or something, if you feel like contacting me on Discord/Zulip...)

nikomatsakis commented 5 years ago

I've been pretty busy over the last few days, but I've been giving this thread some thought. I believe my feeling is the following:

As @oli-obk said, I would separate out the conversion of // comments to /// comments. This has clear external value and if there were a PR that just did that, I would say we should review it.

With respect to the other changes, I find that there are some that I agree with, and some that I don't. This is certainly a sign that others will have their own opinions. As a group, I think we've generally come to the consensus that we don't feel that litigating these opinions into a set of binding guidelines is worth our time, at least not now. As a result, I think it makes sense to say that we should not accept PRs that apply such diffs to code that is not otherwise being changed (the occasional correction seems fine, I mean in large number). Such PRs seem like they just generate ill-feeling, both on the part of the PR author (who put in a lot of work) and the reviewer (who has to litigate these conflicts and judge these diffs); they also can cause merge conflicts and (mildly, but still) erode the usefulness of git history.

At this point, @alexreg, I think it makes sense to simply close the PRs in question. I wouldn't bother trying to extract changes from them. I do regret the confusion around this issue, and that you will lose the work you put into them. Sometimes that happens though. :( In retrospect, I think that when we first discussed the issue we should not have tried to steer a "laissez faire" middle course. Live and learn. (That said, I would very much like to continue working with you on the various trait-related changes we've been pursuing, independent from this.)

With respect to the idea of a "middle way", I am again sympathetic, but it's not really clear to me what separates the existing PR from future PRs, apart from the fact that the work is done. I think it would be semi-reasonable to accept the changes, but I also think it's perfectly reasonable -- indeed, more consistent -- to not want to.

alexreg commented 5 years ago

Alright, I give up. You win. I'm still convinced this approach is overly-dogmatic and a tad draconian, but I appreciate you're trying to aim for consistency at the same time, so it's not like I don't understand the alternative viewpoints. That said, I've already made my feelings manifest about how I consider a lack of concern about quality of comments and code formatting to be very sub-par software engineering, but I confess that's a personal view.

Just for the sake of candour, this will probably mean less keenness on my half to contribute to a codebase with a number of "unpleasantries", so to speak, but hopefully I'll still be able to contribute on some key functionality that I want to see enter Rust.

Finally, since people have apparently decided that in the future conservative cosmetic changes will be accepted to parts of code that are already being modified as part of functional PRs (presumably as separate commits), I offer a branch of such conservative cosmetic improvements here, in the hope (but not the expectation!) people may integrate these piecemeal when submitting their PRs.

Right now, I'm just glad this debate is over, even if it's not with the solution I would I have liked. For that reason, I'd appreciate to be left out of future discussion on this topic. Of course, I hope we can all return to civility and friendliness with each other. :-)

oli-obk commented 5 years ago

Finally, since people have apparently decided that in the future conservative cosmetic changes will be accepted to parts of code that are already being modified as part of functional PRs

That is not what has been meant by drive-by fixes. Please do not explicitly seek out cosmetic changes to apply to functional PRs. Doing so would increase the amount of work for you, PR authors and PR reviewers, when the purpose of this issue was to achieve the opposite.

Just to be clear, I personally want most of these changes once we can automatically apply them.

alexreg commented 5 years ago

Then you definitely meed to clarify what you mean by “drive-by” fixes, because that is the intuitive reading in my opinion. FYI, unsubscribed to this thread already as per above, but can be reached elsewhere.

oli-obk commented 5 years ago

Drive-by fixes as I see them, are things that you notice while modifying code. E.g. when changing a function argument one may also change the function's indentation to block style:

// before
fn long_function_name(&self,
                      first_arg: Type,
                      second_arg: Type) -> ResultType {
    // code here
}

// after
fn long_function_name(
    &self,
    first_arg: Type,
    new_arg_name: NewArgType,
) -> ResultType {

Even though one did not need to change the layout to fit the style guide, this change follows the style guide and one was touching the code anyway.

Similarly, when you're writing code in a file and while trying to figure out what something does, you are reading docs of functions. While reading the docs you stumble over variables not within backticks, so you throw backticks around them. That's all good. We don't have and don't want rules around how many changes are ok, but one should always remember that reviewing code changes intermingled between style changes is harder. So keeping the style changes small is good.

Actively seeking out more style changes to apply and insert into the PR is therefor not desirable, because you'd be increasing the review burden of the PR. PRs that are hard to review will take longer to get merged and thus have more merge conflicts and then take even longer to get merged.

alexreg commented 5 years ago

Okay, that's basically what I meant. Thanks for clarifying.

rfcbot commented 5 years ago

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.