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

Separate content vs presentation for error types (need render method) #7865

Open
Ericson2314 opened this issue Feb 19, 2023 · 7 comments
Open
Labels
error-messages Confusing messages and better diagnostics

Comments

@Ericson2314
Copy link
Member

Ericson2314 commented Feb 19, 2023

Problem

Current our error infrastructure (Error and subclasses) store a format string. This is fine for one off ad-hoc errors, but a problem for structured errors. Those structured errors have various fields (so when caught data can be manipulated without having to parse a string), and those fields duplicate the information stored in the format string.

It also makes awkward attempts to display in the information in other ways, such as with JSON logging (where we might want to display the fields in an object, rather than just give the machine-unfriendly format string).

Solution

We really ought have a separate virtual method for rendering errors. Ad-hoc errors can still store the format string like today, and their render implementation would just display it. However structured errors would forego storing a format string entirely, and just do any string formatting that is needed on the fly as part of the render implementation.

This is much clearer, and makes structured errors (which we should encourage!) easier to use.

@Ericson2314 Ericson2314 changed the title WIP Render method for error types Separate content vs presentation for error types (need render method) Feb 19, 2023
@Ericson2314
Copy link
Member Author

Ericson2314 commented Feb 19, 2023

@bburdette Hi, haven't seen you in a while, but might you want to tackle this sort of thing?

@roberth roberth added the error-messages Confusing messages and better diagnostics label Feb 19, 2023
@roberth
Copy link
Member

roberth commented Feb 19, 2023

@Ericson2314 are you suggesting to store a richer representation of what's in the format string, or to render it last-minute based on exception-specific fields, or both?

@Ericson2314
Copy link
Member Author

@roberth I think just the latter? If we just have the hintformat field today (and probably aren't catching the thing in a nontrivial way) that's fine. But if we have the structured fields then we should be able get rid of the hintformat field.

It may be nice to also make all errors structured like GHC is in the process of doing, but I don't feel so urgent about that / it's a fine follow-up projec to this week if we are interested.

@roberth roberth added this to Nix team Feb 20, 2023
@bburdette
Copy link
Contributor

@bburdette Hi, haven't seen you in a while, but might you want to tackle this sort of thing?

This sounds interesting; perhaps a bit beyond the budget of nix-errors-enhancement right now. But maybe at some point? Do you have some examples of projects that depend on structured errors?

@Ericson2314
Copy link
Member Author

Ericson2314 commented Feb 20, 2023

This sounds interesting; perhaps a bit beyond the budget of nix-errors-enhancement right now. But maybe at some point?

Sure, sounds good. Maybe I'll start something can then the baton can be passed to you at some later point.

Do you have some examples of projects that depend on structured errors?

It's been nagging me for a while, but #7788 is what compelled me to write this issue. As you can see it is awkward writing the structured errors today.

@thufschmitt thufschmitt moved this to ⏰ Postponed in Nix team Feb 24, 2023
@fricklerhandwerk
Copy link
Contributor

fricklerhandwerk commented Feb 24, 2023

Triaged in the Nix team meeting:

  • @thufschmitt: sounds like good to have, but not pressing
  • @edolstra: sounds scary and like a huge opportunity for overengineering
  • @ericson: the idea is to store the data and generate the string dynamically
  • @edolstra: this is the Rust approach, and works for such a language, but in C++ it would be a verbose mess
  • @fricklerhandwerk: do we approve the general direction?
    • @edolstra: depends on how well it's implemented, and how much code churn it produces.
      • won't be any good if it has to rewrite every single error message
  • agreement: this is lower priority but we're not against it
  • postponed, waiting for a prototype

@nixos-discourse
Copy link

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-02-24-nix-team-meeting-minutes-35/25757/1

Ericson2314 added a commit to Ericson2314/nix that referenced this issue Feb 27, 2023
Ericson2314 added a commit to Ericson2314/nix that referenced this issue Apr 13, 2023
Ericson2314 added a commit to Ericson2314/nix that referenced this issue Apr 17, 2023
Ericson2314 added a commit to Ericson2314/nix that referenced this issue Apr 20, 2023
Ericson2314 added a commit to Ericson2314/nix that referenced this issue Jan 16, 2024
@thufschmitt thufschmitt removed this from Nix team Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
error-messages Confusing messages and better diagnostics
Projects
None yet
Development

No branches or pull requests

5 participants