Fixed first comment and last comment not reordering issue #4504

whizsid · 2020-10-30T12:52:53Z

Fixes #4027 .

calebcartwright · 2020-10-30T14:10:47Z

Thanks @whizsid! Note CI is failing and we're blocked on rust-lang/rust#78549

calebcartwright · 2020-11-10T03:24:06Z

Blocking issues should be resolved. When you get a chance can you rebase on master to grab the latest and get CI passing?

…e_cmnt_order

calebcartwright

Thanks so much @whizsid! Minor change requested and a few nits/cleanup suggestions but looking pretty good!

calebcartwright · 2020-11-12T02:12:35Z

src/formatting/lists.rs

+                if !first
+                    && (!inner_item.is_empty()
+                        || item.post_comment.is_some()
+                        || item.pre_comment.is_some())
+                    && !result.is_empty() =>


This guard is starting to get pretty gnarly. Could you refactor it a bit to improve readability? We don't have any other subsequent Vertical arms, so I suppose some of these checks could be moved into the body (since it's a noop if conditions aren't met).

For example i think to keep this in the guard we'd want to potentially shift this to some locals like

let item_has_comments = item.post_comment.is_some() || item.pre_comment.is_some(); // or let non_empty_or_comments = !inner_item.is_empty() || item.post_comment.is_some() || item.pre_comment.is_some();

Otherwise I think it'd probably be much more readable to reduce the guard conditions and use these checks in the body to determine whether to push.

Also could we just do a single result.push(newline_str) now since that's declared above (vs. result.push('\n'); result.push(indent_str);)

calebcartwright · 2020-11-12T02:18:12Z

src/formatting/lists.rs

+                if !inner_item.is_empty()
+                    && (!formatting.align_comments
+                        || (last
+                            && item_max_width.is_some()
+                            && !separate
+                            && !formatting.separator.is_empty()))


I know this one was already pretty busy but the extra condition and subsequent indentation is a bit of an eye sore. I guess it'd be okay but I also wouldn't mind seeing some minor refactoring of this existing code too

calebcartwright · 2020-11-12T02:24:04Z

src/formatting/lists.rs

-                self.inner.peek().is_none(),
-            );
+            let comment_end =
+                if next_item.is_none() && last_line_contains_single_line_comment(post_snippet) {


what condition is the comment check for? a small example snippet would be helpful, my mental runtime is failing me 😄

@calebcartwright The trailing comment of last item is not capturing for the item list. So I put this condition to take the trailing comment if it in the same line.

Without this condition:-

use c; // cpost use b; // bpost use a; // Trailing comment of last item

formatting to

use a; use b; // bpost use c; // cpost // Trailing comment of last item

calebcartwright · 2020-11-12T02:27:16Z

src/formatting/lists.rs

@@ -397,7 +405,9 @@ where
            item_max_width = None;
        }

-        if separate && sep_place.is_front() && !first {
+        if inner_item.is_empty() && item.pre_comment.is_some() && item.post_comment.is_some() {


This is okay on its own, though along the same lines as the above comment I think declaring a local like item_has_comments and reusing could simplify some of these conditionals

calebcartwright · 2020-11-12T02:28:25Z

src/formatting/syntux/session.rs

+    pub(crate) fn line_bounds(&self, pos: BytePos) -> (BytePos, BytePos) {
+        let line = self.parse_sess.source_map().lookup_line(pos).ok();
+
+        match line {
+            Some(line_info) => line_info.sf.line_bounds(line_info.line),
+            None => (BytePos::from_usize(0), BytePos::from_usize(0)),
+        }
+    }


Hmmm, I don't think we'll want to silently map a source map lookup error to hi/lo 0 like this. It doesn't really seem a likely condition but I have a nagging suspicion it could eventually be hit which would cause some weird formatting behavior.

Probably best to set the return type to an option containing tuple, and then call explicitly unwrap from the caller to panic and fail loudly in that hypothetical case

calebcartwright · 2020-11-12T02:37:44Z

tests/source/issue-4027.rs

@@ -0,0 +1,30 @@
+mod a;


Love the thoroughness of the tests ❤️ Would you mind either:

Creating a new file like reorder_imports_comments.rs with separated blocks (with their own comment headers) and the exact snippets from Rustfmt puts single-line comment after wrong import when reordering imports #4027 and Comments on use statements incorrectly rearranged when use statements are rearranged #3720, and then any additional scenarios can go below

Creating separate files for those two issues with the exact snippets provided, and then any additional scenarios can go in new files

It's really helpful for long term maintenance to structure the tests in such a way that issue-specific fixes/tests can be tied back directly

whizsid · 2021-02-20T10:47:35Z

@calebcartwright I adjusted my PR. can you review again?

calebcartwright · 2021-02-21T00:12:00Z

@calebcartwright I adjusted my PR. can you review again?

Thanks for the updates! This PR has been sitting out here for a while though, would you please rebase on master?

whizsid · 2021-02-21T06:32:47Z

src/formatting/lists.rs

@@ -482,19 +481,88 @@ where
                let width = formatting.shape.width.checked_sub(overhead).unwrap_or(1);
                let offset = formatting.shape.indent + overhead;

-                let comment_shape = Shape::legacy(width, offset);


@calebcartwright I rebased with the master branch at the earlier commit. Accidentally I worked in the same commit that resolving the conflicts. L485-L565 lines from the last conflicted commit.

Cheers. That's not too surprising given the age, though I'll need to squash anyway give the merge commit.

I should finally have a bit of bandwidth in the next day or two to get back to the PR queue so will hopefully be able to review soon.

For future reference/PRs though it'd be helpful if to just do a clean rebase of your topic branch on the upstream branch from this repo and avoid merge commits 👍 Thanks again for all your work!

calebcartwright · 2021-04-08T02:30:14Z

While there's a few nits I do think the approach seems reasonable, and the resulting formatting is certainly better than what it exists today (some of these snippets get horribly butchered today).

I'd like to add a couple more test cases before merging though, just to be extra cautious. Could you add a couple snippets to cover the following:

A trailing mix of a block comment followed by a line comment (use foo; /* bar */ // evil
Multiline block comments
imports within indented scopes (e.g inside an inline module, function, etc.)

After those tests are added, should be good to merge

Fixed first comment and last comment not reordering issue

9c63115

calebcartwright added the pr-not-reviewed label Nov 10, 2020

Merge branch 'master' of https://github.com/rust-lang/rustfmt into us…

b5f9f5c

…e_cmnt_order

calebcartwright reviewed Nov 12, 2020

View reviewed changes

calebcartwright added pr-waiting-on-author and removed pr-not-reviewed labels Nov 12, 2020

Added new tests and improve readability

c52c000

Added tests to rust-lang#3127

e424884

whizsid commented Feb 21, 2021

View reviewed changes

calebcartwright removed the pr-waiting-on-author label Feb 23, 2021

calebcartwright added the pr-follow-up-review-pending label Mar 3, 2021

calebcartwright added pr-waiting-on-author and removed pr-follow-up-review-pending labels Apr 8, 2021

calebcartwright changed the base branch from master to rustfmt-2.0.0-rc.2 April 30, 2021 00:22

ytmimi added pr-targeting-2.0 This PR is targeting the 2.0 branch p-low labels Jul 27, 2022

calebcartwright mentioned this pull request Aug 7, 2022

Comments on use statements incorrectly rearranged when use statements are rearranged #3720

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed first comment and last comment not reordering issue #4504

Fixed first comment and last comment not reordering issue #4504

whizsid commented Oct 30, 2020

calebcartwright commented Oct 30, 2020 •

edited

Loading

calebcartwright commented Nov 10, 2020

calebcartwright left a comment

calebcartwright Nov 12, 2020

calebcartwright Nov 12, 2020

calebcartwright Nov 12, 2020

whizsid Feb 20, 2021

calebcartwright Nov 12, 2020

calebcartwright Nov 12, 2020

calebcartwright Nov 12, 2020

whizsid commented Feb 20, 2021

calebcartwright commented Feb 21, 2021

whizsid Feb 21, 2021

calebcartwright Feb 23, 2021

calebcartwright commented Apr 8, 2021

Fixed first comment and last comment not reordering issue #4504

Are you sure you want to change the base?

Fixed first comment and last comment not reordering issue #4504

Conversation

whizsid commented Oct 30, 2020

calebcartwright commented Oct 30, 2020 • edited Loading

calebcartwright commented Nov 10, 2020

calebcartwright left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whizsid commented Feb 20, 2021

calebcartwright commented Feb 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calebcartwright commented Apr 8, 2021

calebcartwright commented Oct 30, 2020 •

edited

Loading