Comments between assignment (without let) lhs and rhs handling#4661
Conversation
calebcartwright
left a comment
calebcartwright
left a comment
There was a problem hiding this comment.
Thanks for this! Looks pretty good, few minor comments left inline
| let span_hi = rhs.span.lo(); | ||
| let (operator_str, span_lo) = match op { | ||
| Some(op) => (context.snippet(op.span), op.span.hi()), | ||
| None => { | ||
| let lo = lhs.span.hi(); | ||
| let offset = context.snippet(mk_sp(lo, span_hi)).find_uncommented("=")? + 1; | ||
| ("=", lo + BytePos(offset as u32)) | ||
| } |
There was a problem hiding this comment.
I could be remembering incorrectly, but I believe the AST nodes for assignment expressions actually contain the span for the operator or binop. Could you double check that (presumably back at the call sites), and if it's there then update our destructuring code to grab it?
Would much rather plug and play from the AST than recalculating spans when possible
There was a problem hiding this comment.
Yes, there is such span. Changed the code to use it.
| let var = /* Block comment longgggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggg */ first; | ||
| var /= /* Block comment longgggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggg */ second; | ||
| } | ||
|
No newline at end of file |
There was a problem hiding this comment.
Solid test suite! For the sake of extreme thoroughness, do you think it'd be worthwhile to add a multiline blockstyle comment case?
There was a problem hiding this comment.
Added multiline comments test cases.
However, I think there is an issue with the multiline comments formatting by combine_strs_with_missing_comments() (not caused by this PR so I didn't try to fix it here). An example of the issue is that the following formatted code:
let var = /* Block comment line 1
* Block comment line 2 */
first;Should probably have been either:
let var = /* Block comment line 1
* Block comment line 2 */
first;or:
let var = /* Block comment line 1
* Block comment line 2 */ first;Is this really an issue? If yes, which of the output versions is desired? If this is an issue that should be fixed, I can try to do that.
There was a problem hiding this comment.
Is this really an issue?
it is an issue yes. some of these comment edge cases can be so exhausting sometimes 😄 it would have to be the former or what we did in other similar scenarios
e.g. something like this (hand typed to give a visual, indentation may be off)
let var =
/* Block comment line 1
* Block comment line 2 */
first;I'd prefer the former in the spirit of maintaining developer defined comment association, as odd as such a comment would be in these types of local cases.
|
Also be sure to rebase to pull in the rustc-ap crate updates |
davidBar-On
force-pushed
the
lhs-to-rhs-bewteen-comments-assignment
branch
from
4522b75 to
24e632e
Compare
|
I am going to go ahead and merge this in the spirit of moving forward, though want to make a note of how it would potentially be problematic in the 1.x case. Currently, in the presence of such comments rustfmt will bail and leave the original formatting in place, but with this change rustfmt will of course reformat all such cases. The multi-line block style comment that opens on the same line as the operator would be problematic though as however unlikely that actually is in practice, we'd be emitting formatting that's off which the user would be unable to correct. Only going to merge here because it's against the master branch and it's such an extreme edge case that we'll get sorted quickly anyway. Let's prioritize getting that edge case resolved though 👍 |
3 participants
Add handling of comments between assignment (without let) lhs and rhs by using
rewrite_assign_rhs_with_comments()(one of the cases identified in #4626 discussion).