Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistent behavior #2716

Closed
volzkzg opened this issue May 18, 2018 · 12 comments
Closed

Inconsistent behavior #2716

volzkzg opened this issue May 18, 2018 · 12 comments
Labels
a-macros bug Panic, non-idempotency, invalid code, etc. good first issue Issues up for grabs, also good candidates for new rustfmt contributors

Comments

@volzkzg
Copy link

volzkzg commented May 18, 2018

The problem is as following.

When I run cargo fmt --all, it should modify all the files that do not follow the format standard. But after I running cargo fmt --all -- --check, it still tells me one file does not have the correct format.

root@d115673865b5:~/cita# cargo fmt --all
root@d115673865b5:~/cita# cargo fmt --all -- --check
Diff in /root/cita/tools/relayer-parser/src/communication.rs at line 126:
         define_reply_type!(ReplyType, $result_type);
         let rpc_cli = RpcClient::new($upstream);
         let body: String = json!({
-                                    "jsonrpc": "2.0",
-                                    "method": $method,
-                                    "params": json!($params),
-                                    "id": 1
-                                }).to_string();
+                                        "jsonrpc": "2.0",
+                                        "method": $method,
+                                        "params": json!($params),
+                                        "id": 1
+                                    }).to_string();
         let data = rpc_cli.do_post(&body)?;
         let reply: ReplyType = serde_json::from_slice(&data).map_err(|_| {
             error!(

Rustfmt version is nightly-2018-05-17
System is Ubuntu 18.04

@nrc
Copy link
Member

nrc commented May 18, 2018

If you run cargo fmt twice, does the second run reformat?

@nrc nrc added the bug Panic, non-idempotency, invalid code, etc. label May 18, 2018
@volzkzg
Copy link
Author

volzkzg commented May 18, 2018

@nrc I just ran cargo fmt twice, more serious problem happened.

root@d115673865b5:~/cita# cargo fmt
root@d115673865b5:~/cita# cargo fmt
root@d115673865b5:~/cita# cargo fmt --check
Unrecognized option: 'check'
usage: cargo fmt [options]

Options:
    -h, --help          show this message
    -q, --quiet         no output printed to stdout
    -v, --verbose       use verbose output
    -p, --package <package>
                        specify package to format (only usable in workspaces)
        --version       print rustfmt version and exit
        --all           format all packages (only usable in workspaces)

This utility formats all bin and lib files of the current crate using rustfmt. Arguments after `--` are passed to rustfmt.
root@d115673865b5:~/cita# cargo fmt -- --check
Diff in /root/cita/tools/relayer-parser/src/communication.rs at line 126:
         define_reply_type!(ReplyType, $result_type);
         let rpc_cli = RpcClient::new($upstream);
         let body: String = json!({
-                                            "jsonrpc": "2.0",
-                                            "method": $method,
-                                            "params": json!($params),
-                                            "id": 1
-                                        }).to_string();
+                                                "jsonrpc": "2.0",
+                                                "method": $method,
+                                                "params": json!($params),
+                                                "id": 1
+                                            }).to_string();
         let data = rpc_cli.do_post(&body)?;
         let reply: ReplyType = serde_json::from_slice(&data).map_err(|_| {
             error!(
root@d115673865b5:~/cita# cargo fmt
root@d115673865b5:~/cita# cargo fmt
root@d115673865b5:~/cita# cargo fmt -- --check
Diff in /root/cita/tools/relayer-parser/src/communication.rs at line 126:
         define_reply_type!(ReplyType, $result_type);
         let rpc_cli = RpcClient::new($upstream);
         let body: String = json!({
-                                                    "jsonrpc": "2.0",
-                                                    "method": $method,
-                                                    "params": json!($params),
-                                                    "id": 1
-                                                }).to_string();
+                                                        "jsonrpc": "2.0",
+                                                        "method": $method,
+                                                        "params": json!($params),
+                                                        "id": 1
+                                                    }).to_string();
         let data = rpc_cli.do_post(&body)?;
         let reply: ReplyType = serde_json::from_slice(&data).map_err(|_| {
             error!(

As you can see, it just keeps adding indents to my code.

In summary, it just adds 4 spaces in my code every time I run cargo fmt

@nrc nrc added the a-macros label May 18, 2018
@volzkzg
Copy link
Author

volzkzg commented May 19, 2018

@nrc I investigate this bug a little bit today. Here is my observations.

Suppose we have same snippet

let body: String = json!({
"jsonrpc": "2.0",
"method": $method,
"params": json!($params),
"id": 1})
.to_string();

in two blocks, the one is inside macro declaration and the another is in main function. (i.e, I guess we have different approach to macro and function).

It will look like this

macro_rules! rpc_send_and_get_result_from_reply {
    ($upstream:ident, $method:expr, $params:tt, $result_type:path) => {{
        define_reply_type!(ReplyType, $result_type);
        let rpc_cli = RpcClient::new($upstream);

        let body: String = json!({
        "jsonrpc": "2.0",
        "method": $method,
        "params": json!($params),
        "id": 1})
        .to_string();

        let data = rpc_cli.do_post(&body)?;
        let reply: ReplyType = serde_json::from_slice(&data).map_err(|_| {
            error!(
                "send {:?} return error: {:?}",
                &body,
                ::std::str::from_utf8(&data)
            );
            Error::Parse
        })?;
        trace!("get reply {:?}.", reply);
        reply.result
    }};
}

fn main() {
    let body: String = json!({
    "jsonrpc": "2.0",
    "method": $method,
    "params": json!($params),
    "id": 1})
    .to_string();
}

When I try `cargo fmt -- --check```, the information is following

root@d115673865b5:~/format# cargo fmt -- --check
Diff in /root/format/src/main.rs at line 2:
     ($upstream:ident, $method:expr, $params:tt, $result_type:path) => {{
         define_reply_type!(ReplyType, $result_type);
         let rpc_cli = RpcClient::new($upstream);
-
+
         let body: String = json!({
-        "jsonrpc": "2.0",
-        "method": $method,
-        "params": json!($params),
-        "id": 1})
-        .to_string();
-
+            "jsonrpc": "2.0",
+            "method": $method,
+            "params": json!($params),
+            "id": 1})
+            .to_string();
+
         let data = rpc_cli.do_post(&body)?;
         let reply: ReplyType = serde_json::from_slice(&data).map_err(|_| {
             error!(
Diff in /root/format/src/main.rs at line 30:
     "method": $method,
     "params": json!($params),
     "id": 1})
-    .to_string();
+        .to_string();
 }

We found two different approach to deal with this json! macro.

  1. The one in main function just ignore any indentation for arguments inside json! macro.
  2. The one in macros indent all arguments inside json!() by 4 spaces and .to_string() by 4 spaces too.

And then I apply cargo fmt, run cargo fmt -- --check again, it says

root@d115673865b5:~/format# cargo fmt
root@d115673865b5:~/format# cargo fmt -- --check
Diff in /root/format/src/main.rs at line 4:
         let rpc_cli = RpcClient::new($upstream);
 
         let body: String = json!({
-            "jsonrpc": "2.0",
-            "method": $method,
-            "params": json!($params),
-            "id": 1})
+                "jsonrpc": "2.0",
+                "method": $method,
+                "params": json!($params),
+                "id": 1})
             .to_string();
 
         let data = rpc_cli.do_post(&body)?;

just want to indent 4 more spaces for json!()'s argument's content (which is inside {}).

And if I run cargo fmt and cargo fmt -- --check, it just want to indent more.

root@d115673865b5:~/format# cargo fmt
root@d115673865b5:~/format# cargo fmt -- --check
Diff in /root/format/src/main.rs at line 4:
         let rpc_cli = RpcClient::new($upstream);
 
         let body: String = json!({
-                "jsonrpc": "2.0",
-                "method": $method,
-                "params": json!($params),
-                "id": 1})
+                    "jsonrpc": "2.0",
+                    "method": $method,
+                    "params": json!($params),
+                    "id": 1})
             .to_string();
 
         let data = rpc_cli.do_post(&body)?;

It seems that reformatting macro invocations is related to the context. And it will have different results in these different contexts.

@nrc
Copy link
Member

nrc commented May 20, 2018

THanks for the additional info! Will try and fix this soon...

@topecongiro
Copy link
Contributor

This seems to be a duplicate of #2695.

@volzkzg
Copy link
Author

volzkzg commented May 23, 2018

@topecongiro I'm not sure, because the version of my rustfmt is 2018-05-17. And they said their issue was fixed in the newest nightly 11 days ago.

@okready
Copy link

okready commented Jun 15, 2018

I'm chiming in because I just encountered this same issue today. If it helps, the following code provides a consistent reproduction:

macro_rules! invoke_for_options {
    ($mac:ident!() $opt:tt $($other:tt)*) => {
        $mac!($opt);
        invoke_for_options!($mac!() $($other)*);
    };

    ($mac:ident!()) => {};
}

macro_rules! do_stuff {
    ($opt:expr) => {
        println!("{}", $opt);
    };
}

macro_rules! outer_macro {
    () => {
        invoke_for_options!(
            do_stuff!()
            "abcd" "efgh" "ijkl" "mnop" "qrst" "uvwx" "yz"
        );
    };
}

fn main() {
    outer_macro!();
}

The arguments passed to invoke_for_options!() within the outer_macro!() definition get indented more with each subsequent run of cargo fmt. Removing outer_macro!() and making the same invoke_for_options!() call directly within main() does not cause cargo fmt to continually indent.

The 1.26.2 stable, 1.27.0-beta.11, and 2018-06-13 nightly toolchains all exhibit the same behavior (I'm currently testing within Windows).

@topecongiro
Copy link
Contributor

I think the issue is already fixed on the master (via #2775). We need to add regression tests to close this issue.

@topecongiro topecongiro added the good first issue Issues up for grabs, also good candidates for new rustfmt contributors label Jun 15, 2018
@okready
Copy link

okready commented Jun 15, 2018

Just installed the latest from master. I can verify that the issue does not occur when running it on my test case or the code from which it was based. Thanks!

@okready
Copy link

okready commented Jun 15, 2018

I was looking into adding a regression test, but it appears this is the same bug as #2607, which has a regression test (even the workaround mentioned in that bug to use curly braces instead of parentheses bypasses the issue with older versions of rustfmt). Should this just be marked a a duplicate, or should there still be similar tests using the code examples from this issue?

@nrc
Copy link
Member

nrc commented Jun 18, 2018

Closed by #2775

@scampi
Copy link
Contributor

scampi commented Jun 23, 2018

@nrc should this issue be closed ?

@nrc nrc closed this as completed Jun 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-macros bug Panic, non-idempotency, invalid code, etc. good first issue Issues up for grabs, also good candidates for new rustfmt contributors
Projects
None yet
Development

No branches or pull requests

5 participants