Correctly format typed self in function arguments #159
Conversation
| @@ -790,6 +759,37 @@ impl<'a> FmtVisitor<'a> { | |||
| } | |||
| } | |||
|
|
|||
| fn rewrite_explicit_self(explicit_self: &ast::ExplicitSelf, args: &[ast::Arg]) -> String { | |||
There was a problem hiding this comment.
Better to return Option<String> than giving a special meaning to the empty string (also saves an allocation).
There was a problem hiding this comment.
I considered this, but reserved the None case for if we want to implement Rewrite for it later. I don't think String::new allocates, by the way.
There was a problem hiding this comment.
I guess maybe return a custom enum then? Alternatively we could always return Option<Option> if we implement Rewrite (which would require making Rewrite more generic I guess). If we have to treat the result specially if its an empty string, then it really ought to be explicit by using an enum of some kind.
|
@marcusklaas thanks for all the patches! You've done a tonne of good work recently :-) Heads up that I'll be travelling next week, so might be a bit slower with reviews and merging. |
|
Updated! |
|
Wait, maybe there is no need to skip it once we fix the span... Why do we need |
|
Should we consider merging this? It's not ideal, but at least fixes the underlying bug. To do it the proper way would be to address rust-lang/rust#27522, which isn't trivial. Until that happens, the comment for the self type is dropped. We could open a separate issue for that. |
|
Yes, we should! (Sorry it took a while to get to). @marcusklaas could you add a fixme comment referencing #27522 please? Then you can push this. |
Correctly format typed self in function arguments
This closes https://github.com/nrc/rustfmt/issues/153.