Skip to content

Add ErrorMessage enum, start using it #355

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
Jun 21, 2025

Conversation

Manishearth
Copy link
Contributor

This is a step towards #113. I didn't finish converting all of the enums, just some of them to give an idea of what this might look like.

This error enum will be private for now: It's primarily encompassing messages, and will wrap all static string messages. The non-static ones I think we should convert to something else at the boundary, potentially logging them with a feature.

Once this is fully filled in, I plan to:

  • Replace the .msg field with the enum
  • Add to/from integer methods for the enum, exposed as helpers on TemporalError
  • Use the integer value in FFI TemporalError.

This is not primarily designed with user matching in mind: it can be matched by the user, but I don't think this is the right level of granularity. Instead, we can use this error message ontology to map to a more structured public error ontology if we want. Basically, in the medium term we can have a RustErrorKind that can be internally obtained from ErrorMessage, which we expose to the user.

Thoughts?

@Manishearth Manishearth changed the title Add ErrorMessage enum Add ErrorMessage enum, start using it Jun 18, 2025
@nekevss nekevss requested review from jedel1043 and nekevss June 18, 2025 14:47
Copy link
Member

@nekevss nekevss left a comment

Choose a reason for hiding this comment

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

Mostly curious what @jedel1043 thinks as he had a much more vocal stance.

I'm a bit mixed, but leaning towards this is fine. I think my mixed opinion comes mostly from it being an intermediary.

@Manishearth Manishearth force-pushed the errormessage branch 2 times, most recently from 674c3bc to 9132927 Compare June 20, 2025 17:09
@Manishearth
Copy link
Contributor Author

Yeah, it's an intermediary, but at the very least it's an intermediary that lets the error enum be Copy, which is super nice.

@jedel1043
Copy link
Member

Hmm, I'm thinking maybe this should be moved to temporal_capi instead, but I'm also thinking that it would be nice to have an enum representing all possible error messages, then put all the additional error data on TemporalError...

Yeah, I like this approach. Let's go with this and see how it would fit on the Rust side. We can still do more adjustments when we migrate the non-static error messages.

Copy link
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

Looks good!

@jedel1043 jedel1043 merged commit 029c5ba into boa-dev:main Jun 21, 2025
8 checks passed
@Manishearth Manishearth deleted the errormessage branch June 21, 2025 22:24
@Manishearth
Copy link
Contributor Author

Wonderful, I'll try and fill in the rest

nekevss added a commit that referenced this pull request Jun 24, 2025
This can even become non-optional after
#355 (maybe), but this allows V8
to start throwing readable errors without needing #355 to be landed
first.

Co-authored-by: Kevin Ness <[email protected]>
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