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

Enable unsafe_ops_in_unsafe_fn lint in all workspaces #3044

Merged

Conversation

ErichDonGubler
Copy link
Member

@ErichDonGubler ErichDonGubler commented Sep 21, 2022

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.1

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! :)

Footnotes

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

@ErichDonGubler 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 ErichDonGubler marked this pull request as ready for review October 10, 2022 14:05
@ErichDonGubler
Copy link
Member Author

ErichDonGubler commented Oct 10, 2022

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. :)

Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
wgpu-core/src/track/mod.rs Outdated Show resolved Hide resolved
wgpu-hal/src/dx11/device.rs Outdated Show resolved Hide resolved
wgpu-hal/src/dx12/mod.rs Outdated Show resolved Hide resolved
wgpu-hal/src/dx12/mod.rs Show resolved Hide resolved
wgpu-hal/src/dx12/view.rs Show resolved Hide resolved
wgpu-hal/src/gles/adapter.rs Show resolved Hide resolved
@ErichDonGubler
Copy link
Member Author

ErichDonGubler commented Oct 13, 2022

@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!
}

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

codecov-commenter commented Nov 8, 2022

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 Outdated Show resolved Hide resolved
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

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 cwfitzgerald enabled auto-merge (squash) November 9, 2022 20:31
@cwfitzgerald
Copy link
Member

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.
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
auto-merge was automatically disabled November 14, 2022 15:34

Head branch was pushed to by a user without write access

@ErichDonGubler
Copy link
Member Author

ErichDonGubler commented Nov 14, 2022

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

@cwfitzgerald
Copy link
Member

Terrifying

@cwfitzgerald cwfitzgerald merged commit 18f3f5f into gfx-rs:master Nov 14, 2022
@ErichDonGubler 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
Totally my bad; this should not have landed. I was using it while
chasing down files to change in gfx-rs#3044. :<
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.

4 participants