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

refactor - improved error regarding issue "Better error message #6302" #6310

Closed
wants to merge 0 commits into from

Conversation

rufevean
Copy link
Contributor

@rufevean rufevean commented Sep 1, 2024

closes #6302

@rufevean rufevean marked this pull request as draft September 1, 2024 16:29
@rufevean rufevean mentioned this pull request Sep 1, 2024
Comment on lines 374 to 375
pub(super) fn from_toml(toml: &str, dir: &Path,file_path : &Path) -> Result<Config, String> {
Self::from_toml_for_style_edition(toml, dir,file_path, None, None, None)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to pass both dir and file_path. The dir can easily be derived from the file path.

Comment on lines 380 to 373
dir: &Path,
file_path: &Path,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same goes for this. We don't need to pass both the dir and file_path. We can derive the dir from the file_path.

@rufevean
Copy link
Contributor Author

rufevean commented Sep 1, 2024

i cant figure out why my tests are failing, could you please guide me through this?

@ding-young
Copy link
Contributor

i cant figure out why my tests are failing, could you please guide me through this?

It looks like the self-tests didn't pass. Since it's important for the rustfmt source code to adhere to the same formatting style that it enforces, the self-test runs rustfmt on its own codebase. You can find more details in the Contributing.md.

To resolve this, you might want to try running cargo run --bin rustfmt /path/to/modified/source.rs to format the modified source code using the rustfmt built from the source. After that, you can check if the tests pass locally by running cargo test.

Hope this helps!

@rufevean rufevean marked this pull request as ready for review September 3, 2024 06:49
@rufevean
Copy link
Contributor Author

rufevean commented Sep 3, 2024

@ytmimi i think its ready for review now, thank you

Copy link
Contributor

@ytmimi ytmimi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for continuing to look into this. I've left a few comments inline. Also, I think it would be great if we could add a unit test similar to those defined here, where we call the rustfmt binary and check the content of stdout / stderr.

In this case we can create a new config file in tests/config that will trigger the error, and we can call rustfmt with the --config-path argument and validate that we see our expected error message in stderr.

Also, one more note. The rustfmt team strongly discourages merge commits. Please rebase your branch to bring it up to date when you get a chance.

Comment on lines 403 to 397
Ok(parsed_config.to_parsed_config(style_edition, edition, version, dir))
Ok(parsed_config.to_parsed_config(style_edition, edition, version, file_path))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you double check what PartialConfig::to_parsed_config does with the dir argument. Although the types match up before the method expected a directory path and now we're passing a file path so that could change behavior. We probably need to derive the dir from the file path here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct me if i am wrong, to_parsed_config() only uses dir arguement for fill_from_parsed_config(self,dir)

    pub(super) fn to_parsed_config(
        self,
        style_edition_override: Option<StyleEdition>,
        edition_override: Option<Edition>,
        version_override: Option<Version>,
        dir: &Path,
    ) -> Config {
        Config::default_for_possible_style_edition(
            style_edition_override.or(self.style_edition),
            edition_override.or(self.edition),
            version_override.or(self.version),
        )
        .fill_from_parsed_config(self, dir)
    }
}

and

        fn fill_from_parsed_config(mut self, parsed: PartialConfig, dir: &Path) -> Config {
        $(
            if let Some(option_value) = parsed.$i {
                let option_stable = self.$i.3;
                if $crate::config::config_type::is_stable_option_and_value(
                    stringify!($i), option_stable, &option_value
                ) {
                    self.$i.1 = true;
                    self.$i.2 = option_value;
                }
            }
        )+
            self.set_heuristics();
            self.set_ignore(dir);
            self.set_merge_imports();
            self.set_fn_args_layout();
            self.set_hide_parse_errors();
            self.set_version();
            self
        }

The dir parameter is used only in the self.set_ignore(dir) method call, i think its to specifiy which files or directories to ignore based on the provided directory path.

Copy link
Contributor

@ytmimi ytmimi Sep 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into this. Given that code down the stack needs the dir we need to pass that along.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok(parsed_config.to_parsed_config(style_edition, edition, version, file_path.parent().unwrap_or(Path::new(""))))

can we refactor that statement to this so that we can also pass dir ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine to just call file_path.parent().unwrap() since that's what from_toml_path did before your change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but if its unwrap() . wont we pass a None value if its empty? and isnt that an error?

Copy link
Contributor Author

@rufevean rufevean Sep 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i have followed your approach and tried just using unwrap() and then tested it, i got this output


---- config::test::test_unstable_from_toml stdout ----
thread 'config::test::test_unstable_from_toml' panicked at src/config/mod.rs:401:40:
called `Option::unwrap()` on a `None` value

---- config::test::deprecated_option_merge_imports::test_new_overridden stdout ----
thread 'config::test::deprecated_option_merge_imports::test_new_overridden' panicked at src/config/mod.rs:401:40:
called `Option::unwrap()` on a `None` value

