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 eats dyn_star syntax #5542

Closed
Tracked by #102425
mejrs opened this issue Sep 15, 2022 · 5 comments · Fixed by #5552
Closed
Tracked by #102425

rustfmt eats dyn_star syntax #5542

mejrs opened this issue Sep 15, 2022 · 5 comments · Fixed by #5552
Labels
bug Panic, non-idempotency, invalid code, etc. p-low

Comments

@mejrs
Copy link

mejrs commented Sep 15, 2022

Playground

Given:

#![feature(dyn_star)]
#![allow(incomplete_features)]

use core::fmt::Debug;

fn main() {
    let i = 42;
    let dyn_i = i as dyn* Debug;
    dbg!(dyn_i);
}

This is formatted to:

#![feature(dyn_star)]
#![allow(incomplete_features)]

use core::fmt::Debug;

fn main() {
    let i = 42;
    let dyn_i = i as Debug;
    dbg!(dyn_i);
}

After formatting, this code no longer compiles.

@ytmimi
Copy link
Contributor

ytmimi commented Sep 21, 2022

Thanks for the report. Any chance you could link me to more info about #![feature(dyn_star)]?

I can confirm that I'm able to reproduce the issue on the playground, however when running with the latest master (ef91154) this is the output that I get:

#![feature(dyn_star)]
#![allow(incomplete_features)]

use core::fmt::Debug;

fn main() {
    let i = 42;
    let dyn_i = i as dyn * Debug;
    dbg!(dyn_i);
}

@ytmimi ytmimi added bug Panic, non-idempotency, invalid code, etc. p-low labels Sep 21, 2022
@mejrs
Copy link
Author

mejrs commented Sep 21, 2022

Any chance you could link me to more info about #![feature(dyn_star)]?

It was introduced with rust-lang/rust#101212

however when running with the latest master (ef91154) this is the output that I get

Ah if it's fixed already that's great. 🙂

@ytmimi
Copy link
Contributor

ytmimi commented Sep 22, 2022

Thanks for linking the dyn* PR. It looks like that PR didn't make changes to handle dyn* formatting in rustfmt, so I don't think it's fixed yet. Also the dyn* Debug from your original snippet is getting converted to dyn * Debug. Not sure if things still compile when formatted like that, but it looks like there's still work to do on the rustfmt side to support dyn*.

@eholk
Copy link
Contributor

eholk commented Sep 29, 2022

dyn* Debug and dyn * Debug will both parse and compile fine, but I prefer the dyn* version.

@ytmimi
Copy link
Contributor

ytmimi commented Sep 29, 2022

I took a look at this again. The latest rustfmt master is using nightly-2022-08-06, which doesn't know about the new ast::TraitObjectSyntax::DynStar variant. In the example I mentioned above #5542 (comment) it turns out the current master parses i as dyn* Debug as a binary multiplication where the lhs is i as dyn and the rhs is Debug. At least that's my understanding of why i as dyn* Debug was formatted as i as dyn * Debug.

In order to resolve this we'll need to do a subtree sync with r-l/rust. @calebcartwright usually handles those, and from what I understand we're waiting for a few PRs to land in r-l/rust before performing the sync.

Changes will ultimately need to be made to the ast::TyKind::TraitObject arm in the Rewrite impl for ast::Ty to properly handle the ast::TraitObjectSyntax::DynStar variant added in rust-lang/rust#101212 .

rustfmt/src/types.rs

Lines 662 to 682 in ef91154

impl Rewrite for ast::Ty {
fn rewrite(&self, context: &RewriteContext<'_>, shape: Shape) -> Option<String> {
match self.kind {
ast::TyKind::TraitObject(ref bounds, tobj_syntax) => {
// we have to consider 'dyn' keyword is used or not!!!
let is_dyn = tobj_syntax == ast::TraitObjectSyntax::Dyn;
// 4 is length of 'dyn '
let shape = if is_dyn { shape.offset_left(4)? } else { shape };
let mut res = bounds.rewrite(context, shape)?;
// We may have falsely removed a trailing ` ` inside macro call.
if context.inside_macro() && bounds.len() == 1 {
if context.snippet(self.span).ends_with(' ') && !res.ends_with(' ') {
res.push(' ');
}
}
if is_dyn {
Some(format!("dyn {}", res))
} else {
Some(res)
}
}

The fix should be pretty straightforward once we've done the sync.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Panic, non-idempotency, invalid code, etc. p-low
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants