Handle dyn* syntax when rewriting ast::TyKind::TraitObject#5552
Handle dyn* syntax when rewriting ast::TyKind::TraitObject#5552calebcartwright merged 1 commit intorust-lang:masterfrom
dyn* syntax when rewriting ast::TyKind::TraitObject#5552Conversation
|
PR on hold until we do a subtree sync and bump the nighty compiler version to one that supports parsing |
ytmimi
added
S-blocked
|
The other thing that's needed is input from the style team relative to how these should be formatted, or at least how we think they should be. Wheels are in motion there |
calebcartwright
left a comment
calebcartwright
left a comment
There was a problem hiding this comment.
Tentative approval as while I believe this is the direction we'll go, it will still need some level of confirmation first
src/types.rs
Outdated
| let shape = if is_dyn { shape.offset_left(4)? } else { shape }; | ||
| let shape = match tobj_syntax { | ||
| ast::TraitObjectSyntax::Dyn => shape.offset_left(4)?, // 4 is offset 'dyn ' | ||
| ast::TraitObjectSyntax::DynStar => shape.offset_left(5)?, // 5 is offset 'dyn* ' |
There was a problem hiding this comment.
hmmm now that I'm thinking about it would it just be better to return None if we get to a DynStart since its experimental syntax?
|
Let's do a simple update to ensure rustfmt stops butchering the syntax altogether, but we should hold off on applying formatting for now. This was a topic in recent t-style discussions, but style/formatting rules are being deferred due to other priorities and focus areas |
compiler-errors
left a comment
compiler-errors
left a comment
There was a problem hiding this comment.
I'm gonna bump this because I almost actually just put up my own PR that duplicated this 🤣.
Per nightly style policy, I'm pretty sure that rustfmt can now feel free to begin formatting dyn* more than just ignoring it altogether. Speaking as an individual on T-style, I think treating dyn* as a single token is what we want to do here anyways.
src/types.rs
Outdated
| match tobj_syntax { | ||
| ast::TraitObjectSyntax::Dyn => Some(format!("dyn {}", res)), | ||
| ast::TraitObjectSyntax::DynStar => Some(format!("dyn* {}", res)), | ||
| ast::TraitObjectSyntax::None => Some(res), |
There was a problem hiding this comment.
My only nit is that this could be merged into a single match with above,
something like:
let (shape, prefix) = match tobj_syntax {
ast::TraitObjectSyntax::Dyn => (shape.offset_left(4)?, "dyn "),
ast::TraitObjectSyntax::DynStar => (shape.offset_left(5)?, "dyn* "),
ast::TraitObjectSyntax::None => (shape, ""),
};There was a problem hiding this comment.
I'd be happy to make this change since it simplifies the code!
Resolves 5542 Prior to rust-lang/rust#101212 the `ast::TraitObjectSyntax` enum only had two variants `Dyn` and `None`. The PR that introduced the `dyn*` syntax added a new variant `DynStar`, but did not update the formatting rules to account for the new variant. Now the new `DynStar` variant is properly handled and is no longer removed by rustfmt.
|
@compiler-errors I made your suggest change. @calebcartwright does your earlier review still apply? |
calebcartwright
added
release-notes
3 participants
Resolves #5542
Prior to rust-lang/rust#101212 the
ast::TraitObjectSyntaxenum only had two variantsDynandNone. The PR that introduced thedyn*syntax added a new variantDynStar, but did not update the formatting rules to account for the new variant.Now the new
DynStarvariant is properly handled and is no longer removed by rustfmt.