Fix misidentification of indented comments#6617
Fix misidentification of indented comments#6617matthewhughes934 wants to merge 1 commit intorust-lang:mainfrom
Conversation
jieyouxu
added
S-waiting-on-review
The comment style detection (i.e. `comment_style`) checks the start of a
line to determine the style, so it will misidentify lines starting with
spaces, e.g. ` //# some content` would be identified as
`CommentStyle::DoubleSlash` and not `CommentStyle::Custom("//# ")`
resulting in it determining there to be a comment of `""`, which it then
indents and appends a `\n` before writing back the original comment.
Fix this by trimming the line before trying to identify its style.
Fixes: rust-lang#6612
matthewhughes934
force-pushed
the
fix-formatting-comments-with-hidden-code
branch
from
326a4c1 to
cada2ef
Compare
|
This PR was rebased onto a different main 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. |
There was a problem hiding this comment.
Question [IDEMPOTENCE 1/2]: is this a "rustfmt doesn't change stable formatting" test? If so, can you remove the source/ copy and leave only the target/ copy (as idempotence check is done against the target/ copy)?
There was a problem hiding this comment.
👍 thanks, I wasn't aware of a standalone file in target meant it is run as an idempotentcy test, I'll double check that's the case and update accordingly
There was a problem hiding this comment.
I did some more trial and error myself, I missed that there is specifically a case where you would want both a source/ and target/ copy, which is to ensure idempotence. That is, having both source/ and target/ even if identical helps you catch the case where the target copy is already in canonical form, but that it make take more than 1 rustfmt runs to get from the source copy to target copy.
When there's only a target/ copy, the test is only good for "the format is indeed canonical" and implies no guarantees on idempotency actually, sorry.
(IOW, leaving the source copies as-is is probably good, because other stuff might have strange interations with these examples.)
I'll update the contributing docs because this wasn't super obvious to me at first, but became obvious on hindsight
EDIT: #6821
| config: &Config, | ||
| is_doc_comment: bool, | ||
| ) -> RewriteResult { | ||
| let style = comment_style(orig, false); |
There was a problem hiding this comment.
Suggestion: can you leave a brief comment here with an example, i.e.
// Account for leading whitespace, e.g. ` //#`There was a problem hiding this comment.
Suggestion: can you leave a brief comment here with an example, i.e.
// Account for leading whitespace, e.g. ` //#`
🤔 let me actually think over this change a bit more: I'm wondering if this is too naive and comments with different indentation levels are now accidentally match on the same style.
There was a problem hiding this comment.
Question [IDEMPOTENCE 2/2]: ditto on the "format as expected"
There was a problem hiding this comment.
Suggestion: can you also add coverage for:
- Doc comment but without the code fenses, i.e.
/// println!("1"); // comment
/// # println!("2")
struct S;- Code block in doc comment but no space between
///and#, i.e.
/// ```
/// println!("1"); // comment
///# println!("2")
/// ```
struct S;/// ```
/// println!("1"); // comment
///#println!("2")
/// ```
struct S;
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
|
I just discovered #5392 which is basically doing the same as this PR (but that one has a bit more context). Can we merge that one instead? |
3 participants
The comment style detection (i.e.
comment_style) checks the start of a line to determine the style, so it will misidentify lines starting with spaces, e.g.//# some contentwould be identified asCommentStyle::DoubleSlashand notCommentStyle::Custom("//# ")resulting in it determining there to be a comment of"", which it then indents and appends a\nbefore writing back the original comment.Fix this by trimming the line before trying to identify its style.
Fixes: #6612