Better error message

[appetrosyan]

I ran into a problem that was resolved by looking at #2057. It took longer than I'd like and the error message is not too helpful.

Currently if the project doesn't have rustfmt.toml in the workspace, cargo fmt checks all parent directories for a configuration file. This is not the expected behaviour, because as a user of Rust one assumes that going to the parent ends at the nearest workspace (and workspaces can't be nested). As such it is unclear which configuration file failed to parse.

So, the error message that looks like this,

Error: Decoding config file failed:
invalid type: integer `2021`, expected string
in `edition`

Please check your config file.

Should really read as more like this:

The file `/home/user/git/rustfmt.toml` failed to parse. 
Invalid type: Integer `2021`, expected string 

Please check `/home/user/git/rustfmt.toml`. 

Ideally, of course the error message can be brought in line with the quality of rustc errors, looking more like

The file `/home/user/git/rustfmt.toml` failed to parse. 
The `edition` variable has the wrong type: expected string, got 2021 (Integer). 

21: edition = 2021 

help: Add double quotes around `2021` in `/home/user/git/rustfmt.toml:21`. 

[ytmimi]

Great suggestion. I think it should be easy enough to add the file name to the error message. PRs are welcome!

rustfmt/src/config/mod.rs

Lines 405 to 409 in 46cb7d3

	Err(e) => {
	err.push_str("Error: Decoding config file failed:\n");
	err.push_str(format!("{e}\n").as_str());
	err.push_str("Please check your config file.");
	Err(err)

[rufevean]

hey, can you assign me this, i will work on this and update you

[rufevean]

            Err(e) => {
                let mut err_msg = String::new();

                if let Some(file_path) = e.file_path() {
                    err_msg.push_str(&format!("Error parsing config file `{}`:\n", file_path));
                } else {
                    err_msg.push_str("Error parsing config file:\n");
                }

                err_msg.push_str(&format!("  {}\n", e));

                if let Some(line_number) = e.line_number() {
                    err_msg.push_str(&format!(
                        "At line {}: {}. Check your config file for syntax issues.\n",
                        line_number, e
                    ));
                } else {
                    err_msg.push_str("Please check your config file for syntax errors.\n");
                }

                Err(err_msg)
            }

I am thinking of something like this, let me know if i am wrong please, thank you

[ytmimi]

@rufevean you can assign yourself by commenting @rustbot claim.

Does the error provide methods to get the file_path and line_number like you're suggesting? I suspect it'll be easiest to focus our effort on displaying the file path. The function gets a path to the directory so we could simply direct users to check their config file at {dir}.

[rufevean]

@rustbot claim

[rufevean]

The file `/home/rufevean/shitbox/open/rustfmt-error-test/rustfmt.toml` failed to parse.
Error details: invalid type: integer `2019`, expected string
in `edition`

Help: Ensure that the configuration file at `/home/rufevean/shitbox/open/rustfmt-error-test/rustfmt.toml` is correctly formatted

i am getting error response like this, will this work?
and this is the code

Err(e) => {
    let project_dir = match std::env::current_dir() {
        Ok(path) => path,
        Err(_) => std::path::PathBuf::from("."),
    };

    let config_file_path = project_dir.join("rustfmt.toml");

    let err_msg = format!(
        "The file `{}` failed to parse.\n\
        Error details: {}\n\
        Help: Ensure that the configuration file at `{}` is correctly formatted.",
        config_file_path.display(),
        e,
        config_file_path.display()
    );
    
    Err(err_msg)
}

please let me know if am doing something wrong , thank you

[appetrosyan]

There’s colour support in rustfmt correct? You could try adding them in line with the Rustc colour palette

[ytmimi]

i am getting error response like this, will this work?
and this is the code

Err(e) => {
   let project_dir = match std::env::current_dir() {
       Ok(path) => path,
       Err(_) => std::path::PathBuf::from("."),
   };

   let config_file_path = project_dir.join("rustfmt.toml");

   let err_msg = format!(
       "The file `{}` failed to parse.\n\
       Error details: {}\n\
       Help: Ensure that the configuration file at `{}` is correctly formatted.",
       config_file_path.display(),
       e,
       config_file_path.display()
   );
   
   Err(err_msg)
}

@rufevean no I don't think this will work. You can't assume that the rustfmt.toml file is located within the current directory, and you also can't assume that the file is named rustfmt.toml. rustfmt will also search for a file named .rustfmt.toml.

The file path that we want to display should be available further up the call stack, It might make more sense to pass the complete path to the file down instead of the directory where the file is found. That should make it much easier to get the correct file.

@appetrosyan yes there is support to display formatting errors with color, but I suspect it'll be a bit harder to add support for that in this case. IMO it'll be best to focus on the simpler problem, which is to enhance the error message to direct users to the config that rustfmt is failing to parse.

Work to make the error message prettier can be done as a follow up.

[rufevean]

Err(e) => {
    let config_file_path = match get_toml_path(Path::new(".")) {
        Ok(Some(path)) => path.display().to_string(),
        Ok(None) | Err(_) => "unknown".to_string(),
    };

    let err_msg = format!(
        "The file `{}` failed to parse.\n\
         Error details: {}\n\
         Help: Ensure that the configuration file at  `{}` is correctly formatted.",
        config_file_path,
        e,
        config_file_path
    );

    Err(err_msg)
}

``` , i have achieved same result with this code, where i have used a method which is in the same file .

[ytmimi]

@rufevean I appreciate that you've continued to look into this. I don't think calling get_toml_path again from the code on the error path is the correct approach. I'm still of the opinion that passing the file path down from the caller would be best.

As your code demonstrates calling get_toml_path may fail to resolve a config file, and I don't think telling the end user that The file `unknown` failed to parse would be a very helpful error message.

[rufevean]

thank you for your patience, and by

. I'm still of the opinion that passing the file path down from the caller would be best

you mean, directly taking file path as an arguement? and we want to point out the file rather than just the directory , right? because if its just the directory, we can acheive that by simple using {dir} arugement that is already taken

[ytmimi]

you mean, directly taking file path as an arguement? and we want to point out the file rather than just the directory , right? because if its just the directory, we can acheive that by simple using {dir} arugement that is already taken

Yes that's what I mean.

I don't see why we couldn't change the code as follows:
(NOTE we'd need to update all the callsites to provide the file path)

     pub(crate) fn from_toml_for_style_edition(
         toml: &str,
-        dir: &Path,
+        file_path: &Path,
         edition: Option<Edition>,
         style_edition: Option<StyleEdition>,
         version: Option<Version>,
     ) -> Result<Config, String> {

[rufevean]

i am getting this output

rustfmt-error-test git:master ❯ cargo fmt                                                 ✭
The file `/home/rufevean/shitbox/open/rustfmt-error-test/.rustfmt.toml` failed to parse.
Error details: invalid type: integer `2019`, expected string
in `edition`

Help: Ensure that the configuration file at `/home/rufevean/shitbox/open/rustfmt-error-test/.rustfmt.toml` is correctly formatted.

and as you suggested, i added file_path as an argument and also added that to other connecting functions wherever an error was occurring, and as the file_path is added i have simply displayed it with this code

Err(e) => {
    
let config_file_path_str = file_path.to_string_lossy(); 

        let err_msg = format!(
            "The file `{}` failed to parse.\n\
             Error details: {}\n\
             Help: Ensure that the configuration file at `{}` is correctly formatted.",
            config_file_path_str,
            e,
            config_file_path_str
        );

        Err(err_msg)
    }    

[ytmimi]

@rufevean feel free to open a PR with the approach you've mentioned. I'll leave any additional comments there.

[rufevean]

hey, i have started a pull request but a major error occurred while running the tests, i have to add an argument of file_path everywhere config::from_toml() is invoked, adding an empty path is a viable approach or is there any better alternative?
#6310

[ytmimi]

I don't think so. If you're changing the function signature then you also need to update all of the invocations.