Enable `unsafe_ops_in_unsafe_fn` lint in all workspaces #3044

ErichDonGubler · 2022-09-21T22:09:03Z

Soft dependency on #3184. Commits start at chore: `warn(unsafe_op_in_unsafe_fn)` for {deno_webgpu,player,wgpu-types}` .

This commit bundles all the trivial cases for enabling the unsafe_op_in_unsafe_fn lint added in Rust 1.52, at @jimblandy's encouragement. I recommend doing this review commit-by-commit, for the best experience. I've done my best to separate out line-heavy-but-mechanical changes from ones where logic is actually modified.

Open questions for reviewers:

What about large chains of unsafe ops?
Lots of pieces of wgpu-hal involve a sequence of commands to an unsafe backend. Adding unsafe { ... } adds a lot of noise in these cases.
- Idea: Leave them as-is; since individually justifying each operation may still be interesting.
- Idea: combine unsafe blocks for operations where the justification is, fundamentally, "We know this sequence of commands is fine".
- Idea: punt on transitioning wgpu-hal; maybe file follow-up work in an issue? Since wgpu-hal is an individual commit, it should be easy to remove, if needed.
What is the community's disposition on requiring SAFETY comments for unsafe blocks in the future? I'm going to guess that wgpu-hal would be too much effort to even contemplate for this right now, but maybe starting with wgpu/wgpu-core might be good?

Checklist

Run cargo clippy. (Making clippy more picky here, so ✅!)
Run RUSTFLAGS=--cfg=web_sys_unstable_apis cargo clippy --target wasm32-unknown-unknown if applicable.
Add change to CHANGELOG.md. See simple instructions inside file.

Connections
✅ Link to the issues addressed by this PR, or dependent PRs in other repositories

See above!

Description
✅ Describe what problem this is solving, and how it's solved.

The unsafe_op_in_unsafe_fn lint can be disallowed to require that unsafe expressions inside unsafe functions still have explicit unsafe blocks around them, rather than unsafe fn acting as a blanket unsafe block. This encourages maintainership of the project to minimize unsafe operation spans, which in turn minimizes the amount of suspect code when problems with unsoundness occur. It also encourages writers and reviewers to acknowledge of why unsafe operations are justified across the board; I'm hoping to follow up with increments on enforcing the justification of unsafe via the clippy::undocumented_unsafe_blocks lint (and maybe the clippy::missing_safety_doc lint) in select parts of the codebase.¹

Testing
✅ Explain how this change is tested.

Everything still compiles, including CI, so it should be fine here, so long as reviewing humans are happy with it! :)

Unfortunately, it's unlikely that we'll be justifying wgpu-hal in any significant percentage soon, but new code could use it, maybe? :) ↩

ErichDonGubler · 2022-10-10T14:06:19Z

Whoo, CI is finally green! This PR should be ready for review now (CC @jimblandy). I've tried noting the open questions I'm aware of. :)

cwfitzgerald

Thank you for taking on this massive change!

I am generally for this change, in a lot of cases it works quite well to highlight where things can actually go wrong. It does increase verbosity, but I think for the most part it's worth it. I think some things can be combined however - I made some comments about adjustments that I think would be worthwhile. But wait for jim's comments before doing anything :)

I think we should keep the wgpu-hal changes. # Safety blocks are useful for memory based stuff (pointer ops, etc) but for api based stuff it would be a lot of # Safety: the rest of the codebase lead up to this being safe :) so I don't think would be necessary,

wgpu-core/src/track/buffer.rs

wgpu-core/src/track/mod.rs

wgpu-hal/src/dx11/device.rs

wgpu-hal/src/dx12/mod.rs

wgpu-hal/src/dx12/view.rs

wgpu-hal/src/gles/adapter.rs

ErichDonGubler · 2022-10-13T01:28:19Z

@cwfitzgerald:

… # Safety blocks are useful for memory based stuff (pointer ops, etc) but for api based stuff it would be a lot of # Safety: the rest of the codebase lead up to this being safe :) so I don't think would be necessary, [sic]

I think I need to clarify: When you say # Safety blocks/sections, I think you're referring to linting doc comments of unsafe fns as future work? That's covered by the clippy::missing_safety_doc lint, which I haven't mentioned yet, but I like it. I've edited the OP to include it. 🙂 To be clear, when I said this in the PR OP:

What is the community's disposition on requiring SAFETY comments for unsafe blocks in the future? …

…

… I'm hoping to follow up with increments on enforcing the justification of unsafe via the clippy::undocumented_unsafe_blocks lint lint in select parts of the codebase.

