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

Formatting with rustfmt no longer works in rustc repository #10209

Closed
RalfJung opened this issue Sep 11, 2021 · 12 comments · Fixed by #13064
Closed

Formatting with rustfmt no longer works in rustc repository #10209

RalfJung opened this issue Sep 11, 2021 · 12 comments · Fixed by #13064
Labels
C-bug Category: bug S-unactionable Issue requires feedback, design decisions or is blocked on other work

Comments

@RalfJung
Copy link
Member

RalfJung commented Sep 11, 2021

It used to be the case that formatting worked even inside the rustc repo, but something seems to have broken since I last tried that (a few weeks ago). Now, when I ask RA to format a file in the rustc repo, nothing at all happens -- I see no error or other kind of feedback, and nothing gets formatted. (So there are at least 2 bugs here: formatting doesn't work, and error reporting also doesn't work.)

My workspace config is:

{
    "rust-analyzer.checkOnSave.enable": true,
    "rust-analyzer.checkOnSave.overrideCommand": [
        "./x.py",
        "check",
        "--json-output",
        "library/std",
        //"compiler/rustc",
    ],
    "rust-analyzer.rustfmt.overrideCommand": [
        "./build/x86_64-unknown-linux-gnu/stage0/bin/rustfmt"
    ],
    "rust-analyzer.cargo.runBuildScripts": false,
    "rust-analyzer.procMacro.enable": false,
    /*"rust-analyzer.linkedProjects": [
        "./Cargo.toml",
        "./compiler/rustc_codegen_cranelift/Cargo.toml" // see https://github.com/rust-analyzer/rust-analyzer/issues/8937
    ],*/
    "editor.formatOnSave": false,
    "files.watcherExclude": {
        "*rustc*/src/llvm-project/**": true,
        "*rustc*/build/**": true,
    },
    "files.exclude": {
        "src/llvm-project/**": true,
        "build/**": true
    },
}

I verified that ./build/x86_64-unknown-linux-gnu/stage0/bin/rustfmt still exists. I can't see any rustfmt-related messages in the various "output" categories.

I updated vscode (v.1.60.0 now), which did not help. It says there are no extension updates available. RA has version v0.2.735.

@lnicola
Copy link
Member

lnicola commented Sep 11, 2021

Just to avoid an obvious issue, does it behave the same if you put in an absolute path? Can you strace/dtrace the language server to see if it spawns something?

@RalfJung
Copy link
Member Author

does it behave the same if you put in an absolute path?

Yeah that does not seem to make a difference.

Can you strace/dtrace the language server to see if it spawns something?

I have no idea how to do that.^^ The entire language server business is a total black box to me.
(Also I don't have a lot of time to debug this currently, I'm afraid.)

@lnicola
Copy link
Member

lnicola commented Sep 11, 2021

Well, on Linux you find the PID of the language server (I don't think you have proc macros enabled on rustc, but take the parent process if there's two of them), then run strace -f -e trace=execvp -p PID and save a file.

@lnicola lnicola added the C-bug Category: bug label Sep 13, 2021
@RalfJung
Copy link
Member Author

RalfJung commented Sep 17, 2021

on Linux you find the PID of the language server

How do I do that? I know nothing about how RA works, and code spawns like 10 processes. One of them is called rust-analyzer-x86_64-unknown-linux-gnu, is that the language server? It doesn't have "language" or "server" anywhere in its name but it is the only one not using the code binary, so I will go with that hypothesis...

Your suggested strace command says "strace: invalid system call 'execvp'". When I just do strace -p PID, I don't get anything interesting:

