Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?
stabilization template, docs #2219
Changes from 1 commit
e4ee951
c63baad
32824bf
6c99b75
2536baa
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to note on current expectations on if this work is blocking (and to what extent)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, this in regards to stabilization, but I do think this comment fits here (i.e. maybe the guideline is "you shouldn't try to stabilize if the affiliated work is not completed")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would, to be honest I'm a bit unclear on the answer which is why I cc'd the relevant teams =)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say that all of the currently listed things are de-facto blocking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For rust-analyzer it depends on the feature really. Imo we don't need to fully support something (see async closures for example). A blocking concern is if the feature degrades the user experience by existing while rust-analyzer hasn't implemented it yet, think new syntax that r-a can't parse -> bogus diagnostics, type inference changes -> bogus diagnostics. I think in fact, its mainly whether a change will make r-a emit false diagnostics due to lacking support, though there might be more things I can't think of right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Less often, but sometimes new features may involve coordination with cargo as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Syntax for accessing features is outdated:
and
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: I think it's also important to summarize design decisions and deviations that have been made since the RFC was accepted (esp. decisions that were FCP'd or otherwise accepted by relevant teams). Many times, the final stabilized feature only vaguely resembles the originally proposed RFC (probably more faithful in preserving intention/use cases but less the exact form).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The 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 comment
The 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 comment
The 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 comment
The 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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Note that people who are not members of the org cannot use team pings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is possible for PR author to ask the reviewer to cc relevant teams on the PR author's behalf, but yes, this can be confusing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should mention that it is a stabilization for language features, at least in passing. Similar to the big note at the top of stabilization_guide.md itself.
With the current wording it is way to easy to stumble upon this link, assume it applies to T-libs stabilization as well, and be very confused.
(Source: me. Someone asked how to stabilize a library feature, and I tried to be helpful and suggested they try out this new shiny still-in-beta template for the stabilization report. And it took me some time to realize that this is not quite applicable there)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it would be useful to mention any OSS nightly users there. usually when a feature works, feedback is quite scant but "searched grep.app for
#![feature(...)]
and had N results" gives a good gauge I think.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the big categories I think are:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remark: I suggested this but the wording feels too vague to me, would be great if anyone has any suggestions for categories of test coverage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going even further:
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.
Choose a reason for hiding this comment
The 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 comment
The 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 comment
The 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 comment
The 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"