Discuss extending the compiler team’s review policy

[michaelwoerister]

Meeting proposal info

• Title: Discuss extending the compiler team’s review policy
• Type: non-technical

Summary

The compiler team has a review policy but it is very sparse and focuses mostly on technicalities like how to use the Bors bot. I’m proposing to make this policy more concrete by more explicitly listing expectations and giving guidelines on what to do in non-obvious cases.

Motivation

It seems like a good idea to have an explicit policy for dealing with a central aspect of an open source project like reviewing code:

• It makes it easier for newcomers to know what's expected of a PR.
• It helps reviewers and PR authors to not forget common things like having regression tests.
• It makes it easier for reviewers, old hands and newbies alike, to ask PR authors for quality improvements, which ultimately benefits everyone working in the project.
• It provides a concrete basis for discussing useful changes to the review policy and how reviewing is done in practice.

Overall, I think, having a concrete review policy with actionable suggestions and decision making guidance is an important manifestation of putting the compiler team's (proposed) Contributor Guiding Principles into practice.

Details

Here is a draft for a more explicit review policy:

https://hackmd.io/@michaelwoerister/SyP6xP2nd

This is meant to be the basis for discussion, not the final outcome :)

Challenges

Having a written policy like this is always a balancing act between being too vague and unactionable versus being too restrictive and adding too much friction to the contribution process -- and thus running the risk of just being ignored. But having a policy still seems to be strictly better than having no policy and everyone just working off their private assumptions with no basis for discussing a common ground. I fully expect this policy to change over time and be adapted and improved as the need arises and we collect more experience with it.

Key design questions

• How can we make this really useful in practice?

About this issue

This issue corresponds to a meeting proposal for the compiler team
steering meeting. It corresponds to a possible topic of
discussion. You can read more about the steering meeting procedure
here.

Comment policy

These issues are meant to be used as an "announcements channel"
regarding the proposal, and not as a place to discuss the technical
details. Feel free to subscribe to updates. We'll post comments when
reviewing the proposal in meetings or making a scheduling decision.
In the meantime, if you have questions or ideas, ping the proposers
on Zulip (or elsewhere).

[jackh726]

So, there's one thing that I'd like to see some discussion about: how to reassign to another reviewer when you don't feel comfortable reviewing a bit of code, for whatever reason.

Currently, if you can't/don't want to review a PR for whatever reason, you have a couple options.

First, if you think it's suitable for anyone to review, or don't have a general idea of who/what set of people might be best to review, you can select pseudo-randomly someone. Should this be from the highfive queue? Or from the teams as a whole. There's been some discussion of adding this functionality to highfive directly, which would be good. But, this still suffers from the problem that a PR may be focused on one particular area that needs a more specific review. Which brings me to...

Second, if you have some idea of a better candidate to review, then you can reassign directly. But this relies on knowing that. Of course, there is the expert map, but that hasn't been updated anytime recently and certainly isn't complete. It also doesn't take into account people being currently absent or unable to review.

Third, you can advertise by either tagging the teams or posting on Zulip to get someone to voluntarily take on the review. In the case of the former, I always feel its a bit heavy handed since it ends up subscribing everyone to that PR. In the case of the latter, it's not always clear the best place to actually post that and can easily be missed.

All this is compacted by the fact that there are only 7 current compiler reviewers. This is despite the fact that there are 23 members of compiler contributers and 13 members of the compiler team. Part of this is due to some people being on vacation. But it doesn't change the fact that I get a lot of PRs, and I'm sure so do the others in the review queue. I think we should be trying to get more people to be in the review queue. Of course, it's voluntary, but there's a few things that I think can help.

First, when new people are added as compiler contributors, we should actually be sure to ask if they want to be added to the queue (because it isn't automatic). I know I personally wasn't asked when I was added to the team. I'm not sure if the recent contributors @the8472 or @BoxyUwU (@LeSeulArtichaut already added themselves) know about this and want to review PRs.

Second, highfive does have the option to add people to specific directories. For people that are only comfortable reviewing specific bits of code, then this might be an option. I'm not sure exactly how reviewers get selected in those scenarios. For example, if a specific directory had ["someuser", "compiler-team", "compiler-team-contributors"], would that just change the list of possible reviewers to the union of the three, or does highfive first pick one from the directory list, and then if a group, select randomly from the group? I would expect the former, because the latter would probably increase the burden on individuals added to the directory (but I can see that it might make sense to select more knowledgeable people more often) . Theoretically, when there are multiple people (like wg-traits) that oversee a particular directory, we can also create new groups. The downside of adding people to individual directories is that directories don't always reflect content in a perfect way.

Finally, I think we should encourage people to be in the queue more often. if all 36 compiler/compiler-contributer team members were in the review queue, that would be 5x few PRs per person. Again, this is completely voluntary and not everyone has the time to review (for whatever reason), but if there are lots reviewing, it easy to pass a PR to another person and not feel as bad because you're adding to the dozen PRs they already have. Along these lines, even if all reviewers aren't fully comfortable r+ing the PR, at least having them as a "first eye" on the PR can be tremendously helpful.

All-in-all, I think there's definitely some progress that can be made here.

[the8472]

First, when new people are added as compiler contributors, we should actually be sure to ask if they want to be added to the queue (because it isn't automatic). I know I personally wasn't asked when I was added to the team. I'm not sure if the recent contributors @the8472 or @BoxyUwU (@LeSeulArtichaut already added themselves) know about this and want to review PRs.

I don't feel comfortable yet being assigned to review random PRs. And my experience currently lies more with the library/ tree rather than compiler/. But sometimes I see simple PRs that I would be comfortable with and know that the assigned reviewer has been busy. I don't know when it would be ok to take those unilaterally.

Another thing about reviews in the libs area are policy decisions. While a PR may look ok on the technical side there often are policy aspects to consider (e.g. API surface, how niche a feature is, maintenance burden) that are more difficult to judge.

[nagisa]

Heh, I didn't realise I needed to be in a list somewhere here; I was a potential reviewer in the past; not sure why I ended up being removed.

[jackh726]

I also just found rust-lang/highfive#266 by @petrochenkov; so, this is something thought about before.

[petrochenkov]

I also just found rust-lang/highfive#266 by @petrochenkov; so, this is something thought about before.

I no longer think that maintaining such an expert map manually is a good idea, the current git blame-based heuristic does it pretty well automatically.

I do still think that all active compiler team members and contributors should be assigned PRs randomly (rust-lang/highfive#278), which would also benefit from @rust-highfive reroll if an unsuitable reviewer is selected (this was suggested numerous times by many people, but never got implemented).

[petrochenkov]

@nagisa

not sure why I ended up being removed.

IIRC, you went on vacation and never returned.
That's not a unique problem, half of the compiler team is "on vacation" or some other leave right now (from rust-highfive's point of view).

rust-highfive needs a second "vacation" list that is subtracted from the first one, instead of outright removing people from the list, it should be more visible this way.

[michaelwoerister]

That's a good point, @jackh726. I think this should be addressed by the guidance section of the policy since it is one of the main practical issues we currently have. There's been some discussion about this recently on zulip but that has petered out again.

[petrochenkov]

One more person is removed from the reviewer list - rust-lang/highfive#345.
Now everything is assigned to just 8 people (6 team + 2 contributors).