Incorrect indentation on multi-line single generic bound

[calebcartwright]

We have a bug in the generic bounds formatting code that does not properly account for the case where there's a single bound that itself has to be formatted across multiple lines.

Input

pub trait PrettyPrinter<'tcx>:
    Printer<
        'tcx,
        Error = fmt::Error,
        Path = Self,
        Region = Self,
        Type = Self,
        DynExistential = Self,
        Const = Self,
    >
    {
         //
    }

Output

pub trait PrettyPrinter<'tcx>:
    Printer<
    'tcx,
    Error = fmt::Error,
    Path = Self,
    Region = Self,
    Type = Self,
    DynExistential = Self,
    Const = Self,
>
{
    //
}

Expected output
The same indentation as if there were 2+ bounds

pub trait PrettyPrinter<'tcx>:
    Printer<
        'tcx,
        Error = fmt::Error,
        Path = Self,
        Region = Self,
        Type = Self,
        DynExistential = Self,
        Const = Self,
    >
{
    //
}

// what it would look like with an extra bound:
pub trait PrettyPrinter<'tcx>:
    Printer<
        'tcx,
        Error = fmt::Error,
        Path = Self,
        Region = Self,
        Type = Self,
        DynExistential = Self,
        Const = Self,
    > + fmt::Write
{
    //
}

Meta

• rustfmt version: all (latest 1.x version as of 1.4.36 and 2.0-rc on master branch
• From where did you install rustfmt?: any

The problematic code can be found here:

rustfmt/src/formatting/types.rs

Lines 1000 to 1007 in d84a32c

	if !force_newline
	&& items.len() > 1
	&& (result.0.contains('\n') || result.0.len() > shape.width)
	{
	join_bounds_inner(context, shape, items, need_indent, true)
	} else {
	Some(result.0)
	}

The issue is an incorrect assumption that if there's just one bound, or multiple bounds but which were able to be formatted on a single line without exceeding the max width limit, then we'll be formatting the bound(s) on a single line and there's no need to reformat the bounds with the proper indentation. However, as noted above this fails to catch the multi-lined single bound case so this check will need to be updated to also account for that case.

[davidBar-On]

Based on the guidelines in the description I tried the "naive" change of just deleting the && items.len() > 1 condition from the if statement, and running the tests with this change. The tests failed on two types of errors, where at least one of them makes the scope of the required change unclear.

1st test error type: when the bound name is in the same line of the preceding code. E.g:

rustfmt/tests/target/issue-3651.rs

Lines 1 to 7 in 257311c

	fn f() -> Box<
	dyn FnMut() -> Thing<
	WithType = LongItemName,
	Error = LONGLONGLONGLONGLONGONGEvenLongerErrorNameLongerLonger,
	>,
	> {
	}

However, when fmt::Write bound is added, using current version of rusfmt (without the code change), indentation is changed to:

fn f() -> Box<
    dyn FnMut() -> Thing<
            WithType = LongItemName,
            Error = LONGLONGLONGLONGLONGONGEvenLongerErrorNameLongerLonger,
        > + fmt::Write,
> {
}

With the rustfmt code change, both cases are indented the same. Should indentation be the same with or without adding the fmt::Write?, i.e. should this case be included in the scope of the rustfmt change? Except for backward compatibility, it doesn't seem right that adding the second bound impacts the indentation of the first one.

2nd test error type: function in where clause. E.g.:

rustfmt/tests/target/issue-3508.rs

Lines 1 to 7 in 257311c

	fn foo<F>(foo2: F)
	where
	F: Fn(
	// this comment is deleted
	),
	{
	}

With the rustfmt change, indentation is changed to:

fn foo<F>(foo2: F)
where
    F: Fn(
            // this comment is deleted
        )
{
}

The same indentation is applied with current rustfmt (without the code change) when + fmt::Write is added after the Fn closing ). Therefore, it is also not clear whether this case should be included in the scope of the rustfmt change.

[calebcartwright]

So there's two things to this, the existing behavior with extra indentation that's added when adding a second bound (however odd that may be in those snippets 😄) is a separate bug.

However, that doesn't change the fact that the naive change is not viable, as that introduces a new bug. A different approach will be needed to address the bug with the trait bounds described in the issue description, though if the additional bug you've discovered can be addressed at the same time that'd be great too!

[jyn514]

I think this might the same issue, but happy to open a new issue if not - rustfmt removes all indentation from the where clause here, making it inconsistent with the function header. https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=f049e90069a93ab8471f51e0a4f27db5

mod inner {
    fn foo() -> String
    where { // running rustfmt removes all whitespace at the start of the line
        String::new()
    }
}

[davidBar-On]

@jyn514,

I think this might the same issue, but happy to open a new issue if not - rustfmt removes all indentation from the where clause here, .....

I checked and this issue is not fixed by the PR. I assume it is better to open a new issue then try to enhance the PR to also fix this problem.

[davidBar-On]

I think this might the same issue, but happy to open a new issue if not ...

@jyn514, the issue is not resolved by this PR. I evaluated it to make sure it is not related and I have some observations. If you will open a new issues I will discuss there what I found.

[ytmimi]

closing since #5390 was merged