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

Show OS error in the Display impl on Windows #133

Closed
bjorn3 opened this issue Jan 9, 2024 · 4 comments
Closed

Show OS error in the Display impl on Windows #133

bjorn3 opened this issue Jan 9, 2024 · 4 comments

Comments

@bjorn3
Copy link

bjorn3 commented Jan 9, 2024

Currently the OS error is ignored at

LoadLibraryExW { .. } => write!(f, "LoadLibraryExW failed"),
and several other of the error variants, making it hard to figure out what the actual error was.

@nagisa
Copy link
Owner

nagisa commented Jan 9, 2024

There's an implementation of Error::source you can use to get to the source error.

EDIT: this is I think is just the latest in the unending war-of-tug between the camps of "format the source error into Display" vs. "leave the presentation concerns to the end user". rustc in fact seems like a perfect example of a codebase which would benefit from being able to display its errors in whatever way it wishes to given its strong focus on user-friendly diagnostics, ability to output them as JSON, etc.

@bjorn3
Copy link
Author

bjorn3 commented Jan 9, 2024

For all other error variants the actual OS error is printed in the Display impl. Also for rustc loading a dynamic library almost never fails. Proc macros are not dynamically linked against anything else and rustc verifies in advance that they were compiled by the right rustc version, so loading them should never fail. And in general I don't think rustc can really show pretty errors for loading dynamic libraries as the error causes are outside of the control of rustc and pretty diverse. All it can really do is print all information it has and hope someone can understand the root cause from it.

@nagisa
Copy link
Owner

nagisa commented Jan 9, 2024

Here are the guidelines I try to follow when I (and many others) implement Error in libraries:

  1. If an user implements their own presentation level for error sources, there isn’t a situation where my library displays the same error twice, think something along the lines of

    error: a very bad thing has happened: this other bad thing: welp
      caused by: this other bad thing: welp
      caused by: welp
    

    which is caused precisely by formatting source errors in error’s own Display implementation.

  2. If there is an error user might be able to introspect and handle, it is made available to them via source (this does not preclude making the source error available as a public field or some other means as well.)

With that in mind…

For all other error variants the actual OS error is printed in the Display impl.

dlerror happens to return a plain string rather than a proper error code, which although we do store in DlDescription for encapsulation reasons, there isn’t any point in making it available for downcasting (or any other reason) through source. On the other hand, Windows errors are proper errors and I can see all the reasons somebody might want to handle some of them specially. That’s why they are exposed through source. Since they are, the guideline 1 above prevents formatting its contents in the impl Display for Error – otherwise there would be duplicates of the error message.

If the consistency was absolutely required I would see making DlDescription an Error and exposing it as a source a less evil alternative here. With or without such change, rustc would need to be adjusted to print out the source somehow.

The only other way I can see to avoid changing rustc is to adopt an ecosystem-wide convention that involves the # formatting flag to somehow indicate whether the user wants to see the sources printed or not. But as far as I know the initiatives along these lines never made very much progress.

@bjorn3
Copy link
Author

bjorn3 commented Jan 22, 2024

Closing. rust-lang/rust#119815 has fixed this on the rustc side.

@bjorn3 bjorn3 closed this as completed Jan 22, 2024
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

No branches or pull requests

2 participants