Skip to content

Commit d77e85b

Browse files
committed
Clarify concerns on API for external consumers and large PRs
1 parent c2133b5 commit d77e85b

File tree

1 file changed

+61
-0
lines changed

1 file changed

+61
-0
lines changed

src/compiler/reviews.md

+61
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,8 @@ depending on the concrete case:
145145
`#t-compiler/private` to ask the rest of the team -- for someone who might be
146146
able to review it, or even if the team is comfortable with accepting the
147147
change at all.
148+
- If the change is intended for another team, roll a reviewer from the relevant
149+
team.
148150

149151
You can also always ask for help on the `#t-compiler` Zulip stream for finding a
150152
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
260262
the code and/or to the PR message (which will become part of the git commit
261263
history).
262264
265+
### PR makes a change to support use of rustc internals for external projects
266+
267+
This will need to be determined on a case-by-case basis.
268+
269+
In general, we should allow changes making things public, cleaning up things or
270+
making them more general, as long as the owners of the compiler region agree (so
271+
just assign to them).
272+
273+
As a concrete example: if someone is using the mir interpreter, and they want to
274+
make something public, it is likely not a problem, but there are some functions
275+
that are module- or crate-private on purpose, as they uphold invariants within
276+
the mir interpreter. So basically, just assign such PRs to the relevant people
277+
(usually they get pinged anyway due to having told highfive that they want to
278+
get pinged on changes to these parts).
279+
280+
Require a doc comment on such APIs identifying which external consumers the API
281+
concerns, and for what kinds of purpose.
282+
283+
If this is possibly contentious, ask for an [mcp].
284+
285+
Note that this can non-obviously bound supposedly-internal compiler APIs to
286+
external consumers. Convey to the external consumers (that are not `rust-lang/`
287+
projects) that we can offer the convenience so as long as it does not impose
288+
significant maintenance burden on the compiler, e.g. gets in the way of
289+
refactorings, and no hard stability guarantees are promised.
290+
291+
### The PR is very large and complicated
292+
293+
Reviewers are **not** expected to stomach PRs that are very large and
294+
complicated. Bring the PR to the attention of the team (through zulip threads
295+
and/or nominate for compiler triage meeting), and the team can decide if:
296+
297+
- The team can find suitable reviewers who can aid the PR author to break up the
298+
large change into smaller logical PRs that are possible to review on their
299+
own, but also in the context of the larger change.
300+
- The team does not have the bandwidth, or team members are is not ready or
301+
willing or able to accept the large change as-is. In such cases, the team
302+
should make a decision to postpone or close, and clearly communicate the
303+
decision to the PR author to explain the reasoning. It is very frustrating if
304+
a PR stalls for many months only for it to be rejected anyway.
305+
306+
263307
[mcp]: https://forge.rust-lang.org/compiler/mcp.html
264308
[whats-a-major-change]:
265309
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
531575
produce a brief summary of what has changed since last review, in addition to
532576
responding to individual review comments.
533577

578+
## Social aspects of reviewing
579+
580+
First and foremost, PR authors and compiler reviews alike are expected to uphold
581+
the [Code of Conduct][coc]. Simply speaking, a reviewer is expected to be
582+
respectful to the PR author, even if the reviewer disagrees with the changes.
583+
584+
Reviewers are encouraged to consider matters from the perspectives of the PR
585+
author too. If a change is stuck due to procedural reasons or reviewer bandwidth
586+
for months without any resolution (including a resolution that the compiler
587+
might not be ready to accept such a change at present time, but thank the PR
588+
author for the contributions anyway), and accrues constant merge conflicts, it
589+
can be very frustrating.
590+
591+
If some discussions are getting heated, ask the [moderation
592+
team](https://www.rust-lang.org/governance/teams/moderation) to step in.
593+
594+
[coc]: https://www.rust-lang.org/policies/code-of-conduct
534595
[rollup]: ../release/rollups.md

0 commit comments

Comments
 (0)