fix(embedded): Resolve multiple bugs in frontmatter parser #15573
Conversation
I left out those dealing with proc-macros or `include`
We could say that we delegate this to rustc but if they add support for multiple frontmatters, we need to update to be able to know which we should read, so its better to error on our side.
|
r? @weihanglo rustbot has assigned @weihanglo. Use |
rustbot
added
the
S-waiting-on-review
There was a problem hiding this comment.
I don't quite follow the conversion in rust-lang/rust#137193 or the other merged PR. How much effort should Cargo provide to parse the frontmatter? Do we still want to delgate to rustc for some diagnostics? Or I just need to make sure this parser look correct on its own, without looking at rustc part?
There was a problem hiding this comment.
Any error we produce will fire before rustc ever gets a chance to report one, so we should aim for rustc-like error messages (yes, we are not there yet). However, we can't catch all cases and have to defer to rustc for those.
Alternatively, we could be really sloppy and ignore, rather than error. The downside is an error will only be produced if rustc is invoked. I'd prefer to not go this route.
| @@ -362,6 +367,7 @@ content: "\n//@ check-pass\n\n// check that frontmatter blocks can have tokens t | |||
|
|
|||
| #[test] | |||
| fn rustc_frontmatter_whitespace_1() { | |||
| // Deferred to rustc since this requires knowledge of Rust grammar | |||
There was a problem hiding this comment.
Curious why this requires Rust grammar. Can we just match the behavior of “invalid preceding whitespace for frontmatter opening” here?
There was a problem hiding this comment.
Currently, the frontmatter specification does not say we can detect this kind of error case and would require changing that or us having more knowledge of rustc grammar in case this could be part of something valid in rustc one day. This is theoretical right now and I doubt we would design ourselves into a hole like this. We also ship with rustc and can change with them. However, if we want our parser to serve as an example or even pull it out, then that puts more weight on this.
What does this PR try to resolve?
This pulls in the tests from rust-lang/rust#140035 and ensures we pass them, including switching our parser to be more like the one from rust-lang/rust#137193.
I've added comments specifying why our test results differ from rustcs.
This is part of #12207
How should we test and review this PR?
Additional information
We still have work to do to improve the quality of our error messages