Merge pull request #230 from nikomatsakis/2019-12-20-minutes

summarize design meeting from 2019-12-20 on major change process

Author: nikomatsakis

content/minutes/design-meeting/2019-12-20-major-change-process.md

@@ -0,0 +1,186 @@
+# Major Change Process
+
+* [Zulip thread](https://zulip-archive.rust-lang.org/131828tcompiler/44719designmeeting20191220.html)
+
+## Summary
+
+* We want some kind of system where people advertise changes that they are making or plan to make 
+    * and the team can give high-level feedback early
+    * and -- if we decide to go with the change -- we can ensure there is a reviewer beforehand
+* This document describes motivations and a specific "early draft" proposal
+
+## Motivations
+
+Proposal is to add *some* sort of notification / lightweight process before making major changes.
+
+In contrast, today there is no stated process for a "major change" beyond just opening a PR.
+Of course some changes get a lot of discussion before hand, but others do not.
+
+Some problems that we see with today's system:
+
+* Sometimes we get large PRs that have attempted a major change without any discussion beforehand
+    * There may not be a reviewer with time and interest
+    * Also reviewing without context is very difficult
+    * There may be concerns about the technical approach or direction
+    * Starts the discussion with "accept or reject this PR" versus "what is best approach"
+        * On the other hand, a more concrete discussion can be more effective
+* Often, as a result, these PRs can sit for a very long time without any feedback
+    * this is frustrating for everyone involved
+* Current "lack of system" can also be a turnoff
+    * do a lot of work to prep PR, but can you know if that change is welcome?
+* Over time, a focus on PRs (versus explaining the idea) leads to
+    * more tech debt and less overall cohesion
+    * lack of documentation
+
+Some strengths of today's system that we would like to preserve:
+
+* Low barrier to entry, not a lot of "bureaucratic overhead"
+* People should be able to experiment without "authorization" before hand
+* We don't want a lot of overhead for the *compiler team* to manage some authorization process
+
+We would know the system is working if:
+
+* We have a better idea of what is being done and by whom
+    * and also whether the team has approved of that direction
+* Major changes will be discussed before they become a PR that is up for active review
+    * reviewers will be identified beforehand
+* When reviewing a PR, reviewers will have a better idea of its goals and how it fits into the bigger picture
+* We still have a "high throughput" system and we don't spend a ton of time on "bureaucratic overhead"
+    * in particular we should be able to "green light" changes fairly quickly and we should do that most of the time
+
+## Initially proposed process
+
+* When considering or experimenting with a "major change", open an issue on compiler-team repo to let people know
+    * Describe the general idea in a sentence or two
+    * Identify mentors or reviewers, if you are working with one
+* These issues will be reviewed weekly and classified
+    * New triage process? Part of existing process?
+        > [name=Felix S Klock II] Note we already often go-over allotted time at the Thursday triage meeting. We *can* add this to the agenda, but we need to figure out what else will get de-prioritized.
+    * Is weekly a good frequency?
+        * mark: this is pretty high latency for some of these changes,
+          we might otherwise merge them in a week's time
+          Maybe that's not a bad thing though :)
+* Some possible outcomes:
+    * Closed -- this doesn't seem like something we want to do
+    * Requires design meeting -- requires a larger group
+    > [name=Felix S Klock II] should also add "Needs RFC" as potential outcome (a more extreme variant on "Needs Design Mtg") 
+    * Deferred -- not deciding for now, or trying to find a reviewer
+    * Approved for experimentation
+        * once PR is ready, nominate for discussion
+        * may request "mitigation", such as a `-Z` flag
+        * implementor should understand that we may just decide the idea isn't worth it
+    * Approved to land -- requires a willing reviewer
+        * no special approval required to land, just r+
+* If a "major change" PR is opened without going through this process
+    * Close with a friendly note recommending an issue be opened (we should write a standard template with a link)
+
+## What is a "major change"?
+
+* "You know it when you see it"
+* If it is a major time commitment to review it, it probably qualifies
+    * This might be beause it affects many parts of the compiler
+    * But it might also be a narrow change with subtle implications
+    * Or require reading up on a relevant RFC or other background material
+* Examples of major changes:
+    * Allocate HIR on an arena:
+        * Yes, because it touches a set of data structures used throughout the whole compiler
+        * However, presuming we could find a reviewer, this would be something we'd like approve quickly within the initial triage because it is fairly mechanical and doesn't require a design meeting
+    * Introducing chalk, nll, or polonius
+        * Yes, replacing a major component
+        * This kind of effort might be "redirected" to forming a working group to help lead the design and implementation
+            * (which is of course what we have done)
+    * Changing universally used internal APIs
+        * Heads up, `PlaceBase::Static` is being removed, breaking every single use site of place.base: https://github.com/rust-lang/rust/pull/67000
+* Grey zone, let's discuss:
+    * Const propagation
+        * It is major in that it is something that has been discussed quite a bit, multiple people might have ideas on it
+        * It is low-risk because it can easily be turned off at any time
+    * [Use the recorded types in MIR to determine generator auto-trait implementations](https://github.com/rust-lang/rust/issues/65782)
+    * [Implement RFC 2532 – Associated Type Defaults](https://github.com/rust-lang/rust/pull/61812)
+        * contained, but reviewing is a lot of work
+        * Niko has rather inexcusably let this sit far too long (plans to change that)
+        * *But* it's an example of something that's hard to schedule, and where some up-front notice might've been helpful (or could've given warning that won't have available bandwidth until later)
+* Examples of minor changes:
+    * Fix some ICE
+    * Local optimizations
+
+## Why this will help
+
+* For people who have very full calendars, being able to have a "heads up" of larger changes and to integrate into a review schedule could be helpful
+    * but it will take discipline to use effectively
+
+## Notes from the meeting
+
+* simulacrum points out that it would be good to have some kind of "fast path" if you have a reviewer and you have documented things, so that no meeting is required at all
+    * the only "hard block" would be if you don't have a "partner" or "sponsor" from compiler team
+    * somewhat analogous to the project group lang team concepts
+* reviewer not expected to be a pair programmer
+* one possible definition for "what is a major change" might be "what would modify the rustc-guide"
+    * or, since rustc-guide is always a WIP, "welp this *should've* in the guide and if it were, it *would've* required a modification"
+* what to do with new PRs that don't follow the process?
+    * should we close them?
+    * maybe have a canned comment and give them some amount of time
+    * this comment might also emphasize the role of documentation
+* [we discussed and settled on](https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/design.20meeting.202019-12-20/near/183945490)
+    * leave a nice message, which it S-waiting-on-author
+    * close per usual triage process if no issue is filed within a certain amount of time
+    * if an issue is filed but it is not "green lighted", then we can close the PR
+        * i.e., if we decide that a design meeting or broader project group is needed
+* when a project is proposed, what are the possible responses?
+    * I have concerns
+    * I approve but don't have ability/time to review
+    * I approve on an experimental basis; we should discuss again when we gain more experience
+    * I am happy to review but I would like another to approve too
+    * I am happy to review and I think we can just go forward (only possible for "members")
+* can we make a Zulip stream where each issue creates a topic?
+* how to handle experimentation?
+    * we should have some way to add "caveats", like 
+        * would like to review performance results
+        * we need a `-Z` flag
+        * we need docs :)
+* final discussion point was about exactly how to handle requests for rustc-guide edits
+    * since a major change is part of a rustc-guide change, it makes sense that it should come accompanied with a rustc-guide write-up
+    * ideally this would come along with the compiler-team issue
+        * but maybe it would be more something we wait for until issue is *approved* or, in extreme cases, co-develop with author
+    * if we want to see more docs, we are going to have to start holding the line *somewhere*
+    * sometimes it's not possible or desirable to write complete docs before-hand
+        * details may change through review process
+        * person may not know enough context to write the docs, need help with that
+    * but the bar should be that the issue can **explain the change** in sufficient detail for it to be understood
+        * the *actual* rustc-guide changes themselves can come later
+        * it may be that the role of the learning wg can be to help with some of that
+
+## Final proposed process
+
+* When considering or experimenting with a "major change", open an issue on compiler-team repo to let people know
+    * Describe the general idea in a sentence or two
+    * Identify mentors or reviewers, if you are working with one
+    * There will be some "prototype" to guide people in this
+* What is a "major change"?
+    * something where it would make sense to update rustc-guide
+    * if rustc-guide doesn't cover this code yet, then you may have to use your imagination about what *ought* to be documented :)
+* These issues will be reviewed by compiler team members
+    * Compiler team members and contributors can leave concerns and approvals asynchronously (see below)
+        * maybe we can make a dedicated Zulip stream where new things get 
+    * In particular, note that a compiler team member who is confident something is correct and will not be controversial can just go ahead and approve and act as reviewer
+        * though there should still be an issue
+    * But there should also be some synchronous sweep, not clear when that should occur
+        * maybe as part of meta wg?
+        * perhaps just team co-leads do it on a regular basis?
+        * existing triage meeting is too full, that's clear
+* Feedback from a compiler team member or contributor typically has the form (these are not fully orthogonal):
+    * I have concerns (give details)
+        * this might lead to more details
+        * or a design meeting
+        * or an RFC
+        * or just closing the idea
+    * I approve but don't have ability/time to review
+    * I approve but with some caveats, e.g. we should examine perf afterwards, would like to re-review, or want a `-Z` flag
+    * I am happy to review but I would like someone else to approve too (must be a "compiler team contributor")
+    * I am happy to review and I think we can just go forward (only possible for "members")
+        * the idea here is that if you are an expert on the code and confident this is a good path, that's fine, do it
+* If a "major change" PR is opened without going through this process
+    * We post a standard comment that directs people to open an issue
+    * And the PR is marked as waiting on author
+        * It can be closed per the usual triage process if author does not respond
+    * If the issue turns out to be controversial (i.e., nobody steps up as reviewer write away), then we close the PR and just focus on discussing in the issue