examples: Use slice::from_ref to not loose lifetime on nested slices#513
Merged
Conversation
As per the readme `.build()` should only be called as late as possible, and only if absolutely necessary; such cases include slices that are passed directly to functions. More precisely, such build calls and the creation of temporary slices should happen inside the same expression as the function call to be sound and completely lifetime-checked. This pattern of `&[my_builder.build()]` is however not possible when constructing intermediary Vulkan objects that reference the slice. In the first place this slice goes out of scope after the expression that creates the Vulkan object, which is caught and disallowed by rustc (unless this expression itself ends in `.build()`, which is completely unsound as it makes rustc unable to validate this lifetime dependency). In the second place - and as is most relevant for this patch that removes `.build()` calls that were not surrounded by temporary slice constructors - said expression drops the lifetime checks on anything held by `my_builder` which _could_ go out of scope before the newly constructed Vulkan object is used, resulting yet again in Undefined Behaviour. Fortunately, for slices of size 1 which are typical in Vulkan, `std::slice::as_ref` exists which is analogous to taking a pointer to an object and considering it an array of length 1 in C(++). This maintains the lifetime through `Deref` and makes rustc able to fully check all lifetimes and prevent unsound code. Albeit improving overall consistency, the `&[my_builder.build()]` pattern is not substituted in aforementioned Vulkan function-call expressions as that is considered "extraneous" [1] and demonstrates the various ways to safely construct Vulkan objects for the observant reader. [1]: #506 (comment)
Ralith
approved these changes
Dec 20, 2021
Ralith
left a comment
Collaborator
There was a problem hiding this comment.
IMO the safest pattern is to only ever construct objects lexically within Vulkan calls. Clear improvement here in any case.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
As per the readme
.build()should only be called as late as possible, and only if absolutely necessary; such cases include slices that are passed directly to functions. More precisely, such build calls and the creation of temporary slices should happen inside the same expression as the function call to be sound and completely lifetime-checked.This pattern of
&[my_builder.build()]is however not possible when constructing intermediary Vulkan objects that reference the slice. In the first place this slice goes out of scope after the expression that creates the Vulkan object, which is caught and disallowed by rustc (unless this expression itself ends in.build(), which is completely unsound as it makes rustc unable to validate this lifetime dependency).In the second place - and as is most relevant for this patch that removes
.build()calls that were not surrounded by temporary slice constructors - said expression drops the lifetime checks on anything held bymy_builderwhich could go out of scope before the newly constructed Vulkan object is used, resulting yet again in Undefined Behaviour.Fortunately, for slices of size 1 which are typical in Vulkan,
std::slice::as_refexists which is analogous to taking a pointer to an object and considering it an array of length 1 in C(++). This maintains the lifetime throughDerefand makes rustc able to fully check all lifetimes and prevent unsound code.Albeit improving overall consistency, the
&[my_builder.build()]pattern is not substituted in aforementioned Vulkan function-call expressions as that is considered "extraneous" 1 and demonstrates the various ways to safely construct Vulkan objects for the observant reader.