-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
fix(parser): Disallow CR in frontmatter #149823
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
Conversation
|
Given that text-direction-codepoint-in-literal and text-direction-codepoint-in-comment are lints, I do think it makes sense for this and the similar error in raw strings to be a lint as well. I doubt that anyone is going to allow it, but I still think it makes sense to not be a hard error. That said, I don't think switching this from a hard error to a lint should be a blocker. Let's go ahead with the simple version of this change to unblock stabilization. |
|
@joshtriplett this only prevents the use of CR and not the text direction code points. However, this is making me more against doing CR in the first place if CR as an error is just a "simple version" of what we actually want. If these should be lints, they should be lints in the tool that owns processing the frontmatter. If we have them as lints in rustc, we then have both tools running those lints as they likely have a format outside of the frontmatter that should also lint and that would be generic code across the files. Having both lint can lead to a bad user experience including:
If we error on CR now and then remove it because tools should do it, that is a subtle change that could easily get lost in communicating back out to those tools. |
|
I've moved this to a draft until this is sorted out so we don't accidentally merge this |
|
r? me |
T-lang came back on the stabilization PR asking for CR to be disallowed to leave room for all stray CRs to be rejected in the future. At that point, the test can remain but the implementation can be removed. If that plan does not go through, we'll need to re-evaluate - whether this is more lint-like and should defer to the calling tool that is managing the frontmatter - how much Rust should treat the frontmatter as Rust and apply the same grammar restrictions of "no stray CR" (like raw string literals)
54dbfe1 to
52d4ef1
Compare
|
This was discussed on the stabilization PR starting at #148051 (comment). Between there and the T-lang meeting discussions, a new reason for rejecting CRs came up: potentially changing CRLF normalization to reject all stray CRs. This is now viewed as a stop gap until then and not a final statement on what should be done if that CRLF normalization change is not made. |
according to this, would it be reasonable to place a FIXME in code for future? like "remove it once CRLF normalization change been made" |
|
Personally, I'm against FIXMEs. They represent a hidden, stale backlog. But if the local way of doings things is to have one I can add it. |
|
fine, otherwise everything looks good @bors r+ rollup |
Rollup of 5 pull requests Successful merges: - #151775 (Portable SIMD subtree update) - #151488 (Tweak E0599 to consolidate unsatisfied trait bound messages) - #149823 (fix(parser): Disallow CR in frontmatter ) - #151475 (add foregin type tests for issue 64458) - #151657 (Cleanup of `#[derive(Diagnostic)]` attribute parsers)
Rollup merge of #149823 - epage:f, r=Kivooeo fix(parser): Disallow CR in frontmatter T-lang came back on the stabilization PR (#148051) asking for CR to be disallowed to leave room for all stray CRs to be rejected in the future. At that point, the test can remain but the implementation can be removed. If that plan does not go through, we'll need to re-evaluate - whether this is more lint-like and should defer to the calling tool that is managing the frontmatter - how much Rust should treat the frontmatter as Rust and apply the same grammar restrictions of "no stray CR" (like raw string literals) Part of #136889
Rollup of 5 pull requests Successful merges: - rust-lang/rust#151775 (Portable SIMD subtree update) - rust-lang/rust#151488 (Tweak E0599 to consolidate unsatisfied trait bound messages) - rust-lang/rust#149823 (fix(parser): Disallow CR in frontmatter ) - rust-lang/rust#151475 (add foregin type tests for issue 64458) - rust-lang/rust#151657 (Cleanup of `#[derive(Diagnostic)]` attribute parsers)
T-lang came back on the stabilization PR (#148051) asking for CR to be disallowed
to leave room for all stray CRs to be rejected in the future.
At that point, the test can remain but the implementation can be
removed.
If that plan does not go through, we'll need to re-evaluate
that is managing the frontmatter
grammar restrictions of "no stray CR" (like raw string literals)
Part of #136889