From ddd945bd81d12d02b77a26128ef5a2f5bbb9e856 Mon Sep 17 00:00:00 2001 From: Michael Woerister Date: Tue, 20 Jul 2021 16:09:28 +0200 Subject: [PATCH 1/9] Update review policy as per https://github.com/rust-lang/compiler-team/issues/444 --- src/compiler/reviews.md | 168 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 160 insertions(+), 8 deletions(-) diff --git a/src/compiler/reviews.md b/src/compiler/reviews.md index 843226254..6f1194db5 100644 --- a/src/compiler/reviews.md +++ b/src/compiler/reviews.md @@ -1,4 +1,156 @@ -# Review policies +Review Policy +============= + +It is the purpose of code reviews to + +- **reduce** the risk of introducing **bugs** and usability and performance **regressions**, +- keep our code **maintainable**, **readable**, and **documented**, and +- ensure that changes are made with the big picture and **appropriate context in mind**. + +Reviewing accomplishes this by bringing in another set of eyes +to look at a proposed change from a different perspective, +which increases the chance of catching mistakes early and +uncovering potential blind spots in the reasoning behind the change. + + +## Basic Reviewing Requirements + +There are a number of requirements that need to be met in order for reviewing to be effective: + +- Reviewers must have a sufficient **understanding of the code** under review. + > This is important to help spot non-obvious, unintentional side effects of a given change. +- Pull requests must provide (1) a concise high-level **description of the change** and (2) the **rationale** behind it. + For the rational to be even more useful, authors are encouraged to list potential **points of contention**. + > Reviewing code is difficult and reviewers only have a limited amount of time to do it. + > Jump-starting the review process by not making the reviewer puzzle together the intention and context + > of a pull request will not only speed things up but also improve the quality of the review. +- Reviewers must have a good idea on whether they are the **right person to approve** the change. + > Knowledge of the code under review is an obvious but not the only criterium for answering this question. + > The reviewer also needs to decide if they can make the decision alone or if the PR needs to go through + > the [major change process][mcp], if they can perform the review in a timely fashion, + > and if they are impartial enough to provide a sufficiently unbiased perspective. + + + +## Reviewing Checklist + +The following list of questions (to be posted as part highfive bot's automatic PR response) +will help reviewers and PR authors alike to bring PRs into good shape and meet the above criteria: + +> #### Checklist for PR authors and reviewers +> * Does the PR message have +> * a concise **high-level description** of the changes? (*what* is being changed) +> * a clear **rationale** for doing them? (*why* is it being changed) +> * a list of potential **points of contention**? +> * **links to relevant issues**, RFCs, MCPs, etc? +> * Does the PR need a **regression test**? +> * Does the change need to be covered by a **[major change proposal][mcp]**? Is it already covered? +> * Would someone trying to understand the PR in a year's time be able to quickly **reconstruct what's going on**? +> * Is the new code **properly documented**? Is existing documentation still up-to-date? +> * Does the PR introduce a **regression** of any of the following: +> * error message quality +> * maintainability (e.g. complex code, no documentation, unsafe) +> * any specific target platforms +> * downstream tooling (e.g. linkers, debuggers) +> * compile times +> * memory usage +> +> #### Checklist for reviewers +> * Am I the **right person to review** this PR: +> * Do I understand the code well enough? +> * Would I be able to spot non-obvious side effects? +> * Would I be able to fix a bug introduced by this PR? +> * Can I do the review in a timely fashion? +> * Do I feel pressure to quickly approve the PR for some reason? +> * Am I impartial enough? +> * Before merging: +> * Is the **PR message still accurate**? +> * Is the **commit history clean** enough? + + + +## Guidance for dealing with common situations + +In most cases common sense is enough for deciding how to apply this policy. +However, sometimes there are gray areas where it is not immediately clear how to proceed. +This section lists a few common cases together with guidance on how to deal with them. + +* ### I don't think I am a good fit for reviewing - what now? + It is completely normal that you get assigned a PR (via highfive or otherwise) but don't feel comfortable reviewing it. + Here is what you can do, depending on the concrete case: + + - If the change seems really big or contentious, consider asking for an MCP (see below). + - If you know just the right person for the review, assign them via `r? @`. + It's polite to leave a comment asking them if they can take over -- + but you don't have to make sure beforehand that they can actually do it. + - If you don't know the code well or already have too much on your plate, + ask highfive to roll the dice again via `r? @rust-lang/compiler-contributors`. + + You can also always ask specific compiler team members for help finding a reviewer. + +* ### It is unclear if something constitutes a major change + Deciding if something is a "major change" requiring an [MCP][mcp] is not always straightforward. + The official guidelines are [here][whats-a-major-change]. + When in doubt, err on side of treating something as a major change. + You can also nominate the PR for discussion in the compiler team's triage meeting by tagging it `I-nominated`. + If you nominate a PR please make sure to state a concrete question for the compiler team to discuss. + +* ### Intransparent discussion and rationale + Sometimes there are PRs that seem to be the result of some prior discussion, with no description or rationale. + They usually have a title like "Change X" and the only content of the PR message is "r? @xyz". + Even though the change might make sense and may even have been suggested by a compiler team member this is not good form. + The PR message should give a self-contained description of what is being changed, + why it is being changed and anything else that might be of interest. + Try to put yourself in the shoes of someone who, a few years down the road, + needs to fix a bug related to the code touched by the PR and needs to reconstruct the rationale for the way things are. + +* ### Reviewer and PR author report to the same entity / work for the same employer + There is no rule that prevents two employees of the same company from reviewing each other's PRs. + The concerns in such a case are no different than for any other two reviewers. + We expect the mechanisms and principles we articulated above to be respected by ALL reviewers, whatever their employer. + Does the PR concisely describe the changes that are being made? + Does it give a clear, transparent rationale for why the changes make sense + so that contributors down the line can follow the reasoning and reconstruct what's going on? + Have points of contention been discussed and cleared up? + Then you are in the clear. + + If you are in doubt if something is contentious, give a heads up to `@rust-lang/compiler-contributors` and ask for another opinion. + If the proposed change is large and/or potentially has a big impact, create a [major change proposal][mcp]. + +* ### Reviewing and Mentoring + In the course of mentoring someone through a PR it often happens that the reviewer has ended up effectively co-writing the changes. + This can be a tricky case because the reviewer is effectively approving their own changes. + There are a number of considerations to take into account when deciding how to proceed: + + - If the general direction of the changes has already been approved as part of an MCP and the concrete advice given + during mentoring was only concerned with resolving minor technical issues, then no further review is required. + - Similarly, if any contentious decisions have visibly been discussed and resolved on the PR with other + compiler team contributors and the rest of the changes don't deviate from the general direction that has been + agreed upon then no further review is required either. + - If the PR was opened as a response to a concrete suggestion by the reviewer (and the changes are not entirely trivial) + then it is advisable that the final review is done by someone else. + However, the initial reviewer/mentor is encouraged to help bring the PR into good shape before handing it off. + + In general, it is advisable to ask for a second opinion by someone knowledgable in the field in such cases, + just to increase the chance of uncovering oversights and blindspots a mentor might have. + +* ### Nobody understands the code that's being changed + Sometimes there is a bug in some code that nobody understands anymore. + The original authors are unavailable and it is hard to gauge the implications of a proposed fix. + In such a case it is a good idea for reviewers to nominate the PR (by tagging it with `I-nominated`) + in order to get it in front of more eyes during the compiler team's triage meeting. + In such cases it is also especially valuable to gather and document as much information as possible on the issue, + such as a description of the problem being fixed, points of unclarity, potential risks, + alternatives that have been considered, et cetera. + Reviewers should ask PR authors to add this kind of information as comments in the code + and/or to the PR message (which will become part of the git commit history). + + +[mcp]: https://forge.rust-lang.org/compiler/mcp.html +[whats-a-major-change]: https://forge.rust-lang.org/compiler/mcp.html#what-constitutes-a-major-change + + +## Technical Aspects of Reviewing Every PR that lands in the compiler and its associated crates must be reviewed by at least one person who is knowledgeable with the code in @@ -11,7 +163,7 @@ will automatically assign someone. It is common to leave a `r? @username` comment at some later point to request review from someone else. This will also reassign the PR. -## bors +### bors We never merge PRs directly. Instead, we use bors. A qualified reviewer with bors privileges (e.g., a [compiler @@ -27,7 +179,7 @@ delegate+` or `@bors delegate=username`. This will allow the PR author to approve the PR by issuing `@bors` commands like the ones above (but this privilege is limited to the single PR). -## Reverts +### Reverts If a merged PR is found to have caused a meaningful unanticipated regression, the best policy is to revert it quickly and re-land it later once a fix and @@ -49,7 +201,7 @@ with one or more compiler team members pointing to this PR, or it's simply obvious to everyone involved). Only revert with lower certainty if the issue is particularly critical or urgent to fix. -### Creating reverts +#### Creating reverts The easiest method for creating a revert is to use the "Revert" button on Github. This appears next to the "bors merged commit abcd" message on a pull @@ -93,7 +245,7 @@ Generally speaking, reverts should have elevated priority and match the `rollup` status of the PR they are reverting. If a non-rollup PR is shown to have no impact on performance, it can be marked `rollup=always`. -### Forward fixes +#### Forward fixes Often it is tempting to address a regression by posting a follow-up PR that, rather than reverting the regressing PR, instead augments the original in @@ -114,7 +266,7 @@ most cases it is much easier to land the PR a second time once a fix can be confirmed. Allowing a revert to land takes pressure off of you and your reviewers to act quickly and gives you time to address the issue fully. -## Rollups +### Rollups All reviewers are strongly encouraged to explicitly mark a PR as to whether or not it should be part of a [rollup]. This is usually done either when approving a @@ -156,7 +308,7 @@ is substituted with one of the following: - `rollup`: this is equivalent to `rollup=always` - `rollup-`: this is equivalent to `rollup=maybe` -## Priority +### Priority Reviewers are encouraged to set one of the rollup statuses listed above instead of setting priority. Bors automatically sorts based on the rollup @@ -186,7 +338,7 @@ The following is some guidance for setting priorities: - Absolutely critical fixes - Release promotions -## Expectations for r+ +### Expectations for r+ bors privileges are binary: the bot doesn't know which code you are familiar with and what code you are not. They must therefore be used From b761423b5282d395d590dafafff9165cac7b3380 Mon Sep 17 00:00:00 2001 From: Michael Woerister Date: Wed, 21 Jul 2021 14:10:24 +0200 Subject: [PATCH 2/9] Incorporate feedback to review policy update. --- src/compiler/reviews.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/compiler/reviews.md b/src/compiler/reviews.md index 6f1194db5..732c4854c 100644 --- a/src/compiler/reviews.md +++ b/src/compiler/reviews.md @@ -20,7 +20,7 @@ There are a number of requirements that need to be met in order for reviewing to - Reviewers must have a sufficient **understanding of the code** under review. > This is important to help spot non-obvious, unintentional side effects of a given change. - Pull requests must provide (1) a concise high-level **description of the change** and (2) the **rationale** behind it. - For the rational to be even more useful, authors are encouraged to list potential **points of contention**. + For the rationale to be even more useful, authors are encouraged to list potential **points of contention**. > Reviewing code is difficult and reviewers only have a limited amount of time to do it. > Jump-starting the review process by not making the reviewer puzzle together the intention and context > of a pull request will not only speed things up but also improve the quality of the review. @@ -84,9 +84,12 @@ This section lists a few common cases together with guidance on how to deal with It's polite to leave a comment asking them if they can take over -- but you don't have to make sure beforehand that they can actually do it. - If you don't know the code well or already have too much on your plate, - ask highfive to roll the dice again via `r? @rust-lang/compiler-contributors`. + ask highfive to roll the dice again via `r? rust-lang/compiler-contributors`. You can also always ask specific compiler team members for help finding a reviewer. + That being said, you are always welcome to do an initial review (to the extent you are comformtable with) + and then pass the PR on to the final reviewer. + This way the PR author will get helpful feedback sooner and subsequent reviewers will have less work to do. * ### It is unclear if something constitutes a major change Deciding if something is a "major change" requiring an [MCP][mcp] is not always straightforward. From 367f0f95654cf3d6d5b06c7959b2443e2c0fe8b0 Mon Sep 17 00:00:00 2001 From: Michael Woerister Date: Fri, 23 Jul 2021 15:36:48 +0200 Subject: [PATCH 3/9] Incorporate more feedback to review policy update. --- src/compiler/reviews.md | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/compiler/reviews.md b/src/compiler/reviews.md index 732c4854c..fb55abf80 100644 --- a/src/compiler/reviews.md +++ b/src/compiler/reviews.md @@ -19,15 +19,16 @@ There are a number of requirements that need to be met in order for reviewing to - Reviewers must have a sufficient **understanding of the code** under review. > This is important to help spot non-obvious, unintentional side effects of a given change. -- Pull requests must provide (1) a concise high-level **description of the change** and (2) the **rationale** behind it. - For the rationale to be even more useful, authors are encouraged to list potential **points of contention**. +- Pull request authors must provide (1) a concise high-level **description of the change** and (2) the **rationale** behind it. + For the rationale to be even more useful, authors are encouraged to list potential **points of contention**, + **compromises** that needed to be made, **alternative approaches** that have been considered, et cetera. > Reviewing code is difficult and reviewers only have a limited amount of time to do it. > Jump-starting the review process by not making the reviewer puzzle together the intention and context > of a pull request will not only speed things up but also improve the quality of the review. - Reviewers must have a good idea on whether they are the **right person to approve** the change. > Knowledge of the code under review is an obvious but not the only criterium for answering this question. > The reviewer also needs to decide if they can make the decision alone or if the PR needs to go through - > the [major change process][mcp], if they can perform the review in a timely fashion, + > the [major change process][mcp], if they can perform the review sufficiently thorough and in a timely fashion, > and if they are impartial enough to provide a sufficiently unbiased perspective. @@ -86,7 +87,7 @@ This section lists a few common cases together with guidance on how to deal with - If you don't know the code well or already have too much on your plate, ask highfive to roll the dice again via `r? rust-lang/compiler-contributors`. - You can also always ask specific compiler team members for help finding a reviewer. + You can also always ask for help on the `#t-compiler/reviews` Zulip stream for finding a reviewer. That being said, you are always welcome to do an initial review (to the extent you are comformtable with) and then pass the PR on to the final reviewer. This way the PR author will get helpful feedback sooner and subsequent reviewers will have less work to do. @@ -140,9 +141,8 @@ This section lists a few common cases together with guidance on how to deal with * ### Nobody understands the code that's being changed Sometimes there is a bug in some code that nobody understands anymore. The original authors are unavailable and it is hard to gauge the implications of a proposed fix. - In such a case it is a good idea for reviewers to nominate the PR (by tagging it with `I-nominated`) - in order to get it in front of more eyes during the compiler team's triage meeting. - In such cases it is also especially valuable to gather and document as much information as possible on the issue, + In such a case it is a good idea for reviewers to ask on the `#t-compiler/reviews` Zulip stream for additional help. + It is also especially valuable to gather and document as much information as possible about the issue, such as a description of the problem being fixed, points of unclarity, potential risks, alternatives that have been considered, et cetera. Reviewers should ask PR authors to add this kind of information as comments in the code From 430ec5017472144dda925359386cfe2bea4827fa Mon Sep 17 00:00:00 2001 From: Michael Woerister Date: Wed, 13 Dec 2023 11:24:49 +0100 Subject: [PATCH 4/9] Update mention of highfive bot --- src/compiler/reviews.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/compiler/reviews.md b/src/compiler/reviews.md index fb55abf80..2c6c9f199 100644 --- a/src/compiler/reviews.md +++ b/src/compiler/reviews.md @@ -35,7 +35,7 @@ There are a number of requirements that need to be met in order for reviewing to ## Reviewing Checklist -The following list of questions (to be posted as part highfive bot's automatic PR response) +The following list of questions (to be posted as part rustbot's automatic PR response) will help reviewers and PR authors alike to bring PRs into good shape and meet the above criteria: > #### Checklist for PR authors and reviewers @@ -77,7 +77,7 @@ However, sometimes there are gray areas where it is not immediately clear how to This section lists a few common cases together with guidance on how to deal with them. * ### I don't think I am a good fit for reviewing - what now? - It is completely normal that you get assigned a PR (via highfive or otherwise) but don't feel comfortable reviewing it. + It is completely normal that you get assigned a PR (via rustbot or otherwise) but don't feel comfortable reviewing it. Here is what you can do, depending on the concrete case: - If the change seems really big or contentious, consider asking for an MCP (see below). @@ -85,7 +85,7 @@ This section lists a few common cases together with guidance on how to deal with It's polite to leave a comment asking them if they can take over -- but you don't have to make sure beforehand that they can actually do it. - If you don't know the code well or already have too much on your plate, - ask highfive to roll the dice again via `r? rust-lang/compiler-contributors`. + ask rustbot to roll the dice again via `r? compiler`. You can also always ask for help on the `#t-compiler/reviews` Zulip stream for finding a reviewer. That being said, you are always welcome to do an initial review (to the extent you are comformtable with) From a2e6ba08574a5ee6d2dc3ef936c8ca1a673734bb Mon Sep 17 00:00:00 2001 From: Michael Woerister Date: Tue, 19 Dec 2023 10:42:25 +0100 Subject: [PATCH 5/9] Apply review suggestions --- src/compiler/reviews.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/compiler/reviews.md b/src/compiler/reviews.md index 2c6c9f199..11ab40fed 100644 --- a/src/compiler/reviews.md +++ b/src/compiler/reviews.md @@ -87,7 +87,7 @@ This section lists a few common cases together with guidance on how to deal with - If you don't know the code well or already have too much on your plate, ask rustbot to roll the dice again via `r? compiler`. - You can also always ask for help on the `#t-compiler/reviews` Zulip stream for finding a reviewer. + You can also always ask for help on the `#t-compiler` Zulip stream for finding a reviewer. That being said, you are always welcome to do an initial review (to the extent you are comformtable with) and then pass the PR on to the final reviewer. This way the PR author will get helpful feedback sooner and subsequent reviewers will have less work to do. @@ -99,7 +99,7 @@ This section lists a few common cases together with guidance on how to deal with You can also nominate the PR for discussion in the compiler team's triage meeting by tagging it `I-nominated`. If you nominate a PR please make sure to state a concrete question for the compiler team to discuss. -* ### Intransparent discussion and rationale +* ### Discussion or rationale is too intransparent Sometimes there are PRs that seem to be the result of some prior discussion, with no description or rationale. They usually have a title like "Change X" and the only content of the PR message is "r? @xyz". Even though the change might make sense and may even have been suggested by a compiler team member this is not good form. @@ -141,14 +141,15 @@ This section lists a few common cases together with guidance on how to deal with * ### Nobody understands the code that's being changed Sometimes there is a bug in some code that nobody understands anymore. The original authors are unavailable and it is hard to gauge the implications of a proposed fix. - In such a case it is a good idea for reviewers to ask on the `#t-compiler/reviews` Zulip stream for additional help. + In such a case it is a good idea for reviewers to I-nominate the PR (if they intend to stay the + main reviewer) or assign a compiler team lead to the issue and add the S-waiting-on-team label. + In both cases, the PR will be brought in the weekly triage meeting. It is also especially valuable to gather and document as much information as possible about the issue, such as a description of the problem being fixed, points of unclarity, potential risks, alternatives that have been considered, et cetera. Reviewers should ask PR authors to add this kind of information as comments in the code and/or to the PR message (which will become part of the git commit history). - [mcp]: https://forge.rust-lang.org/compiler/mcp.html [whats-a-major-change]: https://forge.rust-lang.org/compiler/mcp.html#what-constitutes-a-major-change From f41bb9efeedead1fcdb82868e997e5dc9b56f4b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=AE=B8=E6=9D=B0=E5=8F=8B=20Jieyou=20Xu=20=28Joe=29?= <39484203+jieyouxu@users.noreply.github.com> Date: Tue, 10 Dec 2024 12:41:01 +0800 Subject: [PATCH 6/9] Reflow markdown to make follow-up edits easier --- src/compiler/reviews.md | 329 ++++++++++++++++++++++------------------ 1 file changed, 181 insertions(+), 148 deletions(-) diff --git a/src/compiler/reviews.md b/src/compiler/reviews.md index 11ab40fed..85fac42d2 100644 --- a/src/compiler/reviews.md +++ b/src/compiler/reviews.md @@ -3,51 +3,66 @@ Review Policy It is the purpose of code reviews to -- **reduce** the risk of introducing **bugs** and usability and performance **regressions**, +- **reduce** the risk of introducing **bugs** and usability and performance + **regressions**, - keep our code **maintainable**, **readable**, and **documented**, and -- ensure that changes are made with the big picture and **appropriate context in mind**. +- ensure that changes are made with the big picture and **appropriate context in + mind**. -Reviewing accomplishes this by bringing in another set of eyes -to look at a proposed change from a different perspective, -which increases the chance of catching mistakes early and -uncovering potential blind spots in the reasoning behind the change. +Reviewing accomplishes this by bringing in another set of eyes to look at a +proposed change from a different perspective, which increases the chance of +catching mistakes early and uncovering potential blind spots in the reasoning +behind the change. ## Basic Reviewing Requirements -There are a number of requirements that need to be met in order for reviewing to be effective: +There are a number of requirements that need to be met in order for reviewing to +be effective: - Reviewers must have a sufficient **understanding of the code** under review. - > This is important to help spot non-obvious, unintentional side effects of a given change. -- Pull request authors must provide (1) a concise high-level **description of the change** and (2) the **rationale** behind it. - For the rationale to be even more useful, authors are encouraged to list potential **points of contention**, - **compromises** that needed to be made, **alternative approaches** that have been considered, et cetera. - > Reviewing code is difficult and reviewers only have a limited amount of time to do it. - > Jump-starting the review process by not making the reviewer puzzle together the intention and context - > of a pull request will not only speed things up but also improve the quality of the review. -- Reviewers must have a good idea on whether they are the **right person to approve** the change. - > Knowledge of the code under review is an obvious but not the only criterium for answering this question. - > The reviewer also needs to decide if they can make the decision alone or if the PR needs to go through - > the [major change process][mcp], if they can perform the review sufficiently thorough and in a timely fashion, - > and if they are impartial enough to provide a sufficiently unbiased perspective. + > This is important to help spot non-obvious, unintentional side effects of + > a given change. +- Pull request authors must provide (1) a concise high-level **description of + the change** and (2) the **rationale** behind it. For the rationale to be even + more useful, authors are encouraged to list potential **points of + contention**, **compromises** that needed to be made, **alternative + approaches** that have been considered, et cetera. + > Reviewing code is difficult and reviewers only have a limited amount of + > time to do it. Jump-starting the review process by not making the reviewer + > puzzle together the intention and context of a pull request will not only + > speed things up but also improve the quality of the review. +- Reviewers must have a good idea on whether they are the **right person to + approve** the change. + > Knowledge of the code under review is an obvious but not the only + > criterium for answering this question. The reviewer also needs to decide + > if they can make the decision alone or if the PR needs to go through the + > [major change process][mcp], if they can perform the review sufficiently + > thorough and in a timely fashion, and if they are impartial enough to + > provide a sufficiently unbiased perspective. ## Reviewing Checklist -The following list of questions (to be posted as part rustbot's automatic PR response) -will help reviewers and PR authors alike to bring PRs into good shape and meet the above criteria: +The following list of questions (to be posted as part rustbot's automatic PR +response) will help reviewers and PR authors alike to bring PRs into good shape +and meet the above criteria: > #### Checklist for PR authors and reviewers > * Does the PR message have -> * a concise **high-level description** of the changes? (*what* is being changed) +> * a concise **high-level description** of the changes? (*what* is being +> changed) > * a clear **rationale** for doing them? (*why* is it being changed) > * a list of potential **points of contention**? > * **links to relevant issues**, RFCs, MCPs, etc? > * Does the PR need a **regression test**? -> * Does the change need to be covered by a **[major change proposal][mcp]**? Is it already covered? -> * Would someone trying to understand the PR in a year's time be able to quickly **reconstruct what's going on**? -> * Is the new code **properly documented**? Is existing documentation still up-to-date? +> * Does the change need to be covered by a **[major change proposal][mcp]**? Is +> it already covered? +> * Would someone trying to understand the PR in a year's time be able to +> quickly **reconstruct what's going on**? +> * Is the new code **properly documented**? Is existing documentation still +> up-to-date? > * Does the PR introduce a **regression** of any of the following: > * error message quality > * maintainability (e.g. complex code, no documentation, unsafe) @@ -73,115 +88,132 @@ will help reviewers and PR authors alike to bring PRs into good shape and meet t ## Guidance for dealing with common situations In most cases common sense is enough for deciding how to apply this policy. -However, sometimes there are gray areas where it is not immediately clear how to proceed. -This section lists a few common cases together with guidance on how to deal with them. +However, sometimes there are gray areas where it is not immediately clear how to +proceed. This section lists a few common cases together with guidance on how to +deal with them. * ### I don't think I am a good fit for reviewing - what now? - It is completely normal that you get assigned a PR (via rustbot or otherwise) but don't feel comfortable reviewing it. - Here is what you can do, depending on the concrete case: - - - If the change seems really big or contentious, consider asking for an MCP (see below). - - If you know just the right person for the review, assign them via `r? @`. - It's polite to leave a comment asking them if they can take over -- - but you don't have to make sure beforehand that they can actually do it. - - If you don't know the code well or already have too much on your plate, - ask rustbot to roll the dice again via `r? compiler`. - - You can also always ask for help on the `#t-compiler` Zulip stream for finding a reviewer. - That being said, you are always welcome to do an initial review (to the extent you are comformtable with) - and then pass the PR on to the final reviewer. - This way the PR author will get helpful feedback sooner and subsequent reviewers will have less work to do. + It is completely normal that you get assigned a PR (via rustbot or otherwise) + but don't feel comfortable reviewing it. Here is what you can do, depending on + the concrete case: + + - If the change seems really big or contentious, consider asking for an MCP + (see below). + - If you know just the right person for the review, assign them via `r? + @`. It's polite to leave a comment asking them if they can take + over -- but you don't have to make sure beforehand that they can actually do + it. + - If you don't know the code well or already have too much on your plate, ask + rustbot to roll the dice again via `r? compiler`. + + You can also always ask for help on the `#t-compiler` Zulip stream for finding + a reviewer. That being said, you are always welcome to do an initial review + (to the extent you are comformtable with) and then pass the PR on to the final + reviewer. This way the PR author will get helpful feedback sooner and + subsequent reviewers will have less work to do. * ### It is unclear if something constitutes a major change - Deciding if something is a "major change" requiring an [MCP][mcp] is not always straightforward. - The official guidelines are [here][whats-a-major-change]. - When in doubt, err on side of treating something as a major change. - You can also nominate the PR for discussion in the compiler team's triage meeting by tagging it `I-nominated`. - If you nominate a PR please make sure to state a concrete question for the compiler team to discuss. + Deciding if something is a "major change" requiring an [MCP][mcp] is not + always straightforward. The official guidelines are + [here][whats-a-major-change]. When in doubt, err on side of treating something + as a major change. You can also nominate the PR for discussion in the compiler + team's triage meeting by tagging it `I-nominated`. If you nominate a PR please + make sure to state a concrete question for the compiler team to discuss. * ### Discussion or rationale is too intransparent - Sometimes there are PRs that seem to be the result of some prior discussion, with no description or rationale. - They usually have a title like "Change X" and the only content of the PR message is "r? @xyz". - Even though the change might make sense and may even have been suggested by a compiler team member this is not good form. - The PR message should give a self-contained description of what is being changed, - why it is being changed and anything else that might be of interest. - Try to put yourself in the shoes of someone who, a few years down the road, - needs to fix a bug related to the code touched by the PR and needs to reconstruct the rationale for the way things are. + Sometimes there are PRs that seem to be the result of some prior discussion, + with no description or rationale. They usually have a title like "Change X" + and the only content of the PR message is "r? @xyz". Even though the change + might make sense and may even have been suggested by a compiler team member + this is not good form. The PR message should give a self-contained description + of what is being changed, why it is being changed and anything else that might + be of interest. Try to put yourself in the shoes of someone who, a few years + down the road, needs to fix a bug related to the code touched by the PR and + needs to reconstruct the rationale for the way things are. * ### Reviewer and PR author report to the same entity / work for the same employer - There is no rule that prevents two employees of the same company from reviewing each other's PRs. - The concerns in such a case are no different than for any other two reviewers. - We expect the mechanisms and principles we articulated above to be respected by ALL reviewers, whatever their employer. - Does the PR concisely describe the changes that are being made? - Does it give a clear, transparent rationale for why the changes make sense - so that contributors down the line can follow the reasoning and reconstruct what's going on? - Have points of contention been discussed and cleared up? - Then you are in the clear. - - If you are in doubt if something is contentious, give a heads up to `@rust-lang/compiler-contributors` and ask for another opinion. - If the proposed change is large and/or potentially has a big impact, create a [major change proposal][mcp]. + There is no rule that prevents two employees of the same company from + reviewing each other's PRs. The concerns in such a case are no different than + for any other two reviewers. We expect the mechanisms and principles we + articulated above to be respected by ALL reviewers, whatever their employer. + Does the PR concisely describe the changes that are being made? Does it give a + clear, transparent rationale for why the changes make sense so that + contributors down the line can follow the reasoning and reconstruct what's + going on? Have points of contention been discussed and cleared up? Then you + are in the clear. + + If you are in doubt if something is contentious, give a heads up to + `@rust-lang/compiler-contributors` and ask for another opinion. If the + proposed change is large and/or potentially has a big impact, create a [major + change proposal][mcp]. * ### Reviewing and Mentoring - In the course of mentoring someone through a PR it often happens that the reviewer has ended up effectively co-writing the changes. - This can be a tricky case because the reviewer is effectively approving their own changes. - There are a number of considerations to take into account when deciding how to proceed: - - - If the general direction of the changes has already been approved as part of an MCP and the concrete advice given - during mentoring was only concerned with resolving minor technical issues, then no further review is required. - - Similarly, if any contentious decisions have visibly been discussed and resolved on the PR with other - compiler team contributors and the rest of the changes don't deviate from the general direction that has been - agreed upon then no further review is required either. - - If the PR was opened as a response to a concrete suggestion by the reviewer (and the changes are not entirely trivial) - then it is advisable that the final review is done by someone else. - However, the initial reviewer/mentor is encouraged to help bring the PR into good shape before handing it off. - - In general, it is advisable to ask for a second opinion by someone knowledgable in the field in such cases, - just to increase the chance of uncovering oversights and blindspots a mentor might have. + In the course of mentoring someone through a PR it often happens that the + reviewer has ended up effectively co-writing the changes. This can be a tricky + case because the reviewer is effectively approving their own changes. There + are a number of considerations to take into account when deciding how to + proceed: + + - If the general direction of the changes has already been approved as part of + an MCP and the concrete advice given during mentoring was only concerned + with resolving minor technical issues, then no further review is required. + - Similarly, if any contentious decisions have visibly been discussed and + resolved on the PR with other compiler team contributors and the rest of the + changes don't deviate from the general direction that has been agreed upon + then no further review is required either. + - If the PR was opened as a response to a concrete suggestion by the reviewer + (and the changes are not entirely trivial) then it is advisable that the + final review is done by someone else. However, the initial reviewer/mentor + is encouraged to help bring the PR into good shape before handing it off. + + In general, it is advisable to ask for a second opinion by someone + knowledgable in the field in such cases, just to increase the chance of + uncovering oversights and blindspots a mentor might have. * ### Nobody understands the code that's being changed - Sometimes there is a bug in some code that nobody understands anymore. - The original authors are unavailable and it is hard to gauge the implications of a proposed fix. - In such a case it is a good idea for reviewers to I-nominate the PR (if they intend to stay the - main reviewer) or assign a compiler team lead to the issue and add the S-waiting-on-team label. - In both cases, the PR will be brought in the weekly triage meeting. - It is also especially valuable to gather and document as much information as possible about the issue, - such as a description of the problem being fixed, points of unclarity, potential risks, - alternatives that have been considered, et cetera. - Reviewers should ask PR authors to add this kind of information as comments in the code - and/or to the PR message (which will become part of the git commit history). + Sometimes there is a bug in some code that nobody understands anymore. The + original authors are unavailable and it is hard to gauge the implications of a + proposed fix. In such a case it is a good idea for reviewers to I-nominate the + PR (if they intend to stay the main reviewer) or assign a compiler team lead + to the issue and add the S-waiting-on-team label. In both cases, the PR will + be brought in the weekly triage meeting. It is also especially valuable to + gather and document as much information as possible about the issue, such as a + description of the problem being fixed, points of unclarity, potential risks, + alternatives that have been considered, et cetera. Reviewers should ask PR + authors to add this kind of information as comments in the code and/or to the + PR message (which will become part of the git commit history). [mcp]: https://forge.rust-lang.org/compiler/mcp.html -[whats-a-major-change]: https://forge.rust-lang.org/compiler/mcp.html#what-constitutes-a-major-change +[whats-a-major-change]: + https://forge.rust-lang.org/compiler/mcp.html#what-constitutes-a-major-change ## Technical Aspects of Reviewing -Every PR that lands in the compiler and its associated crates must be -reviewed by at least one person who is knowledgeable with the code in -question. +Every PR that lands in the compiler and its associated crates must be reviewed +by at least one person who is knowledgeable with the code in question. -When a PR is opened, you can request a reviewer by including `r? -@username` in the PR description. If you don't do so, rustbot -will automatically assign someone. +When a PR is opened, you can request a reviewer by including `r? @username` in +the PR description. If you don't do so, rustbot will automatically assign +someone. -It is common to leave a `r? @username` comment at some later point to -request review from someone else. This will also reassign the PR. +It is common to leave a `r? @username` comment at some later point to request +review from someone else. This will also reassign the PR. ### bors -We never merge PRs directly. Instead, we use bors. A qualified -reviewer with bors privileges (e.g., a [compiler -contributor](./membership.md)) will leave a comment like `@bors r+`. -This indicates that they approve the PR. +We never merge PRs directly. Instead, we use bors. A qualified reviewer with +bors privileges (e.g., a [compiler contributor](./membership.md)) will leave a +comment like `@bors r+`. This indicates that they approve the PR. -People with bors privileges may also leave a `@bors r=username` -command. This indicates that the PR was already approved by -`@username`. This is commonly done after rebasing. +People with bors privileges may also leave a `@bors r=username` command. This +indicates that the PR was already approved by `@username`. This is commonly done +after rebasing. -Finally, in some cases, PRs can be "delegated" by writing `@bors -delegate+` or `@bors delegate=username`. This will allow the PR author -to approve the PR by issuing `@bors` commands like the ones above -(but this privilege is limited to the single PR). +Finally, in some cases, PRs can be "delegated" by writing `@bors delegate+` or +`@bors delegate=username`. This will allow the PR author to approve the PR by +issuing `@bors` commands like the ones above (but this privilege is limited to +the single PR). ### Reverts @@ -190,14 +222,15 @@ the best policy is to revert it quickly and re-land it later once a fix and regression test are added. A "meaningful regression" in this case is up to the judgment of the person -approving the revert. As a rule of thumb, this would be a bug in a stable -or otherwise important feature that causes code to stop compiling, changes -runtime behavior, or triggers a (warn-by-default or higher) lint incorrectly in +approving the revert. As a rule of thumb, this would be a bug in a stable or +otherwise important feature that causes code to stop compiling, changes runtime +behavior, or triggers a (warn-by-default or higher) lint incorrectly in real-world code. When these criteria are in doubt, and especially if real-world code is affected, revert the PR. This allows bleeding edge users to continue to use and report -bugs on HEAD with a higher degree of certainty about where new bugs are introduced. +bugs on HEAD with a higher degree of certainty about where new bugs are +introduced. Before being reverted, a PR should be shown to cause a regression with a fairly high degree of certainty (e.g. bisection on commits, or bisection on nightlies @@ -252,10 +285,10 @@ impact on performance, it can be marked `rollup=always`. #### Forward fixes Often it is tempting to address a regression by posting a follow-up PR that, -rather than reverting the regressing PR, instead augments the original in -small ways without reverting its changes overall. However, if real-world users -have reported being affected, this practice is strongly discouraged unless one -of the following is true: +rather than reverting the regressing PR, instead augments the original in small +ways without reverting its changes overall. However, if real-world users have +reported being affected, this practice is strongly discouraged unless one of the +following is true: * A high-confidence fix is already in the bors queue. * The regression has made it to a release branch (beta or stable) and a @@ -273,24 +306,25 @@ reviewers to act quickly and gives you time to address the issue fully. ### Rollups All reviewers are strongly encouraged to explicitly mark a PR as to whether or -not it should be part of a [rollup]. This is usually done either when approving a -PR with `@bors r+ $ROLLUP_STATUS` or with `@bors $ROLLUP_STATUS` where `$ROLLUP_STATUS` -is substituted with one of the following: +not it should be part of a [rollup]. This is usually done either when approving +a PR with `@bors r+ $ROLLUP_STATUS` or with `@bors $ROLLUP_STATUS` where +`$ROLLUP_STATUS` is substituted with one of the following: -- `rollup=always`: These PRs are very unlikely to break tests or have performance - implications. Example scenarios: +- `rollup=always`: These PRs are very unlikely to break tests or have + performance implications. Example scenarios: - Changes are limited to documentation, comments, etc. that is highly unlikely to fail a build. - Changes cannot have performance implications. - Your PR is not landing possibly-breaking or behavior altering changes. - - Feature stabilization without other changes is likely fine to - rollup, though. + - Feature stabilization without other changes is likely fine to rollup, + though. - When in doubt do not use this option! -- `rollup=maybe`: This is the default if `@bors r+` does not specify any rollup - status at all. Use this if you have some doubt that the change won't break - tests. This can be used if you aren't sure if it should be one of the other - categories. Since this is the default, there is usually no need to explicitly - specify this, unless you are un-marking the rollup level from a previous command. +- `rollup=maybe`: This is the default if `@bors r+` does not specify any rollup + status at all. Use this if you have some doubt that the change won't break + tests. This can be used if you aren't sure if it should be one of the other + categories. Since this is the default, there is usually no need to explicitly + specify this, unless you are un-marking the rollup level from a previous + command. - `rollup=iffy`: Use this for mildly risky PRs (more risky than "maybe"). Example scenarios: - The PR is large and non-additive (note: adding 2000 lines of completely @@ -299,14 +333,14 @@ is substituted with one of the following: - LLVM or code generation - bootstrap or the build system - build-manifest - - Has platform-specific changes that are not checked by the normal PR checks. + - Has platform-specific changes that are not checked by the normal PR + checks. - May be affected by MIR migrate mode. - `rollup=never`: This should *never* be included in a rollup (**please** include a comment explaining why you have chosen this). Example scenarios: - May have performance implications. - May cause unclear regressions (we would likely want to bisect to this PR - specifically, as it would be hard to identify as the cause from a - rollup). + specifically, as it would be hard to identify as the cause from a rollup). - Has a high chance of failure. - Is otherwise dangerous to rollup. - `rollup`: this is equivalent to `rollup=always` @@ -314,11 +348,11 @@ is substituted with one of the following: ### Priority -Reviewers are encouraged to set one of the rollup statuses listed above -instead of setting priority. Bors automatically sorts based on the rollup -status (never is the highest priority, always is the lowest), and also by PR -age. If you do change the priority, please use your best judgment to balance -fairness with other PRs. +Reviewers are encouraged to set one of the rollup statuses listed above instead +of setting priority. Bors automatically sorts based on the rollup status (never +is the highest priority, always is the lowest), and also by PR age. If you do +change the priority, please use your best judgment to balance fairness with +other PRs. The following is some guidance for setting priorities: @@ -344,16 +378,15 @@ The following is some guidance for setting priorities: ### Expectations for r+ -bors privileges are binary: the bot doesn't know which code you are -familiar with and what code you are not. They must therefore be used -with discretion. Do not r+ code that you do not know well -- you can -definitely **review** such code, but try to hand off reviewing to -someone else for the final r+. - -Similarly, never issue a `r=username` command unless that person has -done the review, and the code has not changed substantially since the -review was done. Rebasing is fine, but changes in functionality -typically require re-review (though it's a good idea to try and -highlight what has changed, to help the reviewer). +bors privileges are binary: the bot doesn't know which code you are familiar +with and what code you are not. They must therefore be used with discretion. Do +not r+ code that you do not know well -- you can definitely **review** such +code, but try to hand off reviewing to someone else for the final r+. + +Similarly, never issue a `r=username` command unless that person has done the +review, and the code has not changed substantially since the review was done. +Rebasing is fine, but changes in functionality typically require re-review +(though it's a good idea to try and highlight what has changed, to help the +reviewer). [rollup]: ../release/rollups.md From c2133b571f93b1a0c81c57f668c4d668d7669c36 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=AE=B8=E6=9D=B0=E5=8F=8B=20Jieyou=20Xu=20=28Joe=29?= <39484203+jieyouxu@users.noreply.github.com> Date: Tue, 10 Dec 2024 14:54:55 +0800 Subject: [PATCH 7/9] Flesh out and further adjust review policy --- src/compiler/reviews.md | 506 +++++++++++++++++++++++++--------------- 1 file changed, 324 insertions(+), 182 deletions(-) diff --git a/src/compiler/reviews.md b/src/compiler/reviews.md index 85fac42d2..dea1e0b01 100644 --- a/src/compiler/reviews.md +++ b/src/compiler/reviews.md @@ -1,13 +1,15 @@ Review Policy ============= -It is the purpose of code reviews to +It is the purpose of code reviews to: -- **reduce** the risk of introducing **bugs** and usability and performance - **regressions**, -- keep our code **maintainable**, **readable**, and **documented**, and -- ensure that changes are made with the big picture and **appropriate context in - mind**. +- **Reduce** the risk of introducing **bugs** and usability and performance + **regressions**. +- Keep our code **maintainable**: **readable**, **documented** and + **well-tested**. +- Ensure that changes are made with the big picture and **appropriate context in + mind**. This is particularly relevant for changes that seem harmless in + isolation but are problematic or undesirable in the larger context. Reviewing accomplishes this by bringing in another set of eyes to look at a proposed change from a different perspective, which increases the chance of @@ -21,58 +23,86 @@ There are a number of requirements that need to be met in order for reviewing to be effective: - Reviewers must have a sufficient **understanding of the code** under review. - > This is important to help spot non-obvious, unintentional side effects of - > a given change. -- Pull request authors must provide (1) a concise high-level **description of - the change** and (2) the **rationale** behind it. For the rationale to be even - more useful, authors are encouraged to list potential **points of - contention**, **compromises** that needed to be made, **alternative - approaches** that have been considered, et cetera. - > Reviewing code is difficult and reviewers only have a limited amount of - > time to do it. Jump-starting the review process by not making the reviewer - > puzzle together the intention and context of a pull request will not only - > speed things up but also improve the quality of the review. + - This is important to help spot non-obvious, unintentional side effects of a + given change. +- Pull request authors must provide: + 1. A concise high-level **description of the change** and (2) the + **rationale** behind it, unless the change is extremely trivial, like typo + fixes. + 2. For the rationale to be even more useful, authors are encouraged to list + potential **points of contention**, **compromises** that needed to be made, + **alternative approaches** that have been considered, **relevant + documentation, discussions and context**, etc. + - Reviewing code is difficult, and reviewers only have a **limited amount of + time** to do it. Jump-starting the review process by not making the + reviewer puzzle together the intention and context of a pull request will + not only speed things up but also improve the quality of the review. - Reviewers must have a good idea on whether they are the **right person to approve** the change. - > Knowledge of the code under review is an obvious but not the only - > criterium for answering this question. The reviewer also needs to decide - > if they can make the decision alone or if the PR needs to go through the - > [major change process][mcp], if they can perform the review sufficiently - > thorough and in a timely fashion, and if they are impartial enough to - > provide a sufficiently unbiased perspective. - - + - Knowledge of the code under review is an obvious but not the only criteria + for answering this question. + - Procedure wise, the reviewer also needs to decide: + - Can the reviewer make the decision alone? + - Does the PR needs to go through the [Major Change Process][mcp], or does + it need a T-compiler [Final Comment Period][fcp] sign-off, or does it need + a full [Request For Comments (RFC)][rfc]? + - Does the PR need reviews and/or sign-off from other teams, particularly + T-lang? + - Can the changes break stable code or begin accepting new code that we do + not intend to? If the PR contains risks, is it sufficiently justified? + Does the changes need ecosystem impact evaluation through crater runs? + - Will the PR introduce significant perf changes? If there might be a perf + regression, is it justified? Does the PR need a perf run? + - Can the reviewer perform the review sufficiently thorough and in a timely + fashion? + - Is the reviewer impartial enough to provide a sufficiently unbiased + perspective? E.g. due to co-authorship (sufficiently significant changes + to the PR made by the reviewer) or other conflicts-of-interest? + +[rfc]: https://github.com/rust-lang/rfcs ## Reviewing Checklist -The following list of questions (to be posted as part rustbot's automatic PR -response) will help reviewers and PR authors alike to bring PRs into good shape -and meet the above criteria: +The following list of questions will help reviewers and PR authors alike to +bring PRs into good shape and meet the above criteria: > #### Checklist for PR authors and reviewers > * Does the PR message have -> * a concise **high-level description** of the changes? (*what* is being +> * A concise **high-level description** of the changes? (*what* is being > changed) -> * a clear **rationale** for doing them? (*why* is it being changed) -> * a list of potential **points of contention**? -> * **links to relevant issues**, RFCs, MCPs, etc? -> * Does the PR need a **regression test**? +> * A clear **rationale** for doing them? (*why* is it being changed) +> * If non-trivial and if suitable, **how** the bug is fixed or **how** the +> change is implemented? +> * A list of potential **points of contention**? Alternatives? Trade-offs? +> Risks? +> * **Links to relevant issues**, RFCs, MCPs, etc? +> * Does the PR need a **regression test**? Does the PR have **sufficient test +> coverage**? > * Does the change need to be covered by a **[major change proposal][mcp]**? Is -> it already covered? +> it already covered? If there is already a MCP open, was it already accepted, +> or is the PR blocked on that? +> * Does the PR need a **perf run**? +> * Does the PR need reviews and/or sign-offs from **other teams**? E.g. T-lang +> for lint expansions if the ecosystem impact is large or language changes. +> * Does the PR affect other teams in a non-trivial way? Should the affected +> teams get a heads-up? E.g. changes to rustfmt or rust-analyzer or subtrees. > * Would someone trying to understand the PR in a year's time be able to > quickly **reconstruct what's going on**? > * Is the new code **properly documented**? Is existing documentation still > up-to-date? +> * Does the changes in the PR need updates to the Reference or Edition Guide? > * Does the PR introduce a **regression** of any of the following: -> * error message quality -> * maintainability (e.g. complex code, no documentation, unsafe) -> * any specific target platforms -> * downstream tooling (e.g. linkers, debuggers) -> * compile times -> * memory usage +> * Error message quality +> * Maintainability (e.g. complex code, no documentation, unsafe) +> * Any specific target platforms +> * Downstream tooling (e.g. linkers, debuggers) +> * Compile times +> * Memory usage +> * Targets (e.g. baselines, target features, calling conventions, etc.) > > #### Checklist for reviewers > * Am I the **right person to review** this PR: +> * Does the changes in this PR fall under T-compiler purview? > * Do I understand the code well enough? > * Would I be able to spot non-obvious side effects? > * Would I be able to fix a bug introduced by this PR? @@ -80,9 +110,11 @@ and meet the above criteria: > * Do I feel pressure to quickly approve the PR for some reason? > * Am I impartial enough? > * Before merging: -> * Is the **PR message still accurate**? -> * Is the **commit history clean** enough? - +> * Is the **PR title and description still accurate**? +> * Is the **commit history clean** enough? We do not need 16 "fix typo" +> commits in the PR history. +> * Does the PR correctly/incorrectly close relevant issues? +> * Do I need to roll reviewers from other relevant teams? ## Guidance for dealing with common situations @@ -92,96 +124,141 @@ However, sometimes there are gray areas where it is not immediately clear how to proceed. This section lists a few common cases together with guidance on how to deal with them. -* ### I don't think I am a good fit for reviewing - what now? - It is completely normal that you get assigned a PR (via rustbot or otherwise) - but don't feel comfortable reviewing it. Here is what you can do, depending on - the concrete case: - - - If the change seems really big or contentious, consider asking for an MCP - (see below). - - If you know just the right person for the review, assign them via `r? - @`. It's polite to leave a comment asking them if they can take - over -- but you don't have to make sure beforehand that they can actually do - it. - - If you don't know the code well or already have too much on your plate, ask - rustbot to roll the dice again via `r? compiler`. - - You can also always ask for help on the `#t-compiler` Zulip stream for finding - a reviewer. That being said, you are always welcome to do an initial review - (to the extent you are comformtable with) and then pass the PR on to the final - reviewer. This way the PR author will get helpful feedback sooner and - subsequent reviewers will have less work to do. - -* ### It is unclear if something constitutes a major change - Deciding if something is a "major change" requiring an [MCP][mcp] is not - always straightforward. The official guidelines are - [here][whats-a-major-change]. When in doubt, err on side of treating something - as a major change. You can also nominate the PR for discussion in the compiler - team's triage meeting by tagging it `I-nominated`. If you nominate a PR please - make sure to state a concrete question for the compiler team to discuss. - -* ### Discussion or rationale is too intransparent - Sometimes there are PRs that seem to be the result of some prior discussion, - with no description or rationale. They usually have a title like "Change X" - and the only content of the PR message is "r? @xyz". Even though the change - might make sense and may even have been suggested by a compiler team member - this is not good form. The PR message should give a self-contained description - of what is being changed, why it is being changed and anything else that might - be of interest. Try to put yourself in the shoes of someone who, a few years - down the road, needs to fix a bug related to the code touched by the PR and - needs to reconstruct the rationale for the way things are. - -* ### Reviewer and PR author report to the same entity / work for the same employer - There is no rule that prevents two employees of the same company from - reviewing each other's PRs. The concerns in such a case are no different than - for any other two reviewers. We expect the mechanisms and principles we - articulated above to be respected by ALL reviewers, whatever their employer. - Does the PR concisely describe the changes that are being made? Does it give a - clear, transparent rationale for why the changes make sense so that - contributors down the line can follow the reasoning and reconstruct what's - going on? Have points of contention been discussed and cleared up? Then you - are in the clear. - - If you are in doubt if something is contentious, give a heads up to - `@rust-lang/compiler-contributors` and ask for another opinion. If the - proposed change is large and/or potentially has a big impact, create a [major - change proposal][mcp]. - -* ### Reviewing and Mentoring - In the course of mentoring someone through a PR it often happens that the - reviewer has ended up effectively co-writing the changes. This can be a tricky - case because the reviewer is effectively approving their own changes. There - are a number of considerations to take into account when deciding how to - proceed: - - - If the general direction of the changes has already been approved as part of - an MCP and the concrete advice given during mentoring was only concerned - with resolving minor technical issues, then no further review is required. - - Similarly, if any contentious decisions have visibly been discussed and - resolved on the PR with other compiler team contributors and the rest of the - changes don't deviate from the general direction that has been agreed upon - then no further review is required either. - - If the PR was opened as a response to a concrete suggestion by the reviewer - (and the changes are not entirely trivial) then it is advisable that the - final review is done by someone else. However, the initial reviewer/mentor - is encouraged to help bring the PR into good shape before handing it off. - - In general, it is advisable to ask for a second opinion by someone - knowledgable in the field in such cases, just to increase the chance of - uncovering oversights and blindspots a mentor might have. - -* ### Nobody understands the code that's being changed - Sometimes there is a bug in some code that nobody understands anymore. The - original authors are unavailable and it is hard to gauge the implications of a - proposed fix. In such a case it is a good idea for reviewers to I-nominate the - PR (if they intend to stay the main reviewer) or assign a compiler team lead - to the issue and add the S-waiting-on-team label. In both cases, the PR will - be brought in the weekly triage meeting. It is also especially valuable to - gather and document as much information as possible about the issue, such as a - description of the problem being fixed, points of unclarity, potential risks, - alternatives that have been considered, et cetera. Reviewers should ask PR - authors to add this kind of information as comments in the code and/or to the - PR message (which will become part of the git commit history). +### I don't think I am a good fit for reviewing - what now? + +It is completely normal that you get (randomly) assigned a PR (via rustbot or +otherwise) but don't feel comfortable reviewing it. Here is what you can do, +depending on the concrete case: + +- If the change seems really big or contentious, consider asking for an MCP (see + below). The reviewer does not and should not be expected to just stomach a + large and/or significant PR. +- If you know just the right person for the review, assign them via `r? + @`. It's polite to leave a comment asking them if they can take + over -- but you don't have to make sure beforehand that they can actually do + it. +- If the change is not too complicated and you don't expect that another + randomly rolled compiler reviewer will also have trouble with the PR, you can + reroll a random compiler reviewer with `r? compiler`. +- If the change is complicated, or you expect randomly rolling another compiler + reviewer will just lead to multiple rerolls, you should open a thread in + `#t-compiler/private` to ask the rest of the team -- for someone who might be + able to review it, or even if the team is comfortable with accepting the + change at all. + +You can also always ask for help on the `#t-compiler` Zulip stream for finding a +reviewer. That being said, you are always welcome to do an initial review (to +the extent you are comfortable with) and then pass the PR on to the final +reviewer. This way the PR author will get helpful feedback sooner, subsequent +reviewers will have less work to do, and you might also improve your own +understanding of diverse areas in the compiler. + +### It is unclear if something constitutes a major change + +Deciding if something is a "major change" requiring an [MCP][mcp] is not always +straightforward. The official guidelines are [here][whats-a-major-change]. When +in doubt, err on side of treating something as a major change. You can also +nominate the PR for discussion in the compiler team's triage meeting by tagging +it `I-compiler-nominated`. If you nominate a PR please make sure to state a +**concrete question** for the compiler team to discuss, include **useful +context** if suitable. + +> **Example triage meeting nomination message** +> +> ```text +> Nominating for T-compiler triage meeting to determine if we want to make +> `-Z unstable-options` also accept value(s), e.g. `-Z unstable-options=values` to +> guard *unstable values* of *stable flags* like `-C opt-level=4`. +> +> Relevant discussion: . +> See also #123456. +> +> @rustbot label +I-compiler-nominated +> ``` + +### Discussion or rationale is too intransparent + +Sometimes there are PRs that seem to be the result of some prior discussion, +with no description or rationale. They usually have a title like "Change X" and +the only content of the PR message is "r? @xyz". Even though the change might +make sense and may even have been suggested by a compiler team member, this is +not good form. + +Contributors may stumble across the PR several year later during bisections only +to find the PR with absolutely zero context that was discussed offline or +elsewhere, and the information is not available to future contributors. This is +not good for maintainability. Including relevant context will very often help +the PR author themselves in the future! + +The PR message should give a self-contained description of what is being +changed, why it is being changed and anything else that might be of interest. + +**Try to put yourself in the shoes of someone who, a few years down the road, +needs to fix a bug related to the code touched by the PR and needs to +reconstruct the rationale for the way things are.** + +### Reviewer and PR author report to the same entity / work for the same employer + +There is no rule that prevents two employees of the same company from reviewing +each other's PRs. We assume compiler team reviewers to act in good faith, and +vest trust in team members to do so. + +The concerns in such a case are no different than for any other two reviewers. +We expect the mechanisms and principles we articulated above to be respected by +ALL reviewers, whatever their employer. Does the PR concisely describe the +changes that are being made? Does it give a clear, transparent rationale for why +the changes make sense so that contributors down the line can follow the +reasoning and reconstruct what's going on? Have points of contention been +discussed and cleared up? Then you are in the clear. + +If you are in doubt if something is contentious, give a heads up to +`@rust-lang/compiler` and ask for another opinion. If the proposed change is +large and/or potentially has a big impact, you can discuss in a `#t-compiler` +zulip topic, and/or create a [Major Change Proposal][mcp]. + +### Reviewing and Mentoring + +In the course of mentoring someone through a PR it often happens that the +reviewer has ended up effectively co-writing the changes. This can be a tricky +case because the reviewer is effectively approving their own changes. There are +a number of considerations to take into account when deciding how to proceed: + +- If the general direction of the changes has already been approved as part of + an MCP and the concrete advice given during mentoring was only concerned with + resolving minor technical issues, then no further review is required. +- Similarly, if any contentious decisions have visibly been discussed and + resolved on the PR with other compiler team members and the rest of the + changes don't deviate from the general direction that has been agreed upon + then no further review is required either. +- If the PR was opened as a response to a concrete suggestion by the reviewer + (and the changes are not entirely trivial) then it is advisable that the final + review is done by someone else. However, the initial reviewer/mentor is + encouraged to help bring the PR into good shape before handing it off. + +In general, it is advisable to ask for a second opinion by someone knowledgable +in the field in such cases, just to increase the chance of uncovering oversights +and blindspots a mentor might have. + +### Nobody understands the code that's being changed + +Sometimes there is a bug in some code that nobody understands anymore. The +original authors are unavailable and it is hard to gauge the implications of a +proposed fix. In such a case it is a good idea for reviewers to +`I-compiler-nominate` the PR (if they intend to stay the main reviewer) or +assign a compiler team lead to the issue and add the `S-waiting-on-team` label. + +In both cases, the PR will be brought in the weekly triage meeting. It is also +especially valuable to gather and document as much information as possible about +the issue, such as a description of the problem being fixed, points of +unclarity, potential risks, alternatives that have been considered, et cetera. +It is also a good idea to open a tracking issue to document the lack of +understanding of such area, to document the specific questions, concerns and +bugs, and it can be resolved if compiler team members regain better +understanding. + +Reviewers should ask PR authors to add this kind of information as comments in +the code and/or to the PR message (which will become part of the git commit +history). [mcp]: https://forge.rust-lang.org/compiler/mcp.html [whats-a-major-change]: @@ -195,15 +272,25 @@ by at least one person who is knowledgeable with the code in question. When a PR is opened, you can request a reviewer by including `r? @username` in the PR description. If you don't do so, rustbot will automatically assign -someone. +someone from the pool of reviewer candidates, determined by the files affected. It is common to leave a `r? @username` comment at some later point to request review from someone else. This will also reassign the PR. +It is possible to request reviews from multiple reviewers, for example + +```text +Rolling both a T-compiler and T-bootstrap reviewer as this PR contains both +compiler and bootstrap changes. + +r? compiler +r? bootstrap +``` + ### bors We never merge PRs directly. Instead, we use bors. A qualified reviewer with -bors privileges (e.g., a [compiler contributor](./membership.md)) will leave a +bors privileges (e.g., a [compiler team member](./membership.md)) will leave a comment like `@bors r+`. This indicates that they approve the PR. People with bors privileges may also leave a `@bors r=username` command. This @@ -211,9 +298,9 @@ indicates that the PR was already approved by `@username`. This is commonly done after rebasing. Finally, in some cases, PRs can be "delegated" by writing `@bors delegate+` or -`@bors delegate=username`. This will allow the PR author to approve the PR by -issuing `@bors` commands like the ones above (but this privilege is limited to -the single PR). +`@bors delegate=username`. This will allow the PR author or the delegated user +to approve the PR by issuing `@bors` commands like the ones above (but this +privilege is limited to the single PR). ### Reverts @@ -222,15 +309,28 @@ the best policy is to revert it quickly and re-land it later once a fix and regression test are added. A "meaningful regression" in this case is up to the judgment of the person -approving the revert. As a rule of thumb, this would be a bug in a stable or -otherwise important feature that causes code to stop compiling, changes runtime -behavior, or triggers a (warn-by-default or higher) lint incorrectly in -real-world code. +approving the revert. + +Some criteria to consider if a revert is warranted: + +- A bug in a stable or otherwise important feature that causes code to stop + compiling, changes runtime behavior, or triggers a (warn-by-default or higher) + lint incorrectly in real-world code. Especially if the bug is reachable + without any unstable feature gates. +- If a bug or change (incl. ICEs) is particularly easy to hit. +- If a bug or change significantly degrades contributor experience. +- If a test is flaky and unreliable. When these criteria are in doubt, and especially if real-world code is affected, -revert the PR. This allows bleeding edge users to continue to use and report -bugs on HEAD with a higher degree of certainty about where new bugs are -introduced. +revert the PR. This has three benefits: + +1. It allows bleeding edge users (esp. nightly or beta) to continue to use and + report bugs on HEAD with a higher degree of certainty about where new bugs are + introduced. +2. It takes pressure off the original PR author and the team, that no one is + pressured to feel like they have to fix it *immediately*. +3. It might prevent the significant bug or regression from reaching another + nightly/beta/stable build. Before being reverted, a PR should be shown to cause a regression with a fairly high degree of certainty (e.g. bisection on commits, or bisection on nightlies @@ -253,10 +353,34 @@ uploaded as a pull request: $ git revert -m 1 62d5bee ``` +Don't rely *only* on the default commit title and message created by git. +Instead, title the revert commit meaningfully, and link to the relevant PR that +introduced the regression. Link to the specific PR that is being fully or +partially reverted. Link to relevant issues and discussions. Retain the commit +hash being reverted. + +> **Example revert commit title and message** +> +> ```text +> Revert #131669 due to ICEs +> +> Revert due to ICE +> reports: +> +> - (real-world) +> - (fuzzing) +> +> The changes can be re-landed with those cases addressed. +> +> This reverts commit 703bb982303ecab02fec593899639b4c3faecddd, reversing +> changes made to f415c07494b98e4559e4b13a9c5f867b0e6b2444. +> ``` + It's polite to tag the author and reviewer of the original PR so they know -what's going on. You can use the following message template: +what's going on. You can use the following message template for the revert PR +description: -``` +```text Reverts rust-lang/rust#123456 cc @author @reviewer @@ -276,11 +400,23 @@ use cases working, and keep the compiler more stable for everyone. r? compiler ``` -If you have r+ privileges, you can self-approve a revert. +Please include a temporary regression test in a separate commit to check that +the regression is actually addressed by the revert commit. In a reland, this +temporary regression test can be adapted or removed following improved test +coverage as suitable. + +If you have r+ privileges, you can self-approve a revert **if** the revert is +clean and is unlikely to cause new regressions on its own, make sure the revert +is not a case of "the cure is worse than the poison". If non-trivial, please +wait for a review -- from the original reviewer or from another compiler +reviewer via `r? compiler`. You can ask in `#t-compiler` if the matter is more +urgent. Generally speaking, reverts should have elevated priority and match the `rollup` status of the PR they are reverting. If a non-rollup PR is shown to have no -impact on performance, it can be marked `rollup=always`. +impact on performance, it can be marked `rollup=always`. The revert author can +coordinate with contributors authoring rollups to reschedule rollups or +interleave the revert PR between rollups if suitable. #### Forward fixes @@ -293,15 +429,17 @@ following is true: * A high-confidence fix is already in the bors queue. * The regression has made it to a release branch (beta or stable) and a [backport] is needed. Often the "smallest possible change" is desired for a - backport. The offending PR may or may not still be reverted on the main - branch; this is left to the discretion of someone who can `r+` it. + backport (so that the fix doesn't introduce *new* regressions). The offending + PR may or may not still be reverted on the main branch; this is left to the + discretion of someone who can `r+` it. [backport]: ../release/backporting.md While it can feel like a significant step backward to have your PR reverted, in -most cases it is much easier to land the PR a second time once a fix can be -confirmed. Allowing a revert to land takes pressure off of you and your -reviewers to act quickly and gives you time to address the issue fully. +most cases it is much easier to reland the PR once a fix can be confirmed. +Allowing a revert to land takes pressure off of you and your reviewers to act +quickly and gives you time to address the issue fully. It also is an opportunity +to take a step back and reassess the test coverage. ### Rollups @@ -312,13 +450,13 @@ a PR with `@bors r+ $ROLLUP_STATUS` or with `@bors $ROLLUP_STATUS` where - `rollup=always`: These PRs are very unlikely to break tests or have performance implications. Example scenarios: - - Changes are limited to documentation, comments, etc. that is highly - unlikely to fail a build. - - Changes cannot have performance implications. - - Your PR is not landing possibly-breaking or behavior altering changes. - - Feature stabilization without other changes is likely fine to rollup, - though. - - When in doubt do not use this option! + - Changes are limited to documentation, comments, etc. that is highly + unlikely to fail a build. + - Changes cannot have performance implications. + - Your PR is not landing possibly-breaking or behavior altering changes. + - Feature stabilization without other changes is likely fine to rollup, + though. + - When in doubt do not use this option! - `rollup=maybe`: This is the default if `@bors r+` does not specify any rollup status at all. Use this if you have some doubt that the change won't break tests. This can be used if you aren't sure if it should be one of the other @@ -327,22 +465,22 @@ a PR with `@bors r+ $ROLLUP_STATUS` or with `@bors $ROLLUP_STATUS` where command. - `rollup=iffy`: Use this for mildly risky PRs (more risky than "maybe"). Example scenarios: - - The PR is large and non-additive (note: adding 2000 lines of completely - new tests is fine to rollup). - - Messes too much with: - - LLVM or code generation - - bootstrap or the build system - - build-manifest - - Has platform-specific changes that are not checked by the normal PR - checks. - - May be affected by MIR migrate mode. + - The PR is large and non-additive (note: adding 2000 lines of completely + new tests is fine to rollup). + - Has platform-specific changes that are not checked by the normal PR + checks. + - May be affected by MIR migrate mode. - `rollup=never`: This should *never* be included in a rollup (**please** include a comment explaining why you have chosen this). Example scenarios: - - May have performance implications. - - May cause unclear regressions (we would likely want to bisect to this PR - specifically, as it would be hard to identify as the cause from a rollup). - - Has a high chance of failure. - - Is otherwise dangerous to rollup. + - May have performance implications. + - May cause unclear regressions (we would likely want to bisect to this PR + specifically, as it would be hard to identify as the cause from a rollup). + - Has a high chance of failure. + - Is otherwise dangerous to rollup. + - Messes too much with: + - LLVM or code generation + - bootstrap or the build system + - build-manifest - `rollup`: this is equivalent to `rollup=always` - `rollup-`: this is equivalent to `rollup=maybe` @@ -351,8 +489,8 @@ a PR with `@bors r+ $ROLLUP_STATUS` or with `@bors $ROLLUP_STATUS` where Reviewers are encouraged to set one of the rollup statuses listed above instead of setting priority. Bors automatically sorts based on the rollup status (never is the highest priority, always is the lowest), and also by PR age. If you do -change the priority, please use your best judgment to balance fairness with -other PRs. +change the priority, please use your best judgment to balance fairness and +urgency with other PRs. The following is some guidance for setting priorities: @@ -366,7 +504,7 @@ The following is some guidance for setting priorities: - P-critical issue fixes - 10+ - Bitrot-prone PRs (particularly very large ones that touch many files) - - Urgent PRs + - Urgent PRs (e.g. urgent reverts) - Beta backports - 20+ - High priority that needs to jump ahead of any rollups @@ -384,9 +522,13 @@ not r+ code that you do not know well -- you can definitely **review** such code, but try to hand off reviewing to someone else for the final r+. Similarly, never issue a `r=username` command unless that person has done the -review, and the code has not changed substantially since the review was done. -Rebasing is fine, but changes in functionality typically require re-review -(though it's a good idea to try and highlight what has changed, to help the -reviewer). +review, and the code has not changed substantially since the review was done, +and that the person has explicitly indicated that another contributor can `r=` +on their behalf. + +Rebasing is fine and often necessary, but changes in functionality typically +require re-review. It is very helpful for the reviewer if the PR author can +produce a brief summary of what has changed since last review, in addition to +responding to individual review comments. [rollup]: ../release/rollups.md From d77e85bdd524c47552e59bb8d5510c1ed1785afe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=AE=B8=E6=9D=B0=E5=8F=8B=20Jieyou=20Xu=20=28Joe=29?= <39484203+jieyouxu@users.noreply.github.com> Date: Tue, 10 Dec 2024 15:13:21 +0800 Subject: [PATCH 8/9] Clarify concerns on API for external consumers and large PRs --- src/compiler/reviews.md | 61 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/src/compiler/reviews.md b/src/compiler/reviews.md index dea1e0b01..bd5b8dcf1 100644 --- a/src/compiler/reviews.md +++ b/src/compiler/reviews.md @@ -145,6 +145,8 @@ depending on the concrete case: `#t-compiler/private` to ask the rest of the team -- for someone who might be able to review it, or even if the team is comfortable with accepting the change at all. +- If the change is intended for another team, roll a reviewer from the relevant + team. You can also always ask for help on the `#t-compiler` Zulip stream for finding a reviewer. That being said, you are always welcome to do an initial review (to @@ -260,6 +262,48 @@ Reviewers should ask PR authors to add this kind of information as comments in the code and/or to the PR message (which will become part of the git commit history). +### PR makes a change to support use of rustc internals for external projects + +This will need to be determined on a case-by-case basis. + +In general, we should allow changes making things public, cleaning up things or +making them more general, as long as the owners of the compiler region agree (so +just assign to them). + +As a concrete example: if someone is using the mir interpreter, and they want to +make something public, it is likely not a problem, but there are some functions +that are module- or crate-private on purpose, as they uphold invariants within +the mir interpreter. So basically, just assign such PRs to the relevant people +(usually they get pinged anyway due to having told highfive that they want to +get pinged on changes to these parts). + +Require a doc comment on such APIs identifying which external consumers the API +concerns, and for what kinds of purpose. + +If this is possibly contentious, ask for an [mcp]. + +Note that this can non-obviously bound supposedly-internal compiler APIs to +external consumers. Convey to the external consumers (that are not `rust-lang/` +projects) that we can offer the convenience so as long as it does not impose +significant maintenance burden on the compiler, e.g. gets in the way of +refactorings, and no hard stability guarantees are promised. + +### The PR is very large and complicated + +Reviewers are **not** expected to stomach PRs that are very large and +complicated. Bring the PR to the attention of the team (through zulip threads +and/or nominate for compiler triage meeting), and the team can decide if: + +- The team can find suitable reviewers who can aid the PR author to break up the + large change into smaller logical PRs that are possible to review on their + own, but also in the context of the larger change. +- The team does not have the bandwidth, or team members are is not ready or + willing or able to accept the large change as-is. In such cases, the team + should make a decision to postpone or close, and clearly communicate the + decision to the PR author to explain the reasoning. It is very frustrating if + a PR stalls for many months only for it to be rejected anyway. + + [mcp]: https://forge.rust-lang.org/compiler/mcp.html [whats-a-major-change]: https://forge.rust-lang.org/compiler/mcp.html#what-constitutes-a-major-change @@ -531,4 +575,21 @@ require re-review. It is very helpful for the reviewer if the PR author can produce a brief summary of what has changed since last review, in addition to responding to individual review comments. +## Social aspects of reviewing + +First and foremost, PR authors and compiler reviews alike are expected to uphold +the [Code of Conduct][coc]. Simply speaking, a reviewer is expected to be +respectful to the PR author, even if the reviewer disagrees with the changes. + +Reviewers are encouraged to consider matters from the perspectives of the PR +author too. If a change is stuck due to procedural reasons or reviewer bandwidth +for months without any resolution (including a resolution that the compiler +might not be ready to accept such a change at present time, but thank the PR +author for the contributions anyway), and accrues constant merge conflicts, it +can be very frustrating. + +If some discussions are getting heated, ask the [moderation +team](https://www.rust-lang.org/governance/teams/moderation) to step in. + +[coc]: https://www.rust-lang.org/policies/code-of-conduct [rollup]: ../release/rollups.md From 0ded92bd43e744f3c0786ef5538f13a910d10300 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=AE=B8=E6=9D=B0=E5=8F=8B=20Jieyou=20Xu=20=28Joe=29?= <39484203+jieyouxu@users.noreply.github.com> Date: Tue, 10 Dec 2024 17:05:50 +0800 Subject: [PATCH 9/9] Link to bors docs --- src/compiler/reviews.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/compiler/reviews.md b/src/compiler/reviews.md index bd5b8dcf1..69a45b669 100644 --- a/src/compiler/reviews.md +++ b/src/compiler/reviews.md @@ -575,6 +575,8 @@ require re-review. It is very helpful for the reviewer if the PR author can produce a brief summary of what has changed since last review, in addition to responding to individual review comments. +Please refer to [bors documentation for bot usage](../infra/docs/bors.md). + ## Social aspects of reviewing First and foremost, PR authors and compiler reviews alike are expected to uphold