stabilization template, docs #2219
Conversation
8d2e702 to
e4ee951
Compare
src/stabilization_guide.md
Outdated
| 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.
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.
Dropped this partial sentence from the initial version
src/stabilization_guide.md
Outdated
| @@ -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.
Maybe just change this section to team nominations, and put the lang team as the first item (and emphasize)?
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.
+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.
I guess saying "when comments die down, or if you don't get any comments, nominate"
There was a problem hiding this comment.
Fixed title, and tried to add a sentence to that effect.
src/stabilization_guide.md
Outdated
| 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.
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.
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.
To be honest, lang should have that process too :)
There was a problem hiding this comment.
Left this unaddressed, not for an initial version
| - (Positive/negative) Interface tests? (e.g. compiler cli interface) | ||
| - 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.
| Consider what the "edges" of this feature are. We're particularly interested in seeing tests that assure us about exactly what nearby things we're not stabilizing. | |
There was a problem hiding this comment.
Going even further:
| Consider what the "edges" of this feature are. We're particularly interested in seeing tests that assure us about exactly what nearby things we're not stabilizing. | |
| Within each test, include a comment at the top describing the purpose of the test and what set of invariants it intends to demonstrate. This is a great help to those reviewing the tests at stabilization time. | |
| Similarly, please consider including, when appropriate, `//@ reference:` annotations to connect each test with the corresponding item in the Reference. |
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 rust-lang/rust itself, as was recently done with the dev guide, and then the Reference PR becomes a part of the stabilization PR, though we're probably not yet ready to do that in general.)
@ehuss, @nikomatsakis, what do you think?
There was a problem hiding this comment.
it's often useful to us to directly push our own commits to a Reference PR branch rather than going back and forth with the author, but permissions on rust-lang/rust aren't currently set in a way that would enable that.)
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.
Thanks. I must have hit the odd case before then. Edited to correct.
There was a problem hiding this comment.
Note that for this initial version, I intentionally left off the
Similarly, please consider including, when appropriate,
//@ reference:annotations to connect each test with the corresponding item in the Reference.
I don't even know how to do that logistically myself.
src/stabilization_report_template.md
Outdated
|
|
||
| ## 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.
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.
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.
I added oli's question for the initial version
There was a problem hiding this comment.
This file contains the following:
Once we have decided to stabilize a feature, we need to have
a PR that actually makes that stabilization happen. These kinds
of PRs are a great way to get involved in Rust, as they take
you on a little tour through the source code.
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.
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.
Co-authored-by: lcnr <rust@lcnr.de>
Co-authored-by: Ralf Jung <post@ralfj.de>
Co-authored-by: waffle <waffle.lapkin@gmail.com>
|
@nikomatsakis do you mind if I push some changes to this PR (to address some of the more trivial nits)? This draft stabilization template is somewhat funny in that people are already using it for stabilization reports, but it's in an open PR xD Maybe we should merge an intermediate version? It obviously won't be perfect, but at least new stabilization reports would have something (live) to reference, and improvements to the template could be made in follow-ups following more feedback? Or I guess, this could wait for more thorough discussions during All Hands? |
|
people have been using this or have been asked to use this. We should really merge this now. We should move it to the forge instead and also address all the feedback but should happen later. |
|
I spoke with @nikomatsakis and @traviscross, and I'll try to address some outstanding feedback to push towards landing an initial imperfect version in rustc-dev-guide (with a lang team vibeck). Later, I'll move this initial stabilization template to Forge which seems like a better home than rustc-dev-guide, and follow-up improvements can be done there. |
|
|
(This should probably be squashed prior to merge, intermediate commits intended for review only) |
This PR updates the stabilization docs and introduces a new stabilization template based on a series of questions.
cc @rust-lang/lang -- this is designed to help us get the information we need
cc @rust-lang/compiler -- this is designed to ask questions to help gauge implementation quality
cc @rust-lang/types -- this is designed to surface potential type system interactons
cc @rust-lang/style, @rust-lang/rust-analyzer, and @rust-lang/lang-docs -- I'd like advice on how to advice users about doing updates outside of rustc.