impl rewrite_result for ControlFlow, Stmt, update rewrite_index #6291
Conversation
| @@ -1703,12 +1703,11 @@ impl<'a> Iterator for CommentCodeSlices<'a> { | |||
| } | |||
|
|
|||
| /// Checks is `new` didn't miss any comment from `span`, if it removed any, return previous text | |||
| /// (if it fits in the width/offset, else return `None`), else return `new` | |||
There was a problem hiding this comment.
Before updating recover_comment_removed, it always returned Some(String). The previous comment about this function is no longer accurate, as it was written when the function used to call wrap_str a long time ago.
src/expr.rs
Outdated
| match (orig_index_rw, new_index_rw) { | ||
| (_, Some(ref new_index_str)) if !new_index_str.contains('\n') => Some(format!( | ||
| (_, Ok(ref new_index_str)) if !new_index_str.contains('\n') => Ok(format!( | ||
| "{}{}[{}]", | ||
| expr_str, | ||
| indent.to_string_with_newline(context.config), | ||
| new_index_str, | ||
| )), | ||
| (None, Some(ref new_index_str)) => Some(format!( | ||
| (Err(_), Ok(ref new_index_str)) => Ok(format!( | ||
| "{}{}[{}]", | ||
| expr_str, | ||
| indent.to_string_with_newline(context.config), | ||
| new_index_str, | ||
| )), | ||
| (Some(ref index_str), _) => Some(format!("{expr_str}[{index_str}]")), | ||
| _ => None, | ||
| (Ok(ref index_str), _) => Ok(format!("{expr_str}[{index_str}]")), | ||
| (Err(_), Err(new_index_rw_err)) => Err(new_index_rw_err), |
There was a problem hiding this comment.
orig_index_rw is the rewrite result of trying to fit the index on a single line, while new_index_rw is the rewrite result of placing the index on the next line. Since this is kinda first attempt and a second chance approach, it’s better to choose one error to propagate up when both results are err. I decided to propagate the error from the second attempt (new_index_rw_err) since it’s more generous in terms of width constraints.
This kind of matching orig_rw and new_rw is quite common, so if you have any thoughts on which one to return in this situation, please let me know.
There was a problem hiding this comment.
It's an interesting dilemma, but I think returning the second error in this case is fine. It might be worth leaving a comment in the code explaining some of this context and noting that the decision was somewhat arbitrary, but we decided to return the second rewrite failure.
src/expr.rs
Outdated
| match (orig_index_rw, new_index_rw) { | ||
| (_, Some(ref new_index_str)) if !new_index_str.contains('\n') => Some(format!( | ||
| (_, Ok(ref new_index_str)) if !new_index_str.contains('\n') => Ok(format!( | ||
| "{}{}[{}]", | ||
| expr_str, | ||
| indent.to_string_with_newline(context.config), | ||
| new_index_str, | ||
| )), | ||
| (None, Some(ref new_index_str)) => Some(format!( | ||
| (Err(_), Ok(ref new_index_str)) => Ok(format!( | ||
| "{}{}[{}]", | ||
| expr_str, | ||
| indent.to_string_with_newline(context.config), | ||
| new_index_str, | ||
| )), | ||
| (Some(ref index_str), _) => Some(format!("{expr_str}[{index_str}]")), | ||
| _ => None, | ||
| (Ok(ref index_str), _) => Ok(format!("{expr_str}[{index_str}]")), | ||
| (Err(_), Err(new_index_rw_err)) => Err(new_index_rw_err), |
There was a problem hiding this comment.
It's an interesting dilemma, but I think returning the second error in this case is fine. It might be worth leaving a comment in the code explaining some of this context and noting that the decision was somewhat arbitrary, but we decided to return the second rewrite failure.
| .. | ||
| }) => rewrite_bounded_lifetime(lifetime, bounds, context, shape)?, | ||
| span, | ||
| }) => rewrite_bounded_lifetime(lifetime, bounds, span, context, shape).ok()?, |
There was a problem hiding this comment.
Do we need to add a rewrite_result method for ast::WherePredicate? I see that this only updates the rewrite method.
There was a problem hiding this comment.
I added a rewrite_result for ast::WherePredicate in different branch. This pr does not update it.
There was a problem hiding this comment.
@ding-young Is there a PR open for those changes?
tracked by #6206
Description
rewrite_resultforStmt,ControlFlowOverflowableItem,LifetimeThere are still
unknown_error()s due to callingformat_expr, but they will be removed after updatingformat_exprin later pr.rewrite_indexto returnRewriteResultunknown_error()if possible