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.

Sign up for GitHub

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

Jump to bottom

Failure to split a line exceeding the maximum width #6202

Open
Jarcho opened this issue Jun 19, 2024 · 5 comments · May be fixed by #6224
Open

Failure to split a line exceeding the maximum width #6202

Jarcho opened this issue Jun 19, 2024 · 5 comments · May be fixed by #6224
Assignees
@CalebLItalien
Labels
e-max width error[internal]: line formatted, but exceeded maximum width good first issue Issues up for grabs, also good candidates for new rustfmt contributors

Comments

@Jarcho
Copy link

Jarcho commented Jun 19, 2024

The following code fails to format line formatted, but exceeded maximum width:

impl EarlyLintPass for NeedlessContinue {
    fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &Expr) {
        if let ExprKind::Loop(body, label, ..) | ExprKind::While(_, body, label) | ExprKind::ForLoop { body, label, .. } =
            &expr.kind
            && !in_external_macro(cx.sess, expr.span)
        {
            check_final_block_stmt(cx, body, label, expr.span.ctxt());
        }
    }
}

The line causing the error is the if let pattern (122 characters). Shortening the line by one character causes the same issue. Adding one character to the line causes it to be formatted correctly.

config:

max_width = 120
comment_width = 100
match_block_trailing_comma = true
wrap_comments = true
edition = "2021"
error_on_line_overflow = true
imports_granularity = "Module"
version = "Two"
ignore = ["tests/ui/crashes/ice-10912.rs"]

version: rustfmt 1.7.0-nightly (8337ba91 2024-06-12)
@ytmimi
Copy link
Contributor

ytmimi commented Jun 19, 2024

Thanks for the report! I think the issue here is that we're not taking the trailing = into account when rewriting the pattern. I think the issue is in rewrite_let

rustfmt/src/expr.rs

Lines 1852 to 1884 in 3ffd7d4

fn rewrite_let(
context: &RewriteContext<'_>,
shape: Shape,
pat: &ast::Pat,
expr: &ast::Expr,
) -> Option<String> {
let mut result = "let ".to_owned();
// TODO(ytmimi) comments could appear between `let` and the `pat`
// 4 = "let ".len()
let pat_shape = shape.offset_left(4)?;
let pat_str = pat.rewrite(context, pat_shape)?;
result.push_str(&pat_str);
// TODO(ytmimi) comments could appear between `pat` and `=`
result.push_str(" =");
let comments_lo = context
.snippet_provider
.span_after(expr.span.with_lo(pat.span.hi()), "=");
let comments_span = mk_sp(comments_lo, expr.span.lo());
rewrite_assign_rhs_with_comments(
context,
result,
expr,
shape,
&RhsAssignKind::Expr(&expr.kind, expr.span),
RhsTactics::Default,
comments_span,
true,
)
}

and I thinks something like this would resolve the issue:

    if context.config.version() == Version::Two {
        // 2 for the length of " ="
        pat_shape = pat_shape.sub_width(2)?
    }

@ytmimi ytmimi added good first issue Issues up for grabs, also good candidates for new rustfmt contributors e-max width error[internal]: line formatted, but exceeded maximum width labels Jun 19, 2024
@CalebLItalien
Copy link

Can I be assigned to this issue? I haven't contributed to rust-lang before, so I'm looking for a good first issue.

@ytmimi
Copy link
Contributor

ytmimi commented Jun 20, 2024

@CalebLItalien I just assigned you. Just a tip, in the future you can comment @rustbot claim to assign yourself if you see an issue you'd like to work on!

@CalebLItalien
Copy link

CalebLItalien commented Jun 21, 2024 •
edited
Loading

Thanks for letting me know! I see that I can post questions I have on this issue here and to the Discord. Which would you recommend? @ytmimi

@ytmimi
Copy link
Contributor

ytmimi commented Jun 21, 2024 •
edited
Loading

The rustfmt team doesn't use Discord that much these days. You can reach out here, though the best place to ask about rustfmt development related questions is the t-rustfmt channel on Zulip

CalebLItalien added a commit to CalebLItalien/rustfmt that referenced this issue Jun 30, 2024
@CalebLItalien
Source code for example from issue rust-lang#6202
0ebd736
CalebLItalien added a commit to CalebLItalien/rustfmt that referenced this issue Jun 30, 2024
@CalebLItalien
Target code from example issue rust-lang#6202
c014a68
@CalebLItalien CalebLItalien linked a pull request Jun 30, 2024 that will close this issue
Open
CalebLItalien added a commit to CalebLItalien/rustfmt that referenced this issue Jul 1, 2024
@CalebLItalien
Source code for example from issue rust-lang#6202
9d0428c 
Target code from example issue rust-lang#6202

Corrected indentation and use of spacing for resriting let statements

Modified resrite_let to account for trailing ' ='

Reduced configs down to what was necessary to reproduce issue

Reduced configs down to what was necessary to reproduce issue

Documented reason for modifying pat_shape to account for ' ='

Moved comment to match rest of codebase's style

Fixed issue with accounting for trailing ' =' in let statements
CalebLItalien added a commit to CalebLItalien/rustfmt that referenced this issue Jul 1, 2024
@CalebLItalien
Source code for example from issue rust-lang#6202
83af25f 
Target code from example issue rust-lang#6202

Corrected indentation and use of spacing for resriting let statements

Modified resrite_let to account for trailing ' ='

Reduced configs down to what was necessary to reproduce issue

Reduced configs down to what was necessary to reproduce issue

Documented reason for modifying pat_shape to account for ' ='

Moved comment to match rest of codebase's style

Fixed issue with accounting for trailing ' =' in let statements

Removed trailing space

Fixed issue 6202
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@CalebLItalien CalebLItalien

Labels
e-max width error[internal]: line formatted, but exceeded maximum width good first issue Issues up for grabs, also good candidates for new rustfmt contributors
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Issue 6202 CalebLItalien/rustfmt
3 participants
@Jarcho @ytmimi @CalebLItalien