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

invoking rustfmt uses the project-wide rustfmt.toml, not the one for the current subcrate #15540

Closed
RalfJung opened this issue Sep 1, 2023 · 5 comments · Fixed by #15564
Closed
Labels
C-bug Category: bug

Comments

@RalfJung
Copy link
Member

RalfJung commented Sep 1, 2023

The rustc repository contains multiple rustfmt.toml files, for instance:

./rustfmt.toml
./src/tools/clippy/rustfmt.toml
./src/tools/miri/rustfmt.toml

So, when invoking formatting via RA in a file in src/tools/miri, I'd hope ./src/tools/miri/rustfmt.toml would be used. But unfortunately it always uses ./rustfmt.toml, which regularly leads to people accidentally formatting Miri files in their rustc PRs with the wrong settings.

This can be reproduced, for example, by opening src/tools/miri/src/borrow_tracker/tree_borrows/mod.rs and invoking Ctrl-Shift-I in vscode.

#7227 looks similar and is supposedly fixed, but I can still reproduce the problem on a recent RA. I'm not sure who is even doing the config file finding here -- is RA telling rustfmt "use this config file", or is rustfmt supposed to somehow figure this out itself? If the latter, how -- the working directory? AFAIK rustfmt doesn't get the actual filename, it just receives the text on stdin and produces the formatted text on stdout.

rust-analyzer version: rust-analyzer version: 0.3.1641-standalone

rustc version: rustc 1.74.0-nightly (59a829484 2023-08-30)

relevant settings:

{
    "rust-analyzer.rustc.source": "./Cargo.toml",
    "rust-analyzer.linkedProjects": [
        "Cargo.toml",
        "src/bootstrap/Cargo.toml",
        //"compiler/rustc_codegen_cranelift/Cargo.toml", // broken due to toolchain file
    ],
    "rust-analyzer.check.invocationLocation": "root",
    "rust-analyzer.check.invocationStrategy": "once",
    "rust-analyzer.check.overrideCommand": [
        "./x.sh",
        "check",
        "--json-output",
        "library/std",
        "compiler/rustc",
        "src/tools/miri",
        // "compiler/rustc_codegen_cranelift", // doesn't actually show any warnings
    ],
    "rust-analyzer.rustfmt.overrideCommand": [
        "./build/host/rustfmt/bin/rustfmt",
        "--edition=2021",
    ],
    // These just make everything way too slow.
    "rust-analyzer.cargo.buildScripts.enable": false,
    "rust-analyzer.procMacro.enable": false,
    // Some diagnostics don't work properly here
    "rust-analyzer.showUnlinkedFileNotification": false,
    "rust-analyzer.diagnostics.enable": false, // without proc macros, there's just too many false positives
    //
    "files.watcherExclude": {
        "*rustc*/src/llvm-project/**": true,
        "*rustc*/build/**": true,
    },
    "files.exclude": {
        "src/llvm-project/**": true,
        "build/**": true
    },
}
@RalfJung
Copy link
Member Author

RalfJung commented Sep 1, 2023

If I cd into src/tools/miri/src/borrow_tracker/tree_borrows and run rustfmt mod.rs, the right config file is picked up. So it looks like rustfmt is doing everything correctly?

@Veykril
Copy link
Member

Veykril commented Sep 1, 2023

Hmm, when we invoke rustfmt we actually set the working directory to the containing folder of the file we are trying to format, so it should just pick up the correct config,

// try to chdir to the file so we can respect `rustfmt.toml`
// FIXME: use `rustfmt --config-path` once
// https://github.com/rust-lang/rustfmt/issues/4660 gets fixed
match text_document.uri.to_file_path() {
Ok(mut path) => {
// pop off file name
if path.pop() && path.is_dir() {
cmd.current_dir(path);
}
}
Err(_) => {
tracing::error!(
"Unable to get file path for {}, rustfmt.toml might be ignored",
text_document.uri
);
}
}

@RalfJung
Copy link
Member Author

RalfJung commented Sep 1, 2023

It looks like you are only doing that when snap.config.rustfmt() returns RustfmtConfig::Rustfmt, but not when it returns RustfmtConfig::CustomCommand. As you can see in my config above I have a custom command set, so that might explain the problem?

@Veykril
Copy link
Member

Veykril commented Sep 1, 2023

Ah right, then that is in fact the issue.

@davidbarsky
Copy link
Contributor

@RalfJung as a quick heads up, I'm pretty sure I fixed this in #15564.

@bors bors closed this as completed in a843e46 Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: bug
Projects
None yet
3 participants