futex(0x5589ee66aa88, FUTEX_WAIT_PRIVATE, 4294967295, NULL) = 0
futex(0x5589ee676cb8, FUTEX_WAKE_PRIVATE, 1) = 1
futex(0x5589ee673588, FUTEX_WAKE_PRIVATE, 1) = 1
sched_yield()                           = 0
sched_yield()                           = 0
sched_yield()                           = 0
sched_yield()                           = 0
futex(0x5589ee66aa88, FUTEX_WAIT_PRIVATE, 4294967295, NULL) = 0
futex(0x5589ee67e168, FUTEX_WAKE_PRIVATE, 1) = 1
sched_yield()                           = 0
sched_yield()                           = 0
sched_yield()                           = 0
sched_yield()                           = 0
futex(0x5589ee66aa88, FUTEX_WAIT_PRIVATE, 4294967295, NULL) = 0
futex(0x5589ee673588, FUTEX_WAKE_PRIVATE, 1) = 1
sched_yield()                           = 0
sched_yield()                           = 0
sched_yield()                           = 0
sched_yield()                           = 0
sched_yield()                           = 0
futex(0x5589ee66aa88, FUTEX_WAIT_PRIVATE, 4294967295, NULL) = 0
futex(0x5589ee676cf8, FUTEX_WAKE_PRIVATE, 1) = 1
sched_yield()                           = 0
sched_yield()                           = 0
sched_yield()                           = 0
sched_yield()                           = 0
futex(0x5589ee66aa88, FUTEX_WAIT_PRIVATE, 4294967295, NULL) = 0
futex(0x5589ee673588, FUTEX_WAKE_PRIVATE, 1) = 1
sched_yield()                           = 0
sched_yield()                           = 0
sched_yield()                           = 0
sched_yield()                           = 0
futex(0x5589ee66aa88, FUTEX_WAIT_PRIVATE, 4294967295, NULL) = 0
futex(0x5589ee675568, FUTEX_WAKE_PRIVATE, 1) = 1
sched_yield()                           = 0
sched_yield()                           = 0
sched_yield()                           = 0
sched_yield()                           = 0
futex(0x5589ee66aa88, FUTEX_WAIT_PRIVATE, 4294967295, NULL) = 0
futex(0x5589ee673588, FUTEX_WAKE_PRIVATE, 1) = 1
sched_yield()                           = 0
sched_yield()                           = 0
sched_yield()                           = 0
sched_yield()                           = 0
futex(0x5589ee66aa88, FUTEX_WAIT_PRIVATE, 4294967295, NULL^Cstrace: Process 759571 detached
 <detached ...>

@RalfJung
Copy link
Member Author

When I add -f, I actually get something involving "exec":

[pid 760162] execve("./build/x86_64-unknown-linux-gnu/stage0/bin/rustfmt", ["./build/x86_64-unknown-linux-gnu"...], 0x5589ee66ab80 /* 82 vars */ <unfinished ...>
[pid 759582] <... clone resumed>)       = 1425
[pid 760162] <... execve resumed>)      = 0

And then something starts reading rustfmt configuration, so I guess it did launch something?

[pid 760162] statx(AT_FDCWD, "/home/r/src/rust/rustc/.rustfmt.toml", AT_STATX_SYNC_AS_STAT, STATX_ALL, 0x7ffc8423aed0) = -1 ENOENT (No such file or directory)
[pid 760162] statx(AT_FDCWD, "/home/r/src/rust/rustc/rustfmt.toml", AT_STATX_SYNC_AS_STAT, STATX_ALL, {stx_mask=STATX_ALL|STATX_MNT_ID, stx_attributes=0, stx_mode=S_IFREG|0644, stx_size=1055, ...}) = 0
[pid 760162] openat(AT_FDCWD, "/home/r/src/rust/rustc/rustfmt.toml", O_RDONLY|O_CLOEXEC) = 3

@RalfJung
Copy link
Member Author

I managed to set RA_LOG=info and found this in the output:

[INFO rust_analyzer::handlers] rustfmt exited with status 1, assuming parse error and ignoring

That is a wrong assumption, the file parses just fine. Silently dropping non-zero exit codes like this also does not seem very robust...

@RalfJung
Copy link
Member Author

RalfJung commented Sep 17, 2021

When I run rustfmt manually on the given file, I get this output:

error[E0642]: patterns aren't allowed in methods without bodies
  --> /home/r/src/rust/rustc/compiler/rustc_middle/src/mir/interpret/pointer.rs:55:31
   |
