Refactor: arrange transmute lints #6716
Conversation
|
r? @llogiq (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
clippy_lints/src/transmute/mod.rs
Outdated
| if useless_transmute::check(cx, e, from_ty, to_ty, args) { | ||
| return; | ||
| } | ||
|
|
||
| let triggered = wrong_transmute::check(cx, e, from_ty, to_ty) || | ||
| crosspointer_transmute::check(cx, e, from_ty, to_ty) || | ||
| transmute_ptr_to_ref::check(cx, e, from_ty, to_ty, args, qpath) || | ||
| transmute_int_to_char::check(cx, e, from_ty, to_ty, args) || | ||
| transmute_ref_to_ref::check(cx, e, from_ty, to_ty, args, const_context) || | ||
| transmute_ptr_to_ptr::check(cx, e, from_ty, to_ty, args) || | ||
| transmute_int_to_bool::check(cx, e, from_ty, to_ty, args) || | ||
| transmute_int_to_float::check(cx, e, from_ty, to_ty, args, const_context) || | ||
| transmute_float_to_int::check(cx, e, from_ty, to_ty, args, const_context) || | ||
| unsound_collection_transmute::check(cx, e, from_ty, to_ty); |
There was a problem hiding this comment.
Just a question.
These lines seem to implicitly define the priority order of the lints.
For example, useless_transmute > others and wrong_transmute > crosspointer_transmute and so on.
Is it ok? I think this may become a problem if a new lint has a mutual condition that triggers both the lint and another lint.
There was a problem hiding this comment.
Good point.
Generally, all the lints here are exclusive; if a code violates transmute_int_to_char, then this code does never trigger transmute_ptr_to_ptr, transmute_int_to_bool, and so on.
But there are a few exceptions. For example, useless_transmute and transmute_ref_to_ref can potentially be triggered at the same time - this is NOT exclusive. In such a case, I think we'd prefer one diagnostic over multiple ones to avoid getting the programmer overwhelmed. This is why the lint priority is necessary. (Also... the test cases would fail if we removed the priority :-|)
If a new lint related to transmute is added in the future, we will have to consider the priority of it to put it in the appropriate position. If anyone comes up with a better way, that would be really welcome.
There was a problem hiding this comment.
In such a case, I think we'd prefer one diagnostic over multiple ones to avoid getting the programmer overwhelmed.
I agree with you in almost all cases. My concern here is that if #[allow(clippy::useless_transmute)] is specified by the user, then transmute_ref_to_ref may also be suppressed unexpectedly.
I feel like this is a problem that should be generalized (may be as a future task).
There was a problem hiding this comment.
My concern here is that if
#[allow(clippy::useless_transmute)]is specified by the user, thentransmute_ref_to_refmay also be suppressed unexpectedly.
Ah that makes much sense. I guess the current master branch also has that problem; once the lint checking flow goes into useless_transmute, the other lints won't never be checked even if #[allow(clippy::useless_transmute)] is specified.
With this fact in mind, I feel like prioritizing the lints might be bad.
There was a problem hiding this comment.
I'd say it would be better to keep the current behavior in this PR by confirming the existing test cases are passing, because the main focus of this PR is refactoring. We can consider further how to address the priority issue in another issue or PR.
There was a problem hiding this comment.
the main focus of this PR is refactoring.
I agree with you! I created the topic to discuss this issue on zulip
There was a problem hiding this comment.
If two lints cannot conflict, I would avoid short circuiting them like this. Only if one lint triggering should depend on another lint this should be done. So I would write this as
let mut linted = check_lint_A();
linted |= check_lint_B(); // no dependency
linted |= check_lint_C() || check_lint_D(); // D depends on C
...
if !linted {
check_lint_XYZ(); // XYZ depends on ALL lints
}There was a problem hiding this comment.
@flip1995 Looks cool. I'll rewrite the code!
There was a problem hiding this comment.
@flip1995 Rewriting and rebasing with master are done. PTAL :)
|
☔ The latest upstream changes (presumably #6718) made this pull request unmergeable. Please resolve the merge conflicts. |
magurotuna
force-pushed
the
refactor-transmute-mod
branch
from
81ce279 to
9d937c9
Compare
|
Since the other 2 PRs are assigned to me, I might as well also review this. r? @flip1995 |
flip1995
force-pushed
the
refactor-transmute-mod
branch
from
9d937c9 to
bf00098
Compare
|
@bors r+ p=1 Thanks! I will go through the refactor PRs this week one by one, so that you don't have to keep rebasing every other day |
|
📌 Commit bf00098 has been approved by |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
This PR arranges
transmutelints so that they can be accessed more easily.Basically, I followed the instruction described in #6680 as to how to do the refactoring.
declare_clippy_lint!andimpl LintPassis placed intransmute/mod.rstransmute/utils.rstransmuteis moved into its own module, liketransmute/useless_transmute.rsFor ease of review, I refactored step by step, keeping each commit small. For instance, all I did in
2451781 was to move
useless_transmuteinto its own module.changelog: Refactor
transmute.rsfile intotransmutemodule.