…I'm referring to a different lint. The SAFETY comments I mentioned are for // SAFETY: <justification> code comments on unsafe blocks, rather than /// # Safety doc comments on unsafe fns. Here's a concrete snippet to illustrate:

#![deny(unsafe_op_in_unsafe_fn)] // We're adding this in this PR!

#[deny(undocumented_unsafe_blocks)] // What @ErichDonGubler suggested in the OP as follow-up work.
pub unsafe fn do_spooky_thing() {
	unsafe { *std::ptr::null_mut::<i32>() = 0 }; // Compile error, fires `undocumented_unsafe_blocks`.

	// SAFETY: We know what we're doing here, blah blah blah.
	unsafe { *std::ptr::null_mut::<i32>() = 0 }; // Works!
}

codecov-commenter · 2022-11-08T16:10:01Z

Codecov Report

Merging #3044 (5a2d2c5) into master (08b160c) will decrease coverage by 0.04%.
The diff coverage is 61.31%.

@@            Coverage Diff             @@
##           master    #3044       /-   ##
==========================================
- Coverage   64.70%   64.65%   -0.05%     
==========================================
  Files          81       81              
  Lines       38819    39343      524     
==========================================
  Hits        25117    25438      321     
- Misses      13702    13905      203

Impacted Files	Coverage Δ
player/src/lib.rs	`53.31% <ø> (ø)`
wgpu-core/src/device/mod.rs	`66.36% <0.00%> (-0.02%)`	⬇️
wgpu-core/src/instance.rs	`63.33% <0.00%> (ø)`
wgpu-core/src/lib.rs	`96.55% <ø> (ø)`
wgpu-hal/src/dx11/device.rs	`0.00% <0.00%> (ø)`
wgpu-hal/src/lib.rs	`26.21% <ø> (ø)`
wgpu-types/src/lib.rs	`88.04% <ø> (ø)`
wgpu/src/backend/direct.rs	`54.80% <0.00%> (-0.30%)`	⬇️
wgpu/src/lib.rs	`52.21% <0.00%> (-0.61%)`	⬇️
wgpu-hal/src/auxil/renderdoc.rs	`44.04% <17.64%> ( 0.14%)`	⬆️
... and 33 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

CHANGELOG.md

deno_webgpu/src/lib.rs

cwfitzgerald

I still have a couple small changes I would personally make, but this is a much better place to be in, and this is a PR ripe for catching merge conflicts, so lets merge and adjust from there :)

cwfitzgerald · 2022-11-09T21:56:26Z

CI is mad about the GL multithreaded example. This may be some combination of not our bug and has always been our bug, but this PR seems to reproduce it reliably.

…pes}` This commit bundles all of the trivial cases for enabling this lint, where no action is required beyond adding the lint.

Do the simplest mechanical work necessary to enable and satisfy this lint; only put down `unsafe` blocks, don't try to inspect for correctness or anything else.

Do the simplest mechanical work necessary to enable and satisfy this lint; only put down `unsafe` blocks, don't try to inspect for correctness or anything else. N.B.: that there _are_ some adjustments identified that could be made here, like breaking multiple individual `unsafe` operations into their `unsafe` spans. This is left for a follow-up commit.

Unsafe operations can be exhausting to validate by themselves. Therefore, it's often interesting to separate these out so we can justify each individual operation, and let a human reader accumulate and drop supporting safety context in the smallest increments necessary. To that end, this commit can pave the way for future work where we may do something like enable [`clippy::undocumented_unsafe_blocks`], which calls out `unsafe` blocks that do not have a `SAFETY` comment immediately above them. This commit only separates the operations in `unsafe` blocks I added in this same PR; I'm deliberately leaving existing `unsafe` blocks alone, ATM. [`clippy::undocumented_unsafe_blocks`]: https://rust-lang.github.io/rust-clippy/stable/index.html#undocumented_unsafe_blocks

ErichDonGubler · 2022-11-14T15:47:40Z

@cwfitzgerald: Just autosquashed into nice (taking care to respect the merging changes you made), and…now CI is passing. 🥲 Merge plz?

cwfitzgerald · 2022-11-14T18:49:34Z

Terrifying

Totally my bad; this should not have landed. I was using it while chasing down files to change in gfx-rs#3044. :<

ErichDonGubler force-pushed the unsafe_ops_in_unsafe_fn branch from 0c2265d to d31060d Compare September 23, 2022 21:04

ErichDonGubler force-pushed the unsafe_ops_in_unsafe_fn branch 5 times, most recently from 3c3e317 to a81d7d4 Compare October 9, 2022 14:54

ErichDonGubler marked this pull request as ready for review October 10, 2022 14:05

ErichDonGubler requested a review from crowlKats as a code owner October 10, 2022 14:05

cwfitzgerald requested changes Oct 12, 2022

View reviewed changes

ErichDonGubler force-pushed the unsafe_ops_in_unsafe_fn branch 4 times, most recently from fa0ff35 to 782b6b5 Compare November 8, 2022 15:01

ErichDonGubler commented Nov 8, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

ErichDonGubler commented Nov 8, 2022

View reviewed changes

deno_webgpu/src/lib.rs Show resolved Hide resolved

cwfitzgerald approved these changes Nov 9, 2022

View reviewed changes

cwfitzgerald enabled auto-merge (squash) November 9, 2022 20:31

ErichDonGubler added 6 commits November 14, 2022 10:31

chore: warn(unsafe_op_in_unsafe_fn) for {deno_webgpu,player,wgpu-ty…

511c71f

…pes}` This commit bundles all of the trivial cases for enabling this lint, where no action is required beyond adding the lint.

