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

Rustfmt inserts a made-up impl Trait definition into types #5086

Closed
dtolnay opened this issue Nov 15, 2021 · 2 comments · Fixed by #5093
Closed

Rustfmt inserts a made-up impl Trait definition into types #5086

dtolnay opened this issue Nov 15, 2021 · 2 comments · Fixed by #5093

Comments

@dtolnay
Copy link
Member

dtolnay commented Nov 15, 2021

As of current master, rustfmt applies the following incorrect diff to this valid Rust code:

  #[cfg(any())]
- type Type: Bound;
+ type Type: Bound = impl Bound;

To reproduce: cargo run --bin rustfmt repro.rs

@calebcartwright
Copy link
Member

Thanks for the report. Having a major case of déjà vu with this one

@ytmimi
Copy link
Contributor

ytmimi commented Nov 18, 2021

Summary

Was doing a little digging into this.

TLDR; I think the issue is caused by using the bounds for both the generic_bounds_opt and as the OpaqueType used for the rhs when calling items::rewrite_ty.

Bug Details

When calling items::rewrite_type_alias we hit the first match_arm:

rustfmt/src/items.rs

Lines 1523 to 1527 in eee8f04

match (visitor_kind, ty_opt) {
(Item(_), None) => {
let op_ty = OpaqueType { bounds };
rewrite_ty(rw_info, Some(bounds), Some(&op_ty), vis)
}

After we add the generic bounds to the output result (code responsible for that added bellow) we end up with the string type Type: Bound

rustfmt/src/items.rs

Lines 1573 to 1580 in eee8f04

if let Some(bounds) = generic_bounds_opt {
if !bounds.is_empty() {
// 2 = `: `
let shape = Shape::indented(indent, context.config).offset_left(result.len() + 2)?;
let type_bounds = bounds.rewrite(context, shape).map(|s| format!(": {}", s))?;
result.push_str(&type_bounds);
}
}

Because we have a rhs we end up adding a = to the end of the result string (code bellow) so now we have
type Type: Bound =

rustfmt/src/items.rs

Lines 1615 to 1636 in eee8f04

let lhs = match comment_span {
Some(comment_span)
if contains_comment(context.snippet_provider.span_to_snippet(comment_span)?) =>
{
let comment_shape = if has_where {
Shape::indented(indent, context.config)
} else {
Shape::indented(indent, context.config)
.block_left(context.config.tab_spaces())?
};
combine_strs_with_missing_comments(
context,
result.trim_end(),
"=",
comment_span,
comment_shape,
true,
)?
}
_ => format!("{}=", result),
};

And lastly because of how OpaqueType::rewrite is implemented (code bellow) we end up adding the impl Bound. before ultimately returning type Type: Bound = impl Bound; from items::rewrite_ty.

rustfmt/src/items.rs

Lines 1874 to 1881 in eee8f04

impl<'a> Rewrite for OpaqueType<'a> {
fn rewrite(&self, context: &RewriteContext<'_>, shape: Shape) -> Option<String> {
let shape = shape.offset_left(5)?; // `impl `
self.bounds
.rewrite(context, shape)
.map(|s| format!("impl {}", s))
}
}

Proposed Solution

After going through the code it seems to me that passing None as the rhs argument of items::rewrite_ty, would fix the issue, and locally it seems to have done the trick. I'm going to open a PR, but please let me know if I'm overlooking anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants