Skip to content
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

avoid inserting newline between opening curly brace and comment #6265

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions src/parse/session.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use std::ops::Range;
use std::path::Path;
use std::sync::atomic::{AtomicBool, Ordering};

Expand Down Expand Up @@ -237,6 +238,15 @@ impl ParseSess {
}
}

pub(crate) fn line_bounds(&self, pos: BytePos) -> Option<Range<BytePos>> {
let line = self.raw_psess.source_map().lookup_line(pos).ok();

match line {
Some(line_info) => Some(line_info.sf.line_bounds(line_info.line)),
None => None,
}
}

pub(crate) fn line_of_byte_pos(&self, pos: BytePos) -> usize {
self.raw_psess.source_map().lookup_char_pos(pos).line
}
Expand Down
35 changes: 35 additions & 0 deletions src/visitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,41 @@ impl<'b, 'a: 'b> FmtVisitor<'a> {
self.push_str("{");
self.trim_spaces_after_opening_brace(b, inner_attrs);

// Try to detect comments that refer to the block, not the first statement in the block.
if has_braces && self.config.style_edition() >= StyleEdition::Edition2024 {
let block_line_range = self.psess.lookup_line_range(b.span);
if block_line_range.lo != block_line_range.hi {
// Skipping if a single line block
// Make sure there is no code on the first line we have to worry about.
// That would also be ambiguous: what should `if { /* comment */ statement;`
// even do -- should the comment be attached to the `if` or the `statement`?
// The current logic answers this with "statement".
let first_line_contains_stmt = if let Some(first_stmt) = b.stmts.first() {
self.psess.lookup_line_range(first_stmt.span).lo == block_line_range.lo
} else {
false
};
if !first_line_contains_stmt {
let first_line_bounds = self.psess.line_bounds(self.last_pos).unwrap();
let first_line_snip = self
.snippet(mk_sp(self.last_pos, first_line_bounds.end))
.trim();

if contains_comment(first_line_snip) {
if let Ok(comment) =
rewrite_comment(first_line_snip, false, self.shape(), self.config)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SO there's no way to adjust the shape that we pass to rewrite_comment to tell it how much space it actually has available?

How does it usually know how much space it has (e.g. to account for indentation)? I would have thought it looks at the cursor position in the current line in the output, which should always be reliable, but that doesn't seem to be the case.

{
self.push_str(" ");
self.push_str(&comment);
// This line has no statements, so we can jump to the end of it
// (but *before* the newline).
self.last_pos = first_line_bounds.end - BytePos::from_usize(1);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original PR did self.last_pos + BytePos(first_line_snip.len() as u32) here. That led to a bunch of test failures where rustfmt inserts seemingly random characters into previously empty blocks. I think what happens is that if the trim() above removes characters from the start of the string, we'd advance last_pos by too little here, and so the cursor doesn't quite point to the end of the comment -- so the last characters of the comment get duplicated.

I'm not sure what the proper fix for that is. Skipping to the end of the line seemed to make most sense, since we should have processed the entire line.

}
}
}
}
}

// Format inner attributes if available.
if let Some(attrs) = inner_attrs {
self.visit_attrs(attrs, ast::AttrStyle::Inner);
Expand Down
40 changes: 40 additions & 0 deletions tests/source/issue-3255.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
// rustfmt-style_edition: 2024

fn foo(){
if true { // Sample comment
// second-line comment
1
}
}


fn foo(){
if true {
// Sample comment
1
}
}
RalfJung marked this conversation as resolved.
Show resolved Hide resolved

fn foo(){
if true { /* Sample comment */
1
}
}

fn foo(){
if true { /* Sample
comment */
1
}
}
Comment on lines +24 to +29
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably good to add a test case with a multi-line block comment that doesn't have bare lines. Something like this for example:

fn foo(){  
    if true {     /* Sample
                   * another line
                   * another line
                   * end
                   */
        1
    }
}

I noticed that the indentation of the first line was the only one that got updated. Maybe the other's should have as well since they're part of the same block comment? I imagine that might be tougher to get right though. This is the output that I'm currently getting when formatting with this branch:

fn foo() {
    if true { /* Sample
                   * another line
                   * another line
                   * end
                   */
        1
    }
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the entire approach is based on processing just the code from the { to the end of the line, so it doesn't take into account the other lines at all. I have no idea how to fix this.

The alternative originally implemented by #4745 was to not touch /*-style comments at all, but that does not seem better: the result looks nicer but is more likely to change the semantic meaning of the comment.


// This isn't ideal.
fn foo(){
if true { /* Sample
* another line
* another line
* end
*/
1
}
}
39 changes: 39 additions & 0 deletions tests/target/issue-3255.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// rustfmt-style_edition: 2024

fn foo() {
if true { // Sample comment
// second-line comment
1
}
}

fn foo() {
if true {
// Sample comment
1
}
}

fn foo() {
if true { /* Sample comment */
1
}
}

fn foo() {
if true { /* Sample
comment */
1
}
}

// This isn't ideal.
fn foo() {
if true { /* Sample
* another line
* another line
* end
*/
1
}
}
Loading