-
Notifications
You must be signed in to change notification settings - Fork 14k
Stabilize Frontmatter #148051
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: main
Are you sure you want to change the base?
Stabilize Frontmatter #148051
Conversation
|
Some changes occurred in src/doc/style-guide cc @rust-lang/style |
|
r? @davidtwco rustbot has assigned @davidtwco. Use |
| gate_all!(unsafe_fields, "`unsafe` fields are experimental"); | ||
| gate_all!(unsafe_binders, "unsafe binder types are experimental"); | ||
| gate_all!(contracts, "contracts are incomplete"); | ||
| gate_all!(contracts_internals, "contract internal machinery is for internal use only"); |
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.
(Commenting on an arbitrary line to make this discussion threaded)
From the PR description (emphasis and ellipses mine):
Post-RFC changes
[…]
- acceptable places for frontmatter
- […]
- rustc: support removed from […]
include![…]- […]
include!can be used to inject code into the middle of a file which becomes ambiguous between a frontmatter start and a very negated number (----x)
In #146340 I actually struck a (temporary) compromise: We do still strip/lex/recognize frontmatter (and shebang) in item-context include!s. However, we no longer strip frontmatter in expression/statement-context include!s like in let _ = include!(…); (to prevent the manifold negation backcompat issue you've rightly mentioned).
Now, I don't know what's best here. I'm eager to know your and T-lang's stance on the matter. Note that we do strip shebang in expression/statement-context include!s which I consider to be quite odd (but still understandable on a lexical level). I have an open PR #146377 which would stop stripping shebang in that position, too. I still need to rebase+polish, lang-nominate and possibly crater it. The outcome of that T-lang discussion likely affects the decision mentioned in the first paragraph (†).
(†): If that PR was accepted it would mean that we would strip shebang+frontmatter in item-ctxt includes and wouldn't strip either(!) shebang or frontmatter in expr/stmt-ctxt includes which would make the behavior of shebang+frontmatter consistent thereby fulfilling the original goal of "This applies anywhere shebang stripping is performed." in this specific case (obviously the goal wasn't achieved in other cases, like in the proc_macro API 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.
Thanks! I've updated the stabilization report as well as linked to your comment as a place for discussing it.
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 now made that PR ready & lang-nominated it, sorry for the delay. See the nomination text here: #146377 (comment) (it's maybe a bit rambly since I threw it together rather quickly, I hope it's comprehensible enough).
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!
When working on the stabilization report (rust-lang#148051), I found it annoying to determine what cases were covered because areas of the frontmatter feature were either not in the file name or in inconsistent locations. This moves the area of frontmatter to the start of the file name and the moves to more specific the later in the file name so coverage is easier to see.
test(frontmatter): Rename tests to make coverage more obvious When working on the stabilization report (#148051), I found it annoying to determine what cases were covered because areas of the frontmatter feature were either not in the file name or in inconsistent locations. This moves the area of frontmatter to the start of the file name and the moves to more specific the later in the file name so coverage is easier to see. Tracking issue: #136889
test(frontmatter): Rename tests to make coverage more obvious When working on the stabilization report (rust-lang#148051), I found it annoying to determine what cases were covered because areas of the frontmatter feature were either not in the file name or in inconsistent locations. This moves the area of frontmatter to the start of the file name and the moves to more specific the later in the file name so coverage is easier to see. Tracking issue: rust-lang#136889
test(frontmatter): Rename tests to make coverage more obvious When working on the stabilization report (rust-lang#148051), I found it annoying to determine what cases were covered because areas of the frontmatter feature were either not in the file name or in inconsistent locations. This moves the area of frontmatter to the start of the file name and the moves to more specific the later in the file name so coverage is easier to see. Tracking issue: rust-lang#136889
Rollup merge of #148073 - epage:org-frontmatter, r=jieyouxu test(frontmatter): Rename tests to make coverage more obvious When working on the stabilization report (#148051), I found it annoying to determine what cases were covered because areas of the frontmatter feature were either not in the file name or in inconsistent locations. This moves the area of frontmatter to the start of the file name and the moves to more specific the later in the file name so coverage is easier to see. Tracking issue: #136889
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.
Implementation of this LGTM, r=me once t-lang has approved stabilisation
|
☔ The latest upstream changes (presumably #148090) made this pull request unmergeable. Please resolve the merge conflicts. |
test(frontmatter): Rename tests to make coverage more obvious When working on the stabilization report (rust-lang/rust#148051), I found it annoying to determine what cases were covered because areas of the frontmatter feature were either not in the file name or in inconsistent locations. This moves the area of frontmatter to the start of the file name and the moves to more specific the later in the file name so coverage is easier to see. Tracking issue: rust-lang/rust#136889
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@rustbot label +I-lang-nominated This has been quiet for a while. There is an open issue on the rustdoc side but I figure that can be worked out in parallel to T-lang discussions in case T-lang uncovers anything. Even if this goes straight to FCP, someone can raise a blocking concern on behalf of rustdoc. |
|
Having now read through the stabilization report and the linked PRs, issues, and threads, this seems to me to be ready for stabilization modulo the open rustdoc item mentioned by @epage, getting the Reference PR into a final state, and documenting the behavior of @rfcbot fcp merge lang Thanks to @epage for the thorough stabilization report and for his many efforts in pushing along this feature. |
|
Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members: Concerns:
Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns. |
|
@rfcbot concern document-frontmatter-removal In the Reference, we document shebang removal, including the exception for when Let's please document the behavior of frontmatter removal, including how it interacts with the documented shebang removal exception and how this removal is applied by It may also be worth including this in the core library documentation, but I'd at least like to see this in the Reference. In particular, I want to see this detail documented to confirm my understanding is correct on exactly what we're accepting as part of this stabilization. |
|
@rfcbot concern do-we-care-about-context-free-grammar As far as I'm aware, 1) Rust currently has a context-free grammar, and 2) without a finite upper bound on the number of dashes used for a fence (as we do with raw string literals), the grammar for frontmatter would not be context-free and would make Rust a context-sensitive language. (In testing, I notice that the compiler ICEs with greater than 65535 fencing dashes with "Formatting argument out of range" (presumably due to #136932), but this limit isn't documented, doesn't seem intentional, and would be much higher than the limit of 255 Do we care about preserving Rust's context-free grammar? Might we want to set the same upper limit here that we do for raw string literals? I'm filing this concern to hold space for that discussion. |
|
@rfcbot concern do-we-want-to-allow-carriage-returns For raw string literals, I notice that we don't allow any carriage returns that have survived CRLF normalization. I can think of a number of good reasons for this. Do any of those reasons apply to frontmatter (which currently allows them)? I'm thinking, e.g., about the possibility of a line such as |
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.
rfcbot concern do-we-care-about-context-free-grammar
As far as I'm aware, 1) Rust currently has a context-free grammar, and 2) without a finite upper bound on the number of dashes used for a fence (as we do with raw string literals), the grammar for frontmatter would not be context-free and would make Rust a context-sensitive language.
(In testing, I notice that the compiler ICEs with greater than 65535 fencing dashes with "Formatting argument out of range" (presumably due to #136932), but this limit isn't documented, doesn't seem intentional, and would be much higher than the limit of 255
#s for a raw string literal.)Do we care about preserving Rust's context-free grammar? Might we want to set the same upper limit here that we do for raw string literals? I'm filing this concern to hold space for that discussion.
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.
Do we care about preserving Rust's context-free grammar?
Are we in a similar situation with shebangs?
The RFC originally framed frontmatters as being stripped like shebangs. I'm assuming that it is an implementation detail that this is currently being handled inside the lexer rather than as a strip before hand.
Might we want to set the same upper limit here that we do for raw string literals?
Is there a reason you put this under the same concern as being CFG? On the surface, they seem independent which could make talking about them harder and risks losing track of one
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.
Sorry if I'm making a mistake here, but the shebang has a natural and deterministic delimiter, doesn't it? '\n'
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.
Ah, it isn't the ambiguity that is a concern for context free but the capping on the delimiters.
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, exactly, that's why we have the limit of u8::MAX on the effective delimitation for raw strings. The same rule could be applied here.
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.
@rfcbot concern document-frontmatter-removal
In the Reference, we document shebang removal, including the exception for when
#!is followed (even after intervening comments and/or whitespace) by[, and we document (in an admonition) the behavior ofinclude!with respect to this shebang removal.Let's please document the behavior of frontmatter removal, including how it interacts with the documented shebang removal exception and how this removal is applied by
include!(i.e., including with respect to how this differs between item and expression contexts).It may also be worth including this in the core library documentation, but I'd at least like to see this in the Reference. In particular, I want to see this detail documented to confirm my understanding is correct on exactly what we're accepting as part of this stabilization.
I don't think we particularly need to preserve CFG as a property here for its own sake, and don't think we should do it for that reason, but for purely practical reasons I think it'd be reasonable to say "at most 255 dashes". |
|
I would also like the 255 limit. It was useful in strings to have a smaller count to keep the in-memory sizes small anyway. And really, if 255 isn't enough to fix whatever you're trying to fix, I don't think more will fix it either, which to me is the biggest reason to do this. (Like Josh, I think this is a solid change regardless of CFG-ness, and while maintaining context-free is nice when it's otherwise reasonable, it's not the biggest reason to do this.) |
|
Since there seems general interest around this, I'll prepare a PR for capping the fence length |
|
/concern frontmatter-stripping This came up in today's lang meeting when discussing #146377, and shared by Josh here: Essentially, the discussion was whether we can or want to not strip both frontmatter and shebang in item-ctxt To best answer the question of "should we remove stripping of shebang in item-ctxt I also don't want to hard block this stabilization on the above question, in the spirit of "don't let perfect be the enemy of great". I'm filing a concern, but I expect that I'll withdraw it once #146377 lands and other concerns are withdrawn. |
|
Proxying that lang-advisor concern: @rfcbot concern jack-should-we-strip-frontmatter-in-item-context |
|
#149358 limits frontmatte fence length |
Stabilization report
FCP
Summary
This report proposes the stabilization of
#[feature(frontmatter)]in preparation for Cargo Script to be stabilized.A frontmatter is a special kind of attribute intended to be read and modified by external tools, like Cargo. This puts a particular constraint on this attribute in that full awareness of the Rust grammar would be prohibitive.
For example:
The goal of Cargo script is to improve:
Closes #136889
Tracking:
frontmatter#136889Reference PRs:
cc @rust-lang/lang @rust-lang/lang-advisors, @rust-lang/compiler
What is stabilized
This is stabilizing frontmatter, an optional section in a source file that can only exist before any Rust code or comments, for use by external tools.
After the opening frontmatter fence, an infostring is allowed that can identify the contents.
Calling tools are responsible for interpreting the infostring.
What isn't stabilized
Only a subset of the allowed infostring syntax is being stabilized.
(XID_Start |_) ( XID_Continue |-|.)*-in the start of an infostring which is ambiguous with frontmatter open which accepts 3 or more-Future possibilities include:
Design
Reference
RFC history
Answers to unresolved questions
The RFC has no unresolved questions
Post-RFC changes
--cfg(Disallow frontmatter in--cfgand--check-cfgarguments #146137) and proc-macros (Strip frontmatter in fewer places #146340)include!in expression/statement-content (still supported in item context)include!(…)#146377 for having shebangs match--cfginclude!can be used to inject code into the middle of a file which becomes ambiguous between a frontmatter start and a very negated number (----x)Key points
---needs to be escaped---at the start of a line needs escaping---needing escaping-ZunprettyNightly extensions
No nightly extensions exist
Doors closed
There are no known efforts that this affects.
Feedback
Call for testing
Call for testing with feedback at:
Previous call for testing: rust-lang/rfcs#3424 (comment) with some feedback at rust-lang/cargo#12207 (comment)
Overall, feedback is enthusiastic. Any quibbles are with the Cargo side.
Nightly use
Samples from grepping for
-ZscriptImplementation
Major parts
Parts:
PRs:
TokenStream::new#145568--cfgand--check-cfgarguments #146137Coverage
Tests (directory):
,in the infostring.or-.or-elsewhereuse---doesn't need escaping-escape lines starting with fewer-include!doesn't treat---as frontmatter---as frontmatterOutstanding bugs
No known bugs
Outstanding FIXMEs
An idea for future editions
rust/compiler/rustc_parse/src/lib.rs
Lines 176 to 180 in 779e19d
Diagnostic improvements can be iterated on over time, particularly with use feedback
rust/tests/ui/frontmatter/frontmatter-after-tokens.rs
Lines 3 to 6 in 779e19d
rust/tests/ui/frontmatter/multifrontmatter.rs
Lines 1 to 7 in 779e19d
Both can be handled as need is shown.
Tool changes
Breaking changes
No known breaking change
Type system, opsem
Compile-time checks
N/A, this is only relevant to tools inspecting the source
Type system rules
N/A, this is only relevant to tools inspecting the source
Sound by default?
No, this is only relevant to tools inspecting the source
Breaks the AM?
No, this is only relevant to tools inspecting the source
Common interactions
Temporaries
No, this is only relevant to tools inspecting the source
Drop order
No, this is only relevant to tools inspecting the source
Pre-expansion / post-expansion
No, this is only relevant to tools inspecting the source
Edition hygiene
N/A, this is independent of editions
SemVer implications
N/A, downstream users cannot access this feature
Exposing other features
Not aware of any
History
PRs
TokenStream::new#145568--cfgand--check-cfgarguments #146137Acknowledgments
Open items