55 |     fn truncate_to_ptr(&self, (val, over): (u64, bool)) -> (u64, bool) {
   |                               ^^^^^^^^^^^
   |
help: give this argument a name or use an underscore to ignore it
   |
55 |     fn truncate_to_ptr(&self, _: (u64, bool)) -> (u64, bool) {
   |                               ~

So I guess RA is right after all, this is a parse error... but it shouldn't be, rustc parses fine, just rustfmt does not.

I'll report this with rustfmt, but this was a lot harder to debug than I think it has to be. Anyway, thanks for your help!

One proposal that would make this easier to debug: could the "Rust Analyzer Language Server" output just always print the rustfmt output when there was a rustfmt error (even without setting any env var or so)? That way at least there'd be a slightly more discoverable way to get to this error message, without having to figure out a nice UI to expose this this with in the regular editor.

@RalfJung
Copy link
Member Author

Oh, maybe there actually is a RA bug here: @ehuss points out that with --edition=2018 added, it works.

Now, I don't know if RA sees the same rustfmt error that I see in my manual invocation, so maybe this is all a red herring. Sadly the RA_LOG output neither prints the full rustfmt command that is executed nor the output it got back (as far as I could see).

@RalfJung
Copy link
Member Author

Yes, indeed that was it. :) This fixes the problem:

    "rust-analyzer.rustfmt.overrideCommand": [
        "./build/x86_64-unknown-linux-gnu/stage0/bin/rustfmt",
        "--edition=2018"
    ],

I guess so far I was just lucky that rustfmt in the rustc tree worked even with the 2015 edition? Thought it might make sense if RA added that flag even with overrideCommand set... it is very easy to forget the flag there (or just not know that it is required explicitly), and extremely hard to figure out what is going wrong later.

@lnicola
Copy link
Member

lnicola commented Sep 17, 2021

Yeah, sorry for that, it was execve and not execvp. And yes, rust-analyzer-platform is the rust-analyzer language server. Sometimes there's a proc macro server too (child of the main one), but the advice is to disable it for rust-lang/rust.

Sadly the RA_LOG output neither prints the full rustfmt command that is executed nor the output it got back

It does, but it's shortened:

[pid 760162] execve("./build/x86_64-unknown-linux-gnu/stage0/bin/rustfmt", ["./build/x86_64-unknown-linux-gnu"...], 0x5589ee66ab80 /* 82 vars */ <unfinished ...>

strace has a -s parameter that controls the size of the printed arguments, so adding e.g. -s 2048 should make it show the full command line.

Thought it might make sense if RA added that flag even with overrideCommand set

The extraArgs settings add to the command line, but the overrideCommand ones replace all of it.

But yeah, we could log the rustfmt invocations and/or errors.

@RalfJung
Copy link
Member Author

RalfJung commented Sep 17, 2021

It does, but it's shortened:

That is not RA_LOG output, that is strace output you quoted, which is a lot harder to get by.

The extraArgs settings add to the command line, but the overrideCommand ones replace all of it.

extraArgs doesn't help when you want to use a different rustfmt binary but still let RA control the flag passed to rustfmt.

@lnicola
Copy link
Member

lnicola commented Sep 17, 2021

Exactly. I guess the idea is that you might have a completely different formatter (not rustfmt).

But fixing the rustc dev guide should help.

@Veykril Veykril added the S-unactionable Issue requires feedback, design decisions or is blocked on other work label Dec 14, 2021
bors added a commit that referenced this issue Aug 19, 2022
Log rustfmt parsing errors as warnings

We unconditionally pass an edition parameter to rustfmt, for some crates
this might fail rustfmt so instead of swallowing the error, at least Log
it on a level that is logged by default so users won't be completely
confused about it.
See for context #10209

Closes #10209
@bors bors closed this as completed in 638755a Aug 19, 2022
tjdevries pushed a commit to tjdevries/rust-analyzer that referenced this issue Aug 22, 2022
We unconditionally pass an edition parameter to rustfmt, for some crates
this might fail rustfmt so instead of swallowing the error, at least Log
it on a level that is logged by default so users won't be completely
confused about it.
See for context rust-lang#10209

Closes rust-lang#10209
pocket7878 pushed a commit to pocket7878/rust-analyzer that referenced this issue Aug 31, 2022
We unconditionally pass an edition parameter to rustfmt, for some crates
this might fail rustfmt so instead of swallowing the error, at least Log
it on a level that is logged by default so users won't be completely
confused about it.
See for context rust-lang#10209

Closes rust-lang#10209
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: bug S-unactionable Issue requires feedback, design decisions or is blocked on other work
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants