-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Implement RFC 3503: frontmatters #140035
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
Implement RFC 3503: frontmatters #140035
Conversation
This comment has been minimized.
This comment has been minimized.
I'll do a review pass next Monday/Tuesday, but in the meantime, I'll roll this to Wesley who I think have more context on the general vibes. r? @wesleywiser |
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 for the implementation! I left some (adversarial) review notes under the impression based on reading back #137193 that frontmatter is a Rust language syntax. However, the existing diagnostics here I find is already quite nice.
I especially appreciate using error annotations in between frontmatter openers/closers, I found that very clever (and cute) 😆
#![feature(frontmatter)] | ||
|
||
--- | ||
//~^ ERROR: expected item, found `-` | ||
// FIXME(frontmatter): make this diagnostic better | ||
--- |
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: yeah, I'm not sure how to detect this properly without making some convoluted logic, even though I imagine this would be one of the more common mistakes to make. At least this does error, maybe we could provide some kind of contextual HELP like if we see ---
(3+ starting dashes)
error: expected item, found `-`
--> $DIR/frontmatter-after-tokens.rs:3:1
|
LL | ---
| ^ expected item
|
= note: for a full list of items that can appear in modules, see <https://doc.rust-lang.org/reference/items.html>
= help: if you meant to write a frontmatter, the frontmatter must come after an optional shebang but before any regular source code
(With some better wording, I find it tricky to explain)
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.
Words are hard 🙂
If the file doesn't start with a shebang line, maybe we could just say something like
= help: if you meant to write a frontmatter, the frontmatter must appear first in the file
Shebangs are not super commonly used so if we can leave that detail out, it would simplify the error message.
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'm pretty sure this can be achieved not at the lexer/parser level but at the tokens to AST level. That code would not reside in places this PR touches so we can do this as a separate followup! :)
☔ The latest upstream changes (presumably #140180) made this pull request unmergeable. Please resolve the merge conflicts. |
#![feature(frontmatter)] | ||
|
||
--- | ||
//~^ ERROR: expected item, found `-` | ||
// FIXME(frontmatter): make this diagnostic better | ||
--- |
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.
Words are hard 🙂
If the file doesn't start with a shebang line, maybe we could just say something like
= help: if you meant to write a frontmatter, the frontmatter must appear first in the file
Shebangs are not super commonly used so if we can leave that detail out, it would simplify the error message.
cc @epage for awareness 🙂 |
Thanks for your work on this! |
compiler/rustc_lexer/src/cursor.rs
Outdated
#[cfg(debug_assertions)] | ||
prev: char, | ||
} | ||
|
||
pub(crate) const EOF_CHAR: char = '\0'; | ||
|
||
impl<'a> Cursor<'a> { | ||
pub fn new(input: &'a str) -> Cursor<'a> { | ||
pub fn new(input: &'a str, frontmatter_allowed: bool) -> Cursor<'a> { |
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.
Now that we have an implementation using the lexer, instead of stripping (#137193), what are your thoughts on the trade offs?
The cases I can think of
- General code design. I can't speak to this
- Other tools (r-a, rustfmt, etc). I suspect this will need slightly more work for them to passively support the new syntax. For richer features (processing the content, mutating), either they'll still need their own parser or the token's capabilities will need to be expanded which is where we were at with Add unstable frontmatter support #137193
- Other uses for the token existing? I can't think of any but maybe you have some in mind.
- Diagnostics: Provide helpful diagnostics for shebang lookalikes #137619 shows that stripping can still have decent diagnostics. I can't speak to how bad it would be to implement each side
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.
#137619 still does error reporting in the parsing step, which still suggests we can't report errors in the lexer level. As far as my knowledge of the compiler goes, parsing it as a token and reporting it when the token gets "cooked" in the parsing step is pretty much the best way. That is how the implementation for c"str"
literals worked and it was stabilized with that implementation.
f9112c6
to
4f62442
Compare
rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead. cc @rust-lang/rust-analyzer |
This comment has been minimized.
This comment has been minimized.
4f62442
to
d979912
Compare
This comment has been minimized.
This comment has been minimized.
d979912
to
54e4711
Compare
Re. permitting whitespace between dashes and infostring #140035 (comment). |
Reminder, once the PR becomes ready for a review, use |
70a2a0f
to
818ec6b
Compare
@rustbot ready |
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.
Impl looks good to me, not much to say other than a few tiny test nits (that I realized just now).
818ec6b
to
7917bee
Compare
LGTM, waiting a bit in case Wesley has more feedback. |
compiler/rustc_lexer/src/lib.rs
Outdated
.or_else(|| rest.find("\nuse")) | ||
.or_else(|| rest.find("\n//!")) | ||
.or_else(|| rest.find("\n#. This might break rust-analyzer. Will look into how to fix that. Suggestions welcome for how to improve diagnostics.
…llaumeGomez Rollup of 11 pull requests Successful merges: - rust-lang#139764 (Consistent trait bounds for ExtractIf Debug impls) - rust-lang#140035 (Implement RFC 3503: frontmatters) - rust-lang#140080 (mir-opt: Use one MirPatch in MatchBranchSimplification) - rust-lang#140115 (mir-opt: execute MatchBranchSimplification after GVN) - rust-lang#140357 (bypass linker configuration and cross target check on `x check`) - rust-lang#140374 (Resolve instance for SymFn in global/naked asm) - rust-lang#140393 (std: get rid of `sys_common::process`) - rust-lang#140532 (Fix RustAnalyzer discovery of rustc's `stable_mir` crate) - rust-lang#140559 (Removing rustc_type_ir in the rustc_infer codebase) - rust-lang#140636 (implement `PanicTracker` to track `t` panics) - rust-lang#140661 (Make `-Zfixed-x18` into a target modifier) r? `@ghost` `@rustbot` modify labels: rollup
…s, r=jieyouxu,wesleywiser Implement RFC 3503: frontmatters Tracking issue: rust-lang#136889 Supercedes rust-lang#137193. This implements [RFC 3503](https://github.com/rust-lang/rfcs/blob/master/text/3503-frontmatter.md). This might break rust-analyzer. Will look into how to fix that. Suggestions welcome for how to improve diagnostics.
Rollup of 12 pull requests Successful merges: - rust-lang#139550 (Fix `-Zremap-path-scope` rmeta handling) - rust-lang#139773 (Implement `Iterator::last` for `vec::IntoIter`) - rust-lang#140035 (Implement RFC 3503: frontmatters) - rust-lang#140176 (Fix linking statics on Arm64EC) - rust-lang#140251 (coverage-dump: Resolve global file IDs to filenames) - rust-lang#140393 (std: get rid of `sys_common::process`) - rust-lang#140532 (Fix RustAnalyzer discovery of rustc's `stable_mir` crate) - rust-lang#140598 (Steer docs to `utf8_chunks` and `Iterator::take`) - rust-lang#140634 (Use more accurate ELF flags on MIPS) - rust-lang#140673 (Clean rustdoc tests folder) - rust-lang#140678 (Be a bit more relaxed about not yet constrained infer vars in closure upvar analysis) - rust-lang#140687 (Update mdbook to 0.4.49) r? `@ghost` `@rustbot` modify labels: rollup
Rollup of 12 pull requests Successful merges: - rust-lang#139550 (Fix `-Zremap-path-scope` rmeta handling) - rust-lang#139764 (Consistent trait bounds for ExtractIf Debug impls) - rust-lang#139773 (Implement `Iterator::last` for `vec::IntoIter`) - rust-lang#140035 (Implement RFC 3503: frontmatters) - rust-lang#140251 (coverage-dump: Resolve global file IDs to filenames) - rust-lang#140393 (std: get rid of `sys_common::process`) - rust-lang#140532 (Fix RustAnalyzer discovery of rustc's `stable_mir` crate) - rust-lang#140598 (Steer docs to `utf8_chunks` and `Iterator::take`) - rust-lang#140634 (Use more accurate ELF flags on MIPS) - rust-lang#140673 (Clean rustdoc tests folder) - rust-lang#140678 (Be a bit more relaxed about not yet constrained infer vars in closure upvar analysis) - rust-lang#140687 (Update mdbook to 0.4.49) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#140035 - fee1-dead-contrib:push-oszwkkvmpkks, r=jieyouxu,wesleywiser Implement RFC 3503: frontmatters Tracking issue: rust-lang#136889 Supercedes rust-lang#137193. This implements [RFC 3503](https://github.com/rust-lang/rfcs/blob/master/text/3503-frontmatter.md). This might break rust-analyzer. Will look into how to fix that. Suggestions welcome for how to improve diagnostics.
Tracking issue: #136889
Supercedes #137193. This implements RFC 3503.
This might break rust-analyzer. Will look into how to fix that.
Suggestions welcome for how to improve diagnostics.