chore: naively set warn(unsafe_op_in_unsafe_fn) for wgpu

7c128a7

Do the simplest mechanical work necessary to enable and satisfy this lint; only put down `unsafe` blocks, don't try to inspect for correctness or anything else.

docs: add unsafe_op_in_unsafe_fn note to README.md

f411fa7

auto-merge was automatically disabled November 14, 2022 15:34
Head branch was pushed to by a user without write access

ErichDonGubler force-pushed the unsafe_ops_in_unsafe_fn branch from 2c7f9d4 to f411fa7 Compare November 14, 2022 15:34

cwfitzgerald merged commit 18f3f5f into gfx-rs:master Nov 14, 2022

ErichDonGubler deleted the unsafe_ops_in_unsafe_fn branch November 16, 2022 16:11

ErichDonGubler added a commit to erichdongubler-mozilla/wgpu that referenced this pull request Dec 5, 2022

fix: oops, remove gles feature from wgpu-hal's defaults

2543744

Totally my bad; this should not have landed. I was using it while chasing down files to change in gfx-rs#3044. :<

ErichDonGubler mentioned this pull request Dec 5, 2022

fix: oops, remove gles feature from wgpu-hal's defaults #3264

Merged

3 tasks

expenses mentioned this pull request Dec 17, 2022

[wgpu-hal] Initial Vulkan ray query and acceleration structure support #3020

Closed

3 tasks

daniel-keitel mentioned this pull request Feb 20, 2023

[wgpu-hal] Inline RayQuery Support #3507

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable `unsafe_ops_in_unsafe_fn` lint in all workspaces #3044

Enable `unsafe_ops_in_unsafe_fn` lint in all workspaces #3044

ErichDonGubler commented Sep 21, 2022 •

edited

Loading

ErichDonGubler commented Oct 10, 2022 •

edited

Loading

cwfitzgerald left a comment

ErichDonGubler commented Oct 13, 2022 •

edited

Loading

codecov-commenter commented Nov 8, 2022 •

edited

Loading

cwfitzgerald left a comment

cwfitzgerald commented Nov 9, 2022

ErichDonGubler commented Nov 14, 2022 •

edited

Loading

cwfitzgerald commented Nov 14, 2022

Enable unsafe_ops_in_unsafe_fn lint in all workspaces #3044

Enable unsafe_ops_in_unsafe_fn lint in all workspaces #3044

Conversation

ErichDonGubler commented Sep 21, 2022 • edited Loading

Footnotes

ErichDonGubler commented Oct 10, 2022 • edited Loading

cwfitzgerald left a comment

Choose a reason for hiding this comment

ErichDonGubler commented Oct 13, 2022 • edited Loading

codecov-commenter commented Nov 8, 2022 • edited Loading

Codecov Report

cwfitzgerald left a comment

Choose a reason for hiding this comment

cwfitzgerald commented Nov 9, 2022

ErichDonGubler commented Nov 14, 2022 • edited Loading

cwfitzgerald commented Nov 14, 2022

Enable `unsafe_ops_in_unsafe_fn` lint in all workspaces #3044

Enable `unsafe_ops_in_unsafe_fn` lint in all workspaces #3044

ErichDonGubler commented Sep 21, 2022 •

edited

Loading

ErichDonGubler commented Oct 10, 2022 •

edited

Loading

ErichDonGubler commented Oct 13, 2022 •

edited

Loading

codecov-commenter commented Nov 8, 2022 •

edited

Loading

ErichDonGubler commented Nov 14, 2022 •

edited

Loading