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

Prevent trailing whitespace in where clause bound predicate #5019

Merged
merged 1 commit into from
Oct 31, 2021
Merged

Prevent trailing whitespace in where clause bound predicate #5019

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 additions & 0 deletions src/expr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1962,6 +1962,9 @@ fn choose_rhs<R: Rewrite>(
has_rhs_comment: bool,
) -> Option<String> {
match orig_rhs {
Some(ref new_str) if new_str.is_empty() => {
return Some(String::new());
}
Comment on lines +1965 to +1967
Copy link
Member

Choose a reason for hiding this comment

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

This is on one of the hottest paths in the codebase, and while there wouldn't be any significant performance concerns with the proposed diff in this location, we should strive to avoid having to make fixes at this level due to the higher potential for unintended knock on effects.

Ideally, we'd be able to correct the issue upstream instead of having to try to retcon it here. Do you think that will be feasible in this case?

Copy link
Contributor Author

@ytmimi ytmimi Oct 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Ideally, we'd be able to correct the issue upstream instead of having to try to retcon it here. Do you think that will be feasible in this case?

I think we could address it a little further upstream. Before I pitch some ideas I want to lay out my understanding of the code flow.

Code Flow

In this case when we call rewrite on ast::WherePredicate, we hit the first match arm of ast::WherePredicate::BoundPredicate, which ultimately calls expr::rewrite_assign_rhs

rustfmt/src/types.rs

Lines 412 to 433 in 599b2fd

impl Rewrite for ast::WherePredicate {
fn rewrite(&self, context: &RewriteContext<'_>, shape: Shape) -> Option<String> {
// FIXME: dead spans?
let result = match *self {
ast::WherePredicate::BoundPredicate(ast::WhereBoundPredicate {
ref bound_generic_params,
ref bounded_ty,
ref bounds,
..
}) => {
let type_str = bounded_ty.rewrite(context, shape)?;
let colon = type_bound_colon(context).trim_end();
let lhs = if let Some(lifetime_str) =
rewrite_lifetime_param(context, shape, bound_generic_params)
{
format!("for<{}> {}{}", lifetime_str, type_str, colon)
} else {
format!("{}{}", type_str, colon)
};
rewrite_assign_rhs(context, lhs, bounds, shape)?
}

As it's currently implemented, expr::rewrite_assign_rhs just calls expr::rewrite_assign_rhs_with, which is where the lhs and rhs get concatenated (linked below for reference).

rustfmt/src/expr.rs

Lines 1918 to 1928 in 599b2fd

pub(crate) fn rewrite_assign_rhs_with<S: Into<String>, R: Rewrite>(
context: &RewriteContext<'_>,
lhs: S,
ex: &R,
shape: Shape,
rhs_tactics: RhsTactics,
) -> Option<String> {
let lhs = lhs.into();
let rhs = rewrite_assign_rhs_expr(context, &lhs, ex, shape, rhs_tactics)?;
Some(lhs + &rhs)
}

Ideas

  1. We could add a conditional check in expr::rewrite_assign_rhs_with to only concatenate the lhs and rhs if the rhs isn't just whitespace.
  • This change is still on the hot path, and really only patches the underlying issue introduced by expr::choose_rhs returning " " when there is no rhs, but it does tackle the problem slightly further upstream.
  1. Create a new function expr::rewrite_assign_potentially_empty_rhs, (open to name suggestion) that will be called instead of expr::rewrite_assign_rhs when rewriting ast::WherePredicate::BoundPredicate.
  • We'll still leverage code that's on the hot path, but we'll only have to perform the relevant conditional checks in cases where we can't be sure that there will always be a rhs. (I think I'm leaning towards this option)
  1. Go with the current solution that makes changes directly to expr::choose_rhs

Definitely open to your thoughts and if you have other suggestions for where to make this fix!

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the excellent, detailed write up. After reading and reflecting I think you were on the right path to begin with. I was originally thinking we'd check if the bounds where empty at the original callsite and skip trying to join a lhs with a non-existent rhs to begin with. However, doing that would likely start to bleed through the encapsulation of the Rewrite impl for GenericBounds and/or require an upfront rewrite call of the bounds node to check for the empty string.

As such I think I'm leaning pretty heavily towards num 3/your original proposal. Realize this goes slightly past the literal issue reports but could you also add some cases with the snippet from 5012 that are configured to do comma inserts too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

option 3 it is! I'll add those additional test cases and request a follow up review when they've been added!

Some(ref new_str)
if !new_str.contains('\n') && unicode_str_width(new_str) <= shape.width =>
{
Expand Down
8 changes: 8 additions & 0 deletions tests/target/issue-5012/trailing_comma_always.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// rustfmt-trailing_comma: Always

pub struct Matrix<T, const R: usize, const C: usize,>
where
[T; R * C]:,
{
contents: [T; R * C],
}
8 changes: 8 additions & 0 deletions tests/target/issue-5012/trailing_comma_never.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// rustfmt-trailing_comma: Never

pub struct Matrix<T, const R: usize, const C: usize>
where
[T; R * C]:
{
contents: [T; R * C]
}
4 changes: 4 additions & 0 deletions tests/target/issue_4850.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
impl ThisIsALongStructNameToPushTheWhereToWrapLolololol where
[(); this_is_a_long_const_function_name()]:
{
}