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

extra spacing added to normalised inner comment in a function call #1747

Open
topecongiro opened this issue Jun 24, 2017 · 6 comments
Open
Labels
a-comments bug Panic, non-idempotency, invalid code, etc. only-with-option requires a non-default option value to reproduce p-low

Comments

@topecongiro
Copy link
Contributor

topecongiro commented Jun 24, 2017

E.g.

foo(xxxxxxx, xxxxxxxx /* inline comment */, y /* sup */);
foo(
    xxxxxxx,
    xxxxxxxx, // inline comment
    y, /* sup */
);

when normalize_comments = true.

@topecongiro topecongiro added bug Panic, non-idempotency, invalid code, etc. a-comments only-with-option requires a non-default option value to reproduce labels Jun 24, 2017
@scampi
Copy link
Contributor

scampi commented Apr 8, 2019

Do you have a working example @topecongiro ?
The one from the description gets normalised:

 fn bar() {
-    foo(xxxxxxx, xxxxxxxx /* inline comment */, y /* sup */);
+    foo(
+        xxxxxxx, xxxxxxxx, // inline comment
+        y,        // sup
+    );
 }

@scampi
Copy link
Contributor

scampi commented Apr 8, 2019

Although the spacing before // sup is strange...

@topecongiro
Copy link
Contributor Author

Yeah looks like the original issue is fixed.

@scampi scampi changed the title No fixpoint when rewriting function call with inner comment extra spacing added to normalised inner comment in a function call Apr 10, 2019
@scampi
Copy link
Contributor

scampi commented Apr 10, 2019

@topecongiro I have renamed the issue to better reflect the new problem

@ytmimi
Copy link
Contributor

ytmimi commented Jul 19, 2022

This is still reproducible with the current masterrustfmt 1.5.1-nightly (0cb294f0 2022-07-12) when setting normailze_comments=true

input

fn main() {
    foo(xxxxxxx, xxxxxxxx /* inline comment */, y /* sup */);
}

output

fn main() {
    foo(
        xxxxxxx, xxxxxxxx, // inline comment
        y,        // sup
    );
}

Doing some testing, and forcing try_overflow_last_item last item to return a DefinitiveListTactic::Vertical produces the following output:

fn main() {
    foo(
        xxxxxxx,
        xxxxxxxx, // inline comment
        y,        // sup
    );
}

Just a hunch but it seems like the comment alignment is still trying to align the comments even when the tactic = DefinitiveListTactic::Mixed, so I'm guessing the issue occurs somewhere when handling post comments in write_list.

rustfmt/src/lists.rs

Lines 404 to 504 in a7bf009

// Post-comments
if tactic == DefinitiveListTactic::Horizontal && item.post_comment.is_some() {
let comment = item.post_comment.as_ref().unwrap();
let formatted_comment = rewrite_comment(
comment,
true,
Shape::legacy(formatting.shape.width, Indent::empty()),
formatting.config,
)?;
result.push(' ');
result.push_str(&formatted_comment);
}
if separate && sep_place.is_back() {
result.push_str(formatting.separator);
}
if tactic != DefinitiveListTactic::Horizontal && item.post_comment.is_some() {
let comment = item.post_comment.as_ref().unwrap();
let overhead = last_line_width(&result) + first_line_width(comment.trim());
let rewrite_post_comment = |item_max_width: &mut Option<usize>| {
if item_max_width.is_none() && !last && !inner_item.contains('\n') {
*item_max_width = Some(max_width_of_item_with_post_comment(
&cloned_items,
i,
overhead,
formatting.config.max_width(),
));
}
let overhead = if starts_with_newline(comment) {
0
} else if let Some(max_width) = *item_max_width {
max_width + 2
} else {
// 1 = space between item and comment.
item_last_line_width + 1
};
let width = formatting.shape.width.checked_sub(overhead).unwrap_or(1);
let offset = formatting.shape.indent + overhead;
let comment_shape = Shape::legacy(width, offset);
let block_style = if !formatting.ends_with_newline && last {
true
} else if starts_with_newline(comment) {
false
} else {
comment.trim().contains('\n') || comment.trim().len() > width
};
rewrite_comment(
comment.trim_start(),
block_style,
comment_shape,
formatting.config,
)
};
let mut formatted_comment = rewrite_post_comment(&mut item_max_width)?;
if !starts_with_newline(comment) {
if formatting.align_comments {
let mut comment_alignment =
post_comment_alignment(item_max_width, inner_item.len());
if first_line_width(&formatted_comment)
+ last_line_width(&result)
+ comment_alignment
+ 1
> formatting.config.max_width()
{
item_max_width = None;
formatted_comment = rewrite_post_comment(&mut item_max_width)?;
comment_alignment =
post_comment_alignment(item_max_width, inner_item.len());
}
for _ in 0..=comment_alignment {
result.push(' ');
}
}
// An additional space for the missing trailing separator (or
// if we skipped alignment above).
if !formatting.align_comments
|| (last
&& item_max_width.is_some()
&& !separate
&& !formatting.separator.is_empty())
{
result.push(' ');
}
} else {
result.push('\n');
result.push_str(indent_str);
}
if formatted_comment.contains('\n') {
item_max_width = None;
}
result.push_str(&formatted_comment);
} else {
item_max_width = None;
}

@ytmimi
Copy link
Contributor

ytmimi commented Jul 20, 2022

linking tracking issue for normalize_comments #3350

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-comments bug Panic, non-idempotency, invalid code, etc. only-with-option requires a non-default option value to reproduce p-low
Projects
None yet
Development

No branches or pull requests

3 participants