---- config::test::deprecated_option_merge_imports::test_both_set stdout ----
thread 'config::test::deprecated_option_merge_imports::test_both_set' panicked at src/config/mod.rs:401:40:
called `Option::unwrap()` on a `None` value
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- config::test::deprecated_option_merge_imports::test_old_overridden stdout ----
thread 'config::test::deprecated_option_merge_imports::test_old_overridden' panicked at src/config/mod.rs:401:40:
called `Option::unwrap()` on a `None` value

---- config::test::deprecated_option_merge_imports::test_old_option_set stdout ----
thread 'config::test::deprecated_option_merge_imports::test_old_option_set' panicked at src/config/mod.rs:401:40:
called `Option::unwrap()` on a `None` value

---- config::test::test_was_set stdout ----
thread 'config::test::test_was_set' panicked at src/config/mod.rs:401:40:
called `Option::unwrap()` on a `None` value

---- config::test::use_small_heuristics::test_array_width_config_exceeds_max_width stdout ----
thread 'config::test::use_small_heuristics::test_array_width_config_exceeds_max_width' panicked at src/config/mod.rs:401:40:
called `Option::unwrap()` on a `None` value

---- config::test::use_small_heuristics::test_attr_fn_like_width_config_exceeds_max_width stdout ----
thread 'config::test::use_small_heuristics::test_attr_fn_like_width_config_exceeds_max_width' panicked at src/config/mod.rs:401:40:
called `Option::unwrap()` on a `None` value

---- config::test::use_small_heuristics::test_chain_width_config_exceeds_max_width stdout ----
thread 'config::test::use_small_heuristics::test_chain_width_config_exceeds_max_width' panicked at src/config/mod.rs:401:40:
called `Option::unwrap()` on a `None` value

Am i missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ytmimi , what approach should i prefer here?

Comment on lines 400 to 409
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should still add the err_msg to the err String and return that since it might contain additional error details. Also, I think we can simplify the error message as follows since you've already told the user the path to the file on the first line:

                let err_msg = format!(
                    "The file `{}` failed to parse.\n    Error details: {e}",
                    file_path.display()
                );

                err.push_str(&err_msg)
                Err(err)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes i agree that your approach is much better and is also working, i have changed the code accordingly

@rufevean
Copy link
Contributor Author

rufevean commented Sep 4, 2024

Thanks for continuing to look into this. I've left a few comments inline. Also, I think it would be great if we could add a unit test similar to those defined here, where we call the rustfmt binary and check the content of stdout / stderr.

In this case we can create a new config file in tests/config that will trigger the error, and we can call rustfmt with the --config-path argument and validate that we see our expected error message in stderr.

okay, so we have to write a test case in tests/rustfmt/main.rs that takes the file we create in tests/config which is faulty and we should expect the output as an error, right?

Also, one more note. The rustfmt team strongly discourages merge commits. Please rebase your branch to bring it up to date when you get a chance.

would it be better if i do once we have finalized all the refactoring?

@ytmimi
Copy link
Contributor

ytmimi commented Sep 4, 2024

okay, so we have to write a test case in tests/rustfmt/main.rs that takes the file we create in tests/config which is faulty and we should expect the output as an error, right?

Yeah, thats right.

would it be better if i do once we have finalized all the refactoring?

Sure. Just wanted to make you aware of our stance on merge commits.

@rufevean
Copy link
Contributor Author

rufevean commented Sep 5, 2024

@ytmimi , why trying to test this issue, i faced two issues

  1. when we raise the error , we get
The file `/home/rufevean/shitbox/rustfmt/tests/config/issue-6302.toml` failed to parse.
Error details: invalid type: integer `2019`, expected string
in `edition`

we get the entire path . so we cant test it that way as the absolute path will change for every person. so i think we might have to refactor the code where it will only present the path from the main project directory?

  1. in the error display
The file `/home/rufevean/shitbox/rustfmt/tests/config/issue-6302.toml` failed to parse.
Error details: invalid type: integer `2019`, expected string
 in `edition`

the in edition part is forming a new line , to track that issue we might have to tract the {e} directly as of my knowledge, we didnt specify anything that makes it in a new line.

@ytmimi
Copy link
Contributor

ytmimi commented Sep 5, 2024

we get the entire path . so we cant test it that way as the absolute path will change for every person. so i think we might have to refactor the code where it will only present the path from the main project directory?

How exactly are you trying to test this? I think it should be doable. If you could add a commit with your test code that would be helpful for me to review and provide some suggestions.

the in edition part is forming a new line , to track that issue we might have to tract the {e} directly as of my knowledge, we didnt specify anything that makes it in a new line.

If you look at the original issue the in edition was also on a newline. It's maybe not that visually appealing, but I don't think it's something we need to address right now.

@rufevean
Copy link
Contributor Author

rufevean commented Sep 6, 2024

@ytmimi , i have added the commit, could you please check it and let me know where i am getting it wrong?

