-
Notifications
You must be signed in to change notification settings - Fork 550
stabilization template, docs #2219
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 2 commits
e4ee951
c63baad
32824bf
6c99b75
2536baa
882699e
1bbe997
3f559ff
8152a0d
f80902e
fa8dce1
784f90e
ef25866
1831074
5ad366e
c7e38e9
beb25e1
6fe303f
eb4eba8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file contains the following:
And I'm not sure how accurate this is. First of all the "Once we have decided to stabilize a feature" part is too vague. How would a new contributor know which features we decided to stabilize? Additionally the decision and stabilization report need to be done by someone with a good knowledge of the feature, a new contributor just isn't suitable for that. I feel like this paragraph is misguiding, as stabilization is usually an intertwined process involving multiple teams, sometimes confusing changes (like changing lints that don't make sense anymore), checking many components, etc. If for library features it might be the case that a novice can craft a stabilization PR, for language features, more often than not that's not the case. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For this initial template, I will drop that paragraph entirely, and refrain from making substantial wording additions to make this PR less contentious to land. I have some Thoughts:tm: on this matter as well.
jieyouxu marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,44 +43,14 @@ has completed. Meanwhile, we can proceed to the next step. | |
|
||
## Write a stabilization report | ||
|
||
Find the tracking issue of the feature, and create a short | ||
stabilization report. Essentially this would be a brief summary | ||
of the feature plus some links to test cases showing it works | ||
as expected, along with a list of edge cases that came up | ||
and were considered. This is a minimal "due diligence" that | ||
we do before stabilizing. | ||
|
||
The report should contain: | ||
|
||
- A summary, showing examples (e.g. code snippets) what is | ||
enabled by this feature. | ||
- Links to test cases in our test suite regarding this feature | ||
and describe the feature's behavior on encountering edge cases. | ||
- Links to the documentations (the PRs we have made in the | ||
previous steps). | ||
- Any other relevant information. | ||
- The resolutions of any unresolved questions if the stabilization | ||
is for an RFC. | ||
|
||
Examples of stabilization reports can be found in | ||
[rust-lang/rust#44494][report1] and [rust-lang/rust#28237][report2] (these links | ||
will bring you directly to the comment containing the stabilization report). | ||
|
||
[report1]: https://github.com/rust-lang/rust/issues/44494#issuecomment-360191474 | ||
[report2]: https://github.com/rust-lang/rust/issues/28237#issuecomment-363374130 | ||
|
||
## FCP | ||
|
||
If any member of the team responsible for tracking this | ||
feature agrees with stabilizing this feature, they will | ||
start the FCP (final-comment-period) process by commenting | ||
Author a stabilization report using the [template found in this repository][srt]. | ||
Stabilization reports summarize the work that has been done since the RFC. | ||
jieyouxu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
The [template][srt] includes a series of questions that aim to surface interconnections between this feature and the various Rust teams (lang, types, etc) and also to identify items that are commonly overlooked. | ||
|
||
```text | ||
@rfcbot fcp merge | ||
``` | ||
[srt]: ./stabilization_report_template.md | ||
|
||
The rest of the team members will review the proposal. If the final | ||
decision is to stabilize, we proceed to do the actual code modification. | ||
The stabilization report is typically posted as the main comment on the stabilization PR (see the next section). | ||
If you'd like to develop the stabilization report incrementally, we recommend adding it to | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. incomplete: "adding it to"... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh yeah, I never finished. I was going to suggest the practice of adding the stabilization report to the unsafe book and updating it incrementally (e.g., with a list of PRs, if nothing else). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dropped this partial sentence from the initial version |
||
|
||
## Stabilization PR | ||
nikomatsakis marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
|
@@ -194,3 +164,23 @@ if something { /* XXX */ } | |
[Rust by Example]: https://github.com/rust-lang/rust-by-example | ||
[`Unstable Book`]: https://doc.rust-lang.org/unstable-book/index.html | ||
[`src/doc/unstable-book`]: https://github.com/rust-lang/rust/tree/master/src/doc/unstable-book | ||
|
||
## Lang team nomination | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe just change this section to Also, a discussion for another place, but this made me think: Is it all that great to nominate things immediately? There are usually plenty of comments on things and it might be worth delaying the teams discussing things until those immediate comments get addressed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 to the more general title. I wondered the same thing about when to nominate. I don't know the right answer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess saying "when comments die down, or if you don't get any comments, nominate" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed title, and tried to add a sentence to that effect. |
||
|
||
When you feel the PR is ready for consideration by the lang team, you can [nominate the PR](https://lang-team.rust-lang.org/how_to/nominate.html) to get it on the list for discussion in the next meeting. You should also cc the other interacting teams to review the report: | ||
|
||
* `@rust-lang/types`, to look for type system interactions | ||
* `@rust-lang/compiler`, to vouch for implementation quality | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe this should be an explicit step? For either the compiler team as a whole, or at least just the "owner" of the implementation to be on board with stabilization. Otherwise, "everything" should be compiler nominated? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this is a good question. My feeling is that both compiler and types ought to have a process of assigning one person to prepare a summary for others to read, but I felt that needed more discussion before I wrote it in here. I should probably open a tracking issue on this overall topic. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be honest, lang should have that process too :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Left this unaddressed, not for an initial version |
||
* `@rust-lang/opsem`, but only if this feature interacts with unsafe code and can create undefined behavior | ||
* `@rust-lang/libs-api`, but only if there are additions to the standard library | ||
jieyouxu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
## FCP proposed on the PR | ||
|
||
Finally, some member of the team responsible for tracking this feature agrees with stabilizing this feature, will | ||
start the FCP (final-comment-period) process by commenting | ||
|
||
```text | ||
@rfcbot fcp merge | ||
``` | ||
|
||
The rest of the team members will review the proposal. If the final decision is to stabilize, the PR will be reviewed by the compiler team like any other PR. |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,54 @@ | ||||||||||||||||||||
# Stabilization report template | ||||||||||||||||||||
|
||||||||||||||||||||
> **What is this?** This is a template to use for [stabilization reports](./stabilization_guide.md). It consists of a series of questions that aim to provide the information most commonly needed and to help reviewers be more likely to identify potential problems up front. Not all parts of the template will apply to all stabilizations. Feel free to put N/A if a question doesn't seem to apply to your case. | ||||||||||||||||||||
jieyouxu marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||
|
||||||||||||||||||||
## General design | ||||||||||||||||||||
|
||||||||||||||||||||
### What is the RFC for this feature and what changes have occurred to the user-facing design since the RFC was finalized? | ||||||||||||||||||||
|
||||||||||||||||||||
### What behavior are we committing to that has been controversial? Summarize the major arguments pro/con. | ||||||||||||||||||||
|
||||||||||||||||||||
### Are there extensions to this feature that remain unstable? How do we know that we are not accidentally committing to those? | ||||||||||||||||||||
|
||||||||||||||||||||
## Has a call-for-testing period been conducted? If so, what feedback was received? | ||||||||||||||||||||
jieyouxu marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||
|
||||||||||||||||||||
## Implementation quality | ||||||||||||||||||||
|
||||||||||||||||||||
### Summarize the major parts of the implementation and provide links into the code (or to PRs) | ||||||||||||||||||||
|
||||||||||||||||||||
An example for async closures: https://rustc-dev-guide.rust-lang.org/coroutine-closures.html | ||||||||||||||||||||
|
||||||||||||||||||||
### Summarize existing test coverage of this feature | ||||||||||||||||||||
|
||||||||||||||||||||
- What does the test coverage landscape for this feature look like? | ||||||||||||||||||||
- (Positive/negative) Behavioral tests? | ||||||||||||||||||||
- (Positive/negative) Interface tests? (e.g. compiler cli interface) | ||||||||||||||||||||
jieyouxu marked this conversation as resolved.
Show resolved
Hide resolved
jieyouxu marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||
- Maybe link to test folders or individual tests (ui/codegen/assembly/run-make tests, etc.) | ||||||||||||||||||||
- Are there any (intentional/unintentional) gaps in test coverage? | ||||||||||||||||||||
|
||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Going even further:
Suggested change
In reviewing the tests for the arbitrary self types stabilization, I'm reminded how helpful it is for each test to describe at the top what it is intending to demonstrate, so it's worth mentioning that. It's also probably worth mentioning here the utility of the Reference annotations, but that raises an interesting ordering question. We merge tests ahead of the stabilization, generally, but then we don't merge the Reference PR until after the stabilization. So we'd either need to merge the tests with dangling references (to identifiers in unmerged Reference PRs) or perhaps these references could be added to the tests in the stabilization PR itself. Or they could be added later, but then these helpful things aren't there when reviewing the stabilization. (Another wilder option is that we merge the Reference into @ehuss, @nikomatsakis, what do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
AFAIK team members can push to PRs in rust-lang/rust as well, but individual PR authors can opt-out of that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks. I must have hit the odd case before then. Edited to correct. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that for this initial version, I intentionally left off the
I don't even know how to do that logistically myself. |
||||||||||||||||||||
### What outstanding bugs in the issue tracker involve this feature? Are they stabilization-blocking? | ||||||||||||||||||||
|
||||||||||||||||||||
### Summarize contributors to the feature by name for recognition and assuredness that people involved in the feature agree with stabilization | ||||||||||||||||||||
|
||||||||||||||||||||
### What FIXMEs are still in the code for that feature and why is it ok to leave them there? | ||||||||||||||||||||
nikomatsakis marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||
|
||||||||||||||||||||
### Which tools need to be adjusted to support this feature. Has this work been done? | ||||||||||||||||||||
|
||||||||||||||||||||
*Consider rustdoc, clippy, rust-analyzer, rustfmt, rustup, docs.rs.* | ||||||||||||||||||||
|
||||||||||||||||||||
## Type system and execution rules | ||||||||||||||||||||
|
||||||||||||||||||||
### What compilation-time checks are done that are needed to prevent undefined behavior? | ||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar question: Does the feature's implementation need checks to prevent UB or is it sound by default and needs opt in in places to perform the dangerous/unsafe operations? If it is not sound by default, what is the rationale? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've been thinking about generalizing this question -- like, "what type system rules are enforced and what is their purpose" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added oli's question for the initial version |
||||||||||||||||||||
|
||||||||||||||||||||
(Be sure to link to tests demonstrating that these tests are being done.) | ||||||||||||||||||||
|
||||||||||||||||||||
### Can users use this feature to introduce undefined behavior? (Describe.) | ||||||||||||||||||||
nikomatsakis marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||
|
||||||||||||||||||||
### What updates are needed to the reference/specification? (link to PRs when they exist) | ||||||||||||||||||||
|
||||||||||||||||||||
## Common interactions | ||||||||||||||||||||
|
||||||||||||||||||||
### Does this feature introduce new expressions and can they produce temporaries? What are the lifetimes of those temporaries? | ||||||||||||||||||||
|
||||||||||||||||||||
### What other unstable features may be exposed by this feature? | ||||||||||||||||||||
|
Uh oh!
There was an error while loading. Please reload this page.