Code Quality CI Checks

[nicoburns]

Inspired by this comment, I thought it might be a good idea to add more code quality checks to Xilem's CI. I am thinking we should have:

• Build & Test cargo test
• Clippy cargo clippy --workspace -- -D warnings
• Rustfmt cargo fmt --all -- --check
• Documentation RUSTDOCFLAGS="-D warnings" cargo doc

Additionally we may want to enforce documentation requirements more strongly. The following

#![warn(missing_docs)]
#![warn(clippy::missing_docs_in_private_items)]

will enforce doc comments on all items. Which seems to me a sensible way to go about things if we want to have an approachable codebase that is welcoming to newcomers.

We might also want to consider:

#![deny(unsafe_code)]
#![forbid(unsafe_code)]

To completely ban unsafe. I'm not sure if that's appropriate for Xilem, but I checked it doesn't currently have any unsafe. So perhaps it would be sensible to include that at least until someone makes a case that they need unsafe?

[derekdreery]

You can always #![warn(unsafe_code)] if you want to be notified when it's added.

[xStrom]

Yes, I think this should all happen. So for CI I think we can go far by adopting the Druid CI config which is quite battle tested and has the tasks you listed. Of course it will still need some modifications, mostly deletions.

Banning unsafe seems like a good idea to me. We did that for druid and I don't think we ever ran into this as a problem.

Of course I'm also all for enforcing documentation. The private items one is a slight question though, as in does this apply to struct fields? Need to test it I guess, but that might be a bit heavyweight. Public API stuff should definitely all 100% be documented though.

[nicoburns]

The private items one is a slight question though, as in does this apply to struct fields?

Yes it does. And enum variants.

that might be a bit heavyweight. Public API stuff should definitely all 100% be documented though.

I definitely think private items should also be documented for the most part. The way I see it, the purpose of documentation on private items is different the purpose of documentation of public items (it's aimed at developers reading the code not end users), but it's equally valuable.

Whether we need to enforce 100% documentation I guess maybe not. I think it's quite useful as a prompt though. Even if you end up writing a pointless comment it means that you've had to go through and at least consider whether a given item needs documentation. Without the lint it's hard to even remember where might need documentation unless you write it as you're going (which often makes no sense to do if the code itself hasn't settled into it's final form yet).

[DJMcNab]

The one issue with banning unsafe code is the challenges around wgpu Surfaces. Vello's only unsafe code is very unsound, so the containing method will need to be unsafe.

Inherently, whichever library integrates a windowing library and wgpu needs to use unsafe for that integration. In the Xilem stack, that library is Xilem.

[xStrom]

Xilem is currently creating a surface with no unsafe. You think this approach isn't sustainable?

Even so, special cases can receive a special exemption. So if we need to deal with some unsafe surfaces in Xilem, we can still do that.

[DJMcNab]

Yes, it's using the unsound RenderContex::create_surface from Vello.

[xStrom]

Are there plans to make that unsafe? Seems like it should be if in practice it's needed.

[DJMcNab]

I think we can close this now?

[xStrom]

Our lint configuration doesn't yet match the goals here. I would be fine with keeping it open.