Update review policy

[michaelwoerister]

Discussing this was proposed in: rust-lang/compiler-team#444. The actual discussion took place on zulip.

Rendered

@rust-lang/compiler: Please leave comments about anything that you'd still like to have discussed.

[michaelwoerister]

I think it'd be good to add some section on "not assigned reviews" and best practices there -- both in terms of how to indicate you aren't the final reviewer to the author, but also whether e.g. it's acceptable to r- someone's PR asking them to cleanup commit history / fix the description / etc.

@Mark-Simulacrum, where would you like this to be added? Do you want to draft the concrete text?

[michaelwoerister]

I voted :writing: on this because I feel like if you're not feeling comfotable with reviewing a PR, it is unlikely you'll be able to accurately gauge the scope of the PR and/or its contentiousness.

@nagisa, do you want to elaborate on that?

[LeSeulArtichaut]

(rust-lang/highfive#350 has been merged)

[davidtwco]

Little late to the discussion here, updated review policy looks good to me, but wanted to suggest another edge case that we should document in our review policy: a pull request which makes changes to support the use of rustc internals for some external project - these aren't common but I've seen it happen a few times - do we accept these changes if they're small and otherwise improvements, do we reject these unless it's a mature and released project, etc?

[oli-obk]

I think 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)

[ehuss]

@michaelwoerister I know it's been a while, are you still interested in working on this? I'm not clear if this was waiting for more updates or reviews, or if it is ready to go as-is.

[michaelwoerister]

Oh my, that was never merged, was it? Sorry for not following up!

I'll look into getting this over the finish line.

[michaelwoerister]

@wesleywiser & @davidtwco, I think the outstanding questions have been addressed (see the final two commits). I don't think there are any major changes compared to when the document was discussed originally, so I think we could merge the PR in the current state.

I did not add the point @davidtwco made in #553 (comment). I'd be happy to if I can get some help with what the actual text should look like.

[jieyouxu]

I tried to address @davidtwco's concern about changing compiler-internal API visibility for external consumers. I apologize for the reformatting reflow, it was hard for me to read in order to edit.

I also fleshed out the review policy with some more specifics, so this will need a re-review.

There's quite a lot of duplication that I'm not a huge fan of, but this is mostly an initial draft. There's also some overlap between here and other parts of forge docs and rustc-dev-guide's contributor guidelines. I think those could be follow-up cleanups. It's also very verbose which is not good for actually reading and following the guidelines. It likely needs to be torn down and reorganized, probably in a follow-up.

cc @rust-lang/compiler because this has stalled since last year, and now has significant changes. cc compiler leads @wesleywiser and @davidtwco for approving reviews specifically.

Significant changes since approval

[davidtwco]

LGTM after @SparrowLii's comments addressed

[jieyouxu]

Going to wait a bit in case there are anymore immediate feedback, otherwise merge at your discretion.

[oli-obk]

I'd say we land it and edit it in place in follow-up PRs if more people comment