Skip to content

opt(err): show renaming file error source #4414

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

Merged
merged 1 commit into from
Jul 16, 2025

Conversation

Binlogo
Copy link
Contributor

@Binlogo Binlogo commented Jul 15, 2025

Problem you are trying to solve

We occasionally encounter the following error:

error: component download failed for cargo-aarch64-apple-darwin: could not rename downloaded file from '/Users/xxx/.rustup/downloads/344e710d......76ee75be1fff0c7bf3.partial' to '/Users/xxx/.rustup/downloads/344e710d......76ee75be1fff0c7bf3'

This issue is difficult to reproduce, so it's important to add a source for reporting first.

Description

This pull request introduces a RenamingFile error with a source to improve error handling for file renaming operations in the RustupError enum. It updates the codebase to use this new error type, enhancing the clarity and consistency of error reporting.

Copy link
Contributor

@djc djc left a comment

Choose a reason for hiding this comment

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

Should the std::error::Error::source() method be implemented to yield the source error for this?

@Binlogo
Copy link
Contributor Author

Binlogo commented Jul 15, 2025

Should the std::error::Error::source() method be implemented to provide the source error for this?

@djc Thanks for asking, but I'm not sure, actually. I noticed that SettingPermissions is using io::Error here, so I followed the same pattern. Do you have any suggestions?

@Binlogo
Copy link
Contributor Author

Binlogo commented Jul 15, 2025

Should the std::error::Error::source() method be implemented to yield the source error for this?

According to the thiserror documentation, it states:

The Error trait’s source() method is implemented to return whichever field has a #[source] attribute or is named source, if any. This is for identifying the underlying lower level error that caused your error.

The #[from] attribute always implies that the same field is #[source], so you don’t ever need to specify both attributes.

Any error type that implements std::error::Error or dereferences to dyn std::error::Error will work as a source.

#[derive(Error, Debug)]
pub struct MyError {
    msg: String,
    #[source]  // optional if field name is `source`
    source: anyhow::Error,
}

Is it still necessary to implement the std::error::Error::source() method?

@Binlogo Binlogo requested a review from djc July 15, 2025 13:03
@djc
Copy link
Contributor

djc commented Jul 15, 2025

Sure, but your PR does not add any #[source] attribute, right?

@Binlogo
Copy link
Contributor Author

Binlogo commented Jul 15, 2025

Sure, but your PR doesn't add any #[source] attribute, right?

This PR didn't add #[source] because this macro is optional if the field name is source, as the documentation mentions.

So it should implemented std::error::Error::source() method by thiserror crate.

@djc
Copy link
Contributor

djc commented Jul 15, 2025

Sure, but your PR doesn't add any #[source] attribute, right?

This PR didn't add #[source] because this macro is optional if the field name is source, as the documentation mentions.

So it should implemented std::error::Error::source() method by thiserror crate.

Ahh, okay, thanks.

@Binlogo Binlogo force-pushed the opt/rename-error branch from 7251ef7 to 4ce57fd Compare July 16, 2025 00:20
Copy link
Member

@rami3l rami3l left a comment

Choose a reason for hiding this comment

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

Looks nice to me, thanks!

@djc Do you happen to have remaining concerns or all of them have been addressed already?

@djc djc added this pull request to the merge queue Jul 16, 2025
@djc djc removed this pull request from the merge queue due to a manual request Jul 16, 2025
@djc
Copy link
Contributor

djc commented Jul 16, 2025

@Binlogo just squash your commits please -- the second commit really belongs with the first one.

chore: fix ci windows build issue
@Binlogo Binlogo force-pushed the opt/rename-error branch from 4ce57fd to bc90f80 Compare July 16, 2025 08:51
@Binlogo
Copy link
Contributor Author

Binlogo commented Jul 16, 2025

@djc Squashed. Thanks for the review.

@djc djc enabled auto-merge July 16, 2025 08:55
@djc djc added this pull request to the merge queue Jul 16, 2025
Merged via the queue into rust-lang:master with commit 792598c Jul 16, 2025
29 checks passed
@Binlogo Binlogo deleted the opt/rename-error branch July 16, 2025 13:12
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.

3 participants