Comment on lines 222 to 234
#[test]
fn rustfmt_error_improvement_regarding_invalid_toml() {
// See also https://github.com/rust-lang/rustfmt/issues/6302
let args = ["--config-path", "tests/config/issue-6302.toml"];
let (_stdout, stderr) = rustfmt(&args);

let expected_error_message = "The file `rustfmt/tests/config/issue-6302.toml` failed to parse.\n Error details: invalid type: integer `2019`, expected string in `edition`";

assert!(
stderr.contains(expected_error_message),
"Unexpected error message.\nExpected:\n{}\nGot:\n{}",
expected_error_message,
stderr
);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: add a space between the test and the one above it.

Here's how you can write the test to pass:

#[test]
fn rustfmt_error_improvement_regarding_invalid_toml() {
    // See also https://github.com/rust-lang/rustfmt/issues/6302
    let invalid_toml_config = "tests/config/issue-6302.toml";
    let args = ["--config-path", invalid_toml_config];
    let (_stdout, stderr) = rustfmt(&args);

    let toml_path = Path::new(invalid_toml_config).canonicalize().unwrap();
    let expected_error_message = format!("The file `{}` failed to parse", toml_path.display());

    assert!(stderr.contains(&expected_error_message));
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i have updated it , thank you

ytmimi
ytmimi previously approved these changes Sep 9, 2024
Copy link
Contributor

@ytmimi ytmimi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if you'd like to squash the commits / rebase the PR or if you'd rather I just squash the commits on the merge.

@rufevean
Copy link
Contributor Author

rufevean commented Sep 9, 2024

Let me know if you'd like to squash the commits / rebase the PR or if you'd rather I just squash the commits on the merge.

i had left a comment regarding Ok(parsed_config.to_parsed_config(style_edition, edition, version, file_path.parent().unwrap_or(Path::new("")))) and i would like to know how do you want me to proceed, and once i have refactored it according to how you think is better. i will rebase it . thank you

Comment on lines 397 to 402
Ok(parsed_config.to_parsed_config(
style_edition,
edition,
version,
file_path.parent().unwrap_or(Path::new("")),
))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in #6310 (comment) the previous logic was to unwrap, which is going to panic if we can't get a directory from the file, and i think that's fine (and probably very unlikely to panic in practice).

However we can also just return early with an error:

let dir = file_path.patent()
    .ok_or_else(|| format!("failed to get parent directory for {}", file_path.display))?;

Ok(parsed_config.to_parsed_config(
    style_edition,
    edition,
    version,
    dir,
))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i have pushed the code now, if the tests past , i will then rebase the code and i think we are good to merge after that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the tests failed

Copy link
Contributor

@ytmimi ytmimi Sep 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you likely need to update all the tests that just pass Path::new(""). to Config::from_toml since we've updated the semantics to pass a file path instead.

let config = Config::from_toml(toml, Path::new("")).unwrap();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so i need to update tests in config dir ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rufevean I'd recommend you look into the details of the CI failures, and fix the impacted tests. If it turns out to be more involved than what I just mentioned let me know.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it, thank you

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ytmimi , i dont know if this is the right approach but i changed
let config = Config::from_toml(toml, Path::new("")).unwrap();
to
let config = Config::from_toml(toml, Path::new(".")).unwrap();
to have current dir as parent dir? and tests got passed

let config = Config::from_toml("hard_tabs = true", Path::new("")).unwrap();
let config = Config::from_toml("hard_tabs = true", Path::new(".")).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's change this to a file path to better convey that from_toml now expects a file path instead of a directory.

        let config = Config::from_toml("hard_tabs = true", Path::new("./rustfmt.toml")).unwrap();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should i do this for every test that takes from_toml or just this?

Copy link
Contributor

@ytmimi ytmimi Sep 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of them please

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated , thank you

@ytmimi ytmimi dismissed their stale review September 10, 2024 16:03

Turns out there were still a few things that we're working on. Dismissing the old review for now

@rufevean
Copy link
Contributor Author

@ytmimi can we rebase and merge this now?

Copy link
Contributor

@ytmimi ytmimi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rufevean yes, this should be ready now. I'll give you an opportunity to rebase this PR and squash the commits if you'd like to clean up the final commit or I can take care of it. Let me know what you'd prefer.

@ytmimi ytmimi added the release-notes Needs an associated changelog entry label Sep 12, 2024
@rufevean
Copy link
Contributor Author

@ytmimi i think i did something wrong, i rebased it and squashed all my commits into 1. but i can see other's commits here .

@ytmimi
Copy link
Contributor

ytmimi commented Sep 12, 2024

Yeah, definitely doesn't look right. You should be able to get back to the commit before the rebase if you check the git reflog

@rufevean
Copy link
Contributor Author

@ytmimi , i tried squashing it again and deleting the commits that i dont want to include but even then there are some conflicts . its better if you may handle it. thank you

@ytmimi
Copy link
Contributor

ytmimi commented Sep 12, 2024

hmm weird, when I tried to update your branch GitHub marked me as closing the PR 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better error message
4 participants