fix adds a trailing comma to struct-like macro #2490

csmoe · 2018-02-25T07:48:10Z

should resolve #2445
Input

test!(RunPassPretty {
    path: "src/test/run-pass/pretty",
    mode: "pretty",
    suite: "run-pass",
    default: false,
    host: true
});

Output(before)

test!(RunPassPretty {
    path: "src/test/run-pass/pretty",
    mode: "pretty",
    suite: "run-pass",
    default: false,
    host: true,
});

After

test!(RunPassPretty {
    path: "src/test/run-pass/pretty",
    mode: "pretty",
    suite: "run-pass",
    default: false,
    host: true
});

topecongiro

Thank you for your PR! And sorry about the late review.

The added test is wrong, it could be that span_ends_with_comma does not work with comments (I added it, sorry!). Would you please update this PR to fix it?

topecongiro · 2018-03-02T13:03:55Z

rustfmt-core/tests/target/issue-2445.rs

+});
+
+test!(Test {
+    field: i32 // comment


This test seems wrong: we need to preserve a trailing comma if the macro originally has it.

topecongiro · 2018-03-02T13:12:00Z

rustfmt-core/src/expr.rs

 /// trailing comma. This function is used when rewriting macro, as adding or removing a trailing
 /// comma from macro can potentially break the code.
 fn span_ends_with_comma(context: &RewriteContext, span: Span) -> bool {
    let mut encountered_closing_paren = false;
+    let mut encountered_closing_braces = false;
    for c in context.snippet(span).chars().rev() {


Would you please try using CharClasses here?

topecongiro · 2018-03-02T13:23:05Z

And this PR needs rebase. Sorry!

nrc · 2018-03-05T03:15:14Z

@csmoe I think the rebase went wrong. Within the branch struct_macro_trailing_comma you should be able to run git rebase master to rebase your commits on top of master without pulling in the intermediate commits. To get back to a state where you can do that, you probably want to use git reflog and checkout an earlier version of your branch. Let me know if you need more help with this.

…com/csmoe/rustfmt into struct_macro_trailing_comma" This reverts commit 21916b9.

csmoe · 2018-03-06T16:09:31Z

src/expr.rs

+#[allow(dead_code)]
+/// Check if a struct representation ends with comma by match on the chars of struct
+/// snippet reversely, once a FullCodeCharKind::Normal comma matched, returns true
+fn struct_lit_ends_with_comma(context: &RewriteContext, span: Span) -> bool {


I added a function to check if a struct lit ends with comma here, but I need to match the comma reversely because normally match might never handle the last field before it returns.

I had tried to impl DoubleEndedIterator for CharClasses {...}, the basic next_back method is easy to learn from std-docs, but it's a bit difficult for me to tackle the method on CharClasses, could you please give me some tips the deal with it? @topecongiro

csmoe · 2018-03-07T13:16:56Z

Input

test!(RunPassPretty {
            // comment
    path: "src/test/run-pass/pretty",
    mode: "pretty",
    suite: "run-pass",
    default: false,
    host: true  // should, force no trailing comma here
});

test!(RunPassPretty {
            // comment
    path: "src/test/run-pass/pretty",
    mode: "pretty",
    suite: "run-pass",
    default: false,
    host: true,         // should, preserve the trailing comma
});

test!(Test{
    field: i32, // comment
});

Output(before)

test!(RunPassPretty {
    // comment
    path: "src/test/run-pass/pretty",
    mode: "pretty",
    suite: "run-pass",
    default: false,
    host: true, // should, force no trailing comma here
});

test!(RunPassPretty {
    // comment
    path: "src/test/run-pass/pretty",
    mode: "pretty",
    suite: "run-pass",
    default: false,
    host: true, // should, preserve the trailing comma
});

test!(Test {
    field: i32, // comment
});

After

test!(RunPassPretty {
    // comment
    path: "src/test/run-pass/pretty",
    mode: "pretty",
    suite: "run-pass",
    default: false,
    host: true // should, force no trailing comma here
});

test!(RunPassPretty {
    // comment
    path: "src/test/run-pass/pretty",
    mode: "pretty",
    suite: "run-pass",
    default: false,
    host: true, // should, preserve the trailing comma
});

test!(Test {
    field: i32, // comment
});

topecongiro · 2018-03-08T04:47:21Z

@csmoe Thank you for your update! And my apologies that I could not give you a feedback in a timely manner.

I am afraid that using retain does not work well when there is a comma inside comment. Would you please add a test where there is a comma inside comment?

I think that you do not need to implement DoubleEndedIterator for CharClasses, just check that the last normal code is ,) or ,}. Rather than returning early, we could keep processing so that the last field will be checked. For example, you could write like this:

    let mut result;
    let mut prev_char;

    for (kind, c) in CharClasses::new(context.snippet(span).chars()) {
        match c {
            _ if kind.is_comment() || c.is_whitespace() => continue,
            ')' | '}' => result = result && prev_char != c,
            ',' => result = true,
            _ => result = false,
        }
        prev_char = c;
    }

    result

csmoe · 2018-03-08T07:19:19Z

@topecongiro Comma in comment fixed.

Thank for your patience on guiding me here and sorry for bothering you with the annoying errors in those undeliberate commits. I have learned some tips to write a proper test case and skills to tackle string scanning, Thank you.

topecongiro

Thank you for the update! I left two inline comments about minor updates, otherwise LGTM.

topecongiro · 2018-03-08T08:02:50Z

src/expr.rs

@@ -7,7 +7,6 @@
 // <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
-


Would you please bring back an empty line here?

topecongiro · 2018-03-08T08:03:11Z

src/lib.rs

@@ -12,6 +12,7 @@
 #![feature(decl_macro)]
 #![feature(match_default_bindings)]
 #![feature(type_ascription)]
+#![feature(string_retain)]


Would you please remove this unused feature?

topecongiro · 2018-03-08T09:05:47Z

Thank you!

topecongiro reviewed Mar 2, 2018

View reviewed changes

csmoe changed the title ~~fix adds a trailing comma to struct-like macro~~ [WIP]fix adds a trailing comma to struct-like macro Mar 5, 2018

csmoe changed the title ~~[WIP]fix adds a trailing comma to struct-like macro~~ [WIP] fix adds a trailing comma to struct-like macro Mar 5, 2018

csmoe force-pushed the struct_macro_trailing_comma branch from 21916b9 to 5df80f2 Compare March 5, 2018 13:34

csmoe added 2 commits March 5, 2018 22:02

fix adds a trailing comma to struct-like macro

e4c9683

Revert "Merge branch 'struct_macro_trailing_comma' of https://github.…

73b06bb

…com/csmoe/rustfmt into struct_macro_trailing_comma" This reverts commit 21916b9.

csmoe force-pushed the struct_macro_trailing_comma branch from 5df80f2 to 73b06bb Compare March 5, 2018 14:08

check struct lit ends with comma

e96403e

csmoe commented Mar 6, 2018

View reviewed changes

csmoe changed the title ~~[WIP] fix adds a trailing comma to struct-like macro~~ fix adds a trailing comma to struct-like macro Mar 7, 2018

fix tests and not needed force no trailing comma

a4cbcb9

csmoe force-pushed the struct_macro_trailing_comma branch from 10e808b to a4cbcb9 Compare March 7, 2018 13:23

csmoe added 2 commits March 8, 2018 15:02

fix test for comma in comment

34b009f

fix comma in comment

cd9ef30

topecongiro suggested changes Mar 8, 2018

View reviewed changes

remove extra feature and bring back newline

75d948b

topecongiro approved these changes Mar 8, 2018

View reviewed changes

topecongiro merged commit a2f8617 into rust-lang:master Mar 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix adds a trailing comma to struct-like macro #2490

fix adds a trailing comma to struct-like macro #2490

Uh oh!

csmoe commented Feb 25, 2018

Uh oh!

topecongiro left a comment

Uh oh!

topecongiro Mar 2, 2018

Uh oh!

topecongiro Mar 2, 2018

Uh oh!

topecongiro commented Mar 2, 2018

Uh oh!

nrc commented Mar 5, 2018

Uh oh!

csmoe Mar 6, 2018

Uh oh!

csmoe commented Mar 7, 2018 •

edited

Loading

Uh oh!

topecongiro commented Mar 8, 2018

Uh oh!

csmoe commented Mar 8, 2018

Uh oh!

topecongiro left a comment

Uh oh!

topecongiro Mar 8, 2018

Uh oh!

topecongiro Mar 8, 2018

Uh oh!

topecongiro commented Mar 8, 2018

Uh oh!

Uh oh!

fix adds a trailing comma to struct-like macro #2490

fix adds a trailing comma to struct-like macro #2490

Uh oh!

Conversation

csmoe commented Feb 25, 2018

Uh oh!

topecongiro left a comment

Choose a reason for hiding this comment

Uh oh!

topecongiro Mar 2, 2018

Choose a reason for hiding this comment

Uh oh!

topecongiro Mar 2, 2018

Choose a reason for hiding this comment

Uh oh!

topecongiro commented Mar 2, 2018

Uh oh!

nrc commented Mar 5, 2018

Uh oh!

csmoe Mar 6, 2018

Choose a reason for hiding this comment

Uh oh!

csmoe commented Mar 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

topecongiro commented Mar 8, 2018

Uh oh!

csmoe commented Mar 8, 2018

Uh oh!

topecongiro left a comment

Choose a reason for hiding this comment

Uh oh!

topecongiro Mar 8, 2018

Choose a reason for hiding this comment

Uh oh!

topecongiro Mar 8, 2018

Choose a reason for hiding this comment

Uh oh!

topecongiro commented Mar 8, 2018

Uh oh!

Uh oh!

csmoe commented Mar 7, 2018 •

edited

Loading