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

Builder pattern allows invalid usage #163

Open
Timbals opened this issue Jul 23, 2024 · 5 comments · May be fixed by #166
Open

Builder pattern allows invalid usage #163

Timbals opened this issue Jul 23, 2024 · 5 comments · May be fixed by #166

Comments

@Timbals
Copy link
Contributor

Timbals commented Jul 23, 2024

The generated builders zero out all fields other than ty when created.
Some structures aren't valid with zeroed out fields, but they can be submitted to functions anyway.
Some examples:

  • XrCompositionLayerProjection::viewCount must be greater than 0.
  • XrCompositionLayerProjection::space must be a valid XrSpace handle.
  • XrInteractionProfileDpadBindingEXT::actionSet must be a valid XrActionSet handle
  • generally everything that includes a handle

The solution to this is probably to take all required fields as arguments to the new functions instead of using a builder.
I will work on a PR for this.

@Ralith
Copy link
Owner

Ralith commented Jul 23, 2024

I'm not sure this is worth pursuing. Large argument lists can significantly degrade readability compared to populating struct fields, and IIRC OpenXR guarantees graceful error detection for this case, so there is no safety issue. We can't statically prevent erroneous inputs in general, so we should be wary of worsening ergonomics in exchange for handling a few marginal cases.

@Timbals
Copy link
Contributor Author

Timbals commented Jul 23, 2024

I think the spec doesn't guarantee graceful error detection:

Runtimes may detect XR_NULL_HANDLE and other invalid handles passed where a valid handle is required and return XR_ERROR_HANDLE_INVALID. However, runtimes are not required to do so unless otherwise specified, and so use of any invalid handle may result in undefined behavior.

But I agree that it may not be worth the extra complexity.
Because all these requirements are basically that some field wasn't filled, the validation layer seems to catch them anyway.

@Ralith
Copy link
Owner

Ralith commented Jul 23, 2024

If UB is possible in a safe API, then we need to at least insert such checks ourselves, or the API is unsound.

@Timbals
Copy link
Contributor Author

Timbals commented Jul 23, 2024

Adding asserts to all relevant functions seems like a good solution. It doesn't change the API.

Should I go through all structs/functions and create a PR?

@Ralith
Copy link
Owner

Ralith commented Jul 23, 2024

That would be very welcome! Note that handling this correctly for FrameStream::end in particular may be tricky as we don't statically know the input types. Might be easiest to just mark that one unsafe, or redesign its API somehow; perhaps a builder pattern for the call itself, with a generic method to add new layers that knows how to validate them?

@Timbals Timbals linked a pull request Jul 24, 2024 that will close this issue
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 a pull request may close this issue.

2 participants