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

Single type for all validation errors #2194

Merged
merged 12 commits into from
May 5, 2023
Merged

Conversation

Rua
Copy link
Contributor

@Rua Rua commented Apr 24, 2023

Changelog:

### Breaking changes

Changes to error reporting:
- `VulkanError` is renamed to `RuntimeError`.
- Added new types `ValidationError` and `VulkanError` (enum of `ValidationError` + `RuntimeError`) to return errors from any function. Some existing functions have been converted to use these types, others will follow later.

After some Discord discussion earlier today, this PR proposes a new way to report validation errors to the user. I've replaced the old error type in compute pipeline creation, as an example to show how it looks in practice. If accepted, this change would be made across the board, but we can do it piecemeal, it doesn't have to be all at once.

The enums that Vulkano currently uses to report errors have many problems:

  • Users are very unlikely to want to match on any of the enum variants. These are validation errors, if they happen then they are bugs in the user's code.
  • When you unwrap an error, which most people will do, then you only get the debug print, which is far less helpful than the display print.
  • They are tedious to create: You have to define a new enum, think of a name, then implement various traits.
  • They are tedious to extend: You have to add new variants each time a new error type is added, and then also update the Display implementation to add a friendly message for the user. And again, you have to think of names for all of these variants.
  • Their definitions are elsewhere in the code from where the error is triggered, meaning that you have to scroll back and forth whenever you need to change something.
  • There are many of them, in the many tens of different types.
  • Frankly, I'm fed up with writing enums!

Several years ago, I made a proposal to replace all validation errors in Vulkano with panics: #1546. This was rejected, but it seemed that people were on board with simplifying the error types if Vulkan kept the standard Result + Error error reporting. This implements that now.

@Rua Rua requested a review from AustinJ235 April 26, 2023 14:39
vulkano/src/lib.rs Outdated Show resolved Hide resolved
vulkano/src/lib.rs Outdated Show resolved Hide resolved
vulkano/src/lib.rs Outdated Show resolved Hide resolved
vulkano/src/lib.rs Outdated Show resolved Hide resolved
Rua and others added 8 commits May 5, 2023 11:30
Co-authored-by: marc0246 <40955683+marc0246@users.noreply.github.com>
Co-authored-by: marc0246 <40955683+marc0246@users.noreply.github.com>
Co-authored-by: marc0246 <40955683+marc0246@users.noreply.github.com>
@marc0246
Copy link
Contributor

marc0246 commented May 5, 2023

Mucho gracias.

@marc0246 marc0246 merged commit 59e0df3 into vulkano-rs:master May 5, 2023
marc0246 added a commit that referenced this pull request May 5, 2023
@Rua Rua deleted the new-err branch October 25, 2023 14:25
hakolao pushed a commit to hakolao/vulkano that referenced this pull request Feb 20, 2024
* Single type for all validation errors

* Documentation

* Small improvement

* Rename VulkanError to RuntimeError

* Simplify the error type by using an intermediate struct

* Update vulkano/src/lib.rs

Co-authored-by: marc0246 <40955683+marc0246@users.noreply.github.com>

* Update vulkano/src/lib.rs

Co-authored-by: marc0246 <40955683+marc0246@users.noreply.github.com>

* Update vulkano/src/lib.rs

Co-authored-by: marc0246 <40955683+marc0246@users.noreply.github.com>

* Better solution

* Revert to original state

---------

Co-authored-by: marc0246 <40955683+marc0246@users.noreply.github.com>
hakolao pushed a commit to hakolao/vulkano that referenced this pull request Feb 20, 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

Successfully merging this pull request may close these issues.

2 participants