Impl rewrite_result for ast nodes in items.rs

[ding-young]

Tracked by #6206

Description

• added RewriteError and blanket implementation for rewrite_result
• above 6 ast nodes that implements rewrite, replaced rewrite function body with rewrite_result for 4 nodes;
  ast::Local, ast::FieldDef, ast::Param, ast::FnRetTy

[ytmimi]

Looks like we're on the right track with this. Left a few suggestions for way that we could improve things.

[ytmimi]

This isn't something we need to address right now, but I've noticed a lot of .ok_or_else(|| { RewriteError::ExceedsMaxWidth {..} }) calls throughout the code.

I wonder if we can make this a little more ergonomic. Here are three ideas I have:

1. Define new helper methods on Shape

2. Add a macro that will handle some of this boilerplate

3. Define an extension trait so we can more easily convert from Option<T> -> Result<T, RewriteError>.
   Here's one idea that I had:

   /// Extension trait used to conveniently convert to RewriteError
   pub(crate) trait RewriteErrorExtension<T> {
       fn max_with_error(self, width: usize, span: Span) -> Result<T, RewriteError>;
       fn unknown_error(self) -> Result<T, RewriteError>;
   }
   
   impl<T> RewriteErrorExtension<T> for Option<T> {
       fn max_with_error(self, width: usize, span: Span) -> Result<T, RewriteError> {
           self.ok_or_else(|| RewriteError::ExceedsMaxWidth {
               configured_width: width,
               span: span,
           })
       }
   
       fn unknown_error(self) -> Result<T, RewriteError> {
           self.ok_or_else(|| RewriteError::Unknown)
       }
   }

[ding-young]

@ytmimi Thank you for your review. It seems like I missed some details. I'll take a more careful look at the next PR.
I am wondering to what extent I should modify functions like rewrite_*** in this PR. For example, you recommended modifying rewrite_let_else_block and rewrite_explicit_self, while you suggested changing the signature of combine_str_with_missing_comment later. Should I proceed with modifying the simpler ones for now?

[ytmimi]

I'm fairly certain that rewrite_let_else_block and rewrite_explicit_self are only used within these functions so it's likely easier to update their signatures now, while combine_str_with_missing_comment is used throughout the codebase and would require touching a lot of different files.

If you feel like it would be a lot to extend the scope of the current PR then we can always revisit modifying all of these functions in future PRs, but if it's not a heavy lift then we might want to consider doing some of that work in this PR.

[ytmimi]

Thanks for working through all the feedback! Here's my follow up review

[ytmimi]

On thing I've been wondering is whether we should use shap.width or context.config.max_width() for the max_width_error. Would love to get your thoughts on this.

[ding-young]

I believe shape.width provides more specific information, considering that rewrite tries to fit the formatted code into the given shape, while context.config.max_width() is set by the global configuration. Even if we want to expose only max_width to users in error messages (since shape.width might be confusing), in most cases, callers can retrieve context.config.max_width() from its rewrite context so converting RewriteError to ErrorKind would be possible.

However, if there isn't much opportunity to retry formatting based on the returned RewriteError with shape.width and span, it might be pointless to include shape.width, especially if this information is unused.

[ytmimi]

I think it's fine to leave this as shape.width for now. We can always revisit this decision in the future if it turns out there's something better we should use instead of the width.

[ding-young]

Since we are gonna continue formatting though offset_left returns None, I replaced ty.rewrite with ty.rewrite_result.ok() instead of converting Option into RwResult like below.

let orig_ty = shape
        .offset_left(overhead + spacing.len())
        .max_width(shape.width, field.span())
        .and_then(|ty_shape| field.ty.rewrite_result(context, ty_shape));

    if let Ok(ref ty) = orig_ty {
        if !ty.contains('\n') && !contains_comment(context.snippet(missing_span)) {
            return Ok(attr_prefix + &spacing + ty);
        }
    }

Although it seems somewhat pointless to calling rewrite_result.ok() instead of rewrite, I did so since rewrite will return Result after all.
It's pretty minor issue, but I want to hear your suggestion if you have one.

[ytmimi]

I think calling rewrite_result.ok() is fine. We could also follow your suggestion, but I don't think it makes much of a difference right now.

[ytmimi]

Great work. I took another look and I think we're good to go on this one

[ytmimi]

@ding-young when you have a chance can you squash the commits and rebase this PR to bring it up to date