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

Fix clippy: operator precedence can trip the unwary. #1055

Merged
merged 1 commit into from
Feb 22, 2025

Conversation

Yarwin
Copy link
Contributor

@Yarwin Yarwin commented Feb 22, 2025

No description provided.

@Yarwin Yarwin requested a review from Bromeon February 22, 2025 10:41
@Yarwin Yarwin added bug c: tooling CI, automation, tools labels Feb 22, 2025
@GodotRust
Copy link

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1055

@Bromeon
Copy link
Member

Bromeon commented Feb 22, 2025

I tend to agree with the sentiments from rust-lang/rust-clippy#14097 and rust-lang/rust-clippy#14115:

I would rather not have the part of clippy::precedence that flags "mixed usage of bitmasking and bit shifting operators without parentheses", because when you work with bitmasking regularly, it's reasonably well-known that x | 1 << 10 or y & 1 << 10 has the precedence you would expect.

Commit 2550530 has extended the precedence lint to include bitmasking and shift operations. The lint is warn by default, and this generates many hits, especially in embedded or system code, where it is very idiomatic to use expressions such as 1 << 3 | 1 << 5 without parentheses.

godot-rust being a FFI-centric project many low-level components, and bit manipulations are very common across the Godot engine. Being aware of bit operator precedence is something we can expect from contributors (even if mistakes can of course happen, me included). I find the extra readability from not having to parenthesise worth it.

I consider this lint being warn-by-default a regression, and clippy maintainers have agreed by splitting the lint up into separate ones. As such we should do it like microsoft/openvmm@f403f51 and temporarily disable this lint, with a TODO that we re-enable it for Rust 1.86. The useless busywork is a bit annoying, but it's also on us for using the latest version in CI. We should not do it on a Cargo.toml level though, but in full-ci.yml/minimal-ci.yml/check.sh.

@Bromeon Bromeon added quality-of-life No new functionality, but improves ergonomics/internals and removed bug labels Feb 22, 2025
@Bromeon
Copy link
Member

Bromeon commented Feb 22, 2025

Side note, I usually label clippy issues as quality-of-life, because most of them are hardly bugs but (sometimes opinionated) style choices. There are of course cases where clippy detects real bugs.

@Yarwin Yarwin force-pushed the fix-clippy branch 2 times, most recently from 2133316 to 3c54e66 Compare February 22, 2025 12:07
Includes bitmasking and shift operations in Rust 1.85.
This behaviour will be reverted once Rust 1.86 is stable.
Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

Thanks a lot! 👍

Added TODOs in CI workflows as well, and linked to this PR.

@Bromeon Bromeon enabled auto-merge February 22, 2025 15:24
@Bromeon Bromeon added this pull request to the merge queue Feb 22, 2025
Merged via the queue into godot-rust:master with commit 40dcc56 Feb 22, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: tooling CI, automation, tools quality-of-life No new functionality, but improves ergonomics/internals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants