Fix Issue#17414: needless_borrow do not contradict with `dangerous_implicit_aut…#17433
Fix Issue#17414: needless_borrow do not contradict with `dangerous_implicit_aut…#17433skiefucker wants to merge 11 commits into
needless_borrow do not contradict with `dangerous_implicit_aut…#17433Conversation
…orefs` Add Logic: If the innermost expression of the borrow is a deref of a raw pointer, and the borrow after multiple Index/Field operations, is used to call a method with `#[rustc_no_implicit_autorefs]`, skip the lint. Remove the narrow `&*raw_ptr` pattern check added in PR rust-lang#14810. The old guard only matched the literal `&*raw_ptr`. Add Test `issue_17414` to `tests/ui/needless_borrow.rs` covering: - `(&(*tuple_slice).field).method()` — the reported case - `(&*raw_slice).len()` and `(&*raw_slice)[idx]` — direct deref - nested tuple field access through a raw pointer - array indexing via `Index::index` on a raw pointer - trait method dispatch (`dyn T`) through a raw pointer Problems Left: - Index Op of slice/array: `(*slice)[0]` can compile, but index method of Index trait has `#[rustc_no_implicit_autorefs]`. (See codes in tests/ui/needless_borrow.rs) - Is there any other operation between deref raw pointer and method with #[rustc_no_implicit_autorefs]? changelog: [`needless_borrow`]: do not contradict `dangerous_implicit_autorefs`.
|
Thanks for the pull request, and welcome! You should hear from one of our reviewers after this PR gets at least 2 reviews from the community. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
This comment has been minimized.
This comment has been minimized.
…orefs` Add Logic: If the innermost expression of the borrow is a deref of a raw pointer, and the borrow after multiple Index/Field operations, is used to call a method with `#[rustc_no_implicit_autorefs]`, skip the lint. Remove the narrow `&*raw_ptr` pattern check added in PR rust-lang#14810. The old guard only matched the literal `&*raw_ptr`. Add Test `issue_17414` to `tests/ui/needless_borrow.rs` covering: - `(&(*tuple_slice).field).method()` — the reported case - `(&*raw_slice).len()` and `(&*raw_slice)[idx]` — direct deref - nested tuple field access through a raw pointer - array indexing via `Index::index` on a raw pointer - trait method dispatch (`dyn T`) through a raw pointer Problems Left: - Index Op of slice/array: `(*slice)[0]` can compile, but index method of Index trait has `#[rustc_no_implicit_autorefs]`. (See codes in tests/ui/needless_borrow.rs) - Is there any other operation between deref raw pointer and method with #[rustc_no_implicit_autorefs]? changelog: [`needless_borrow`]: do not contradict `dangerous_implicit_autorefs`.
…orefs` Add Logic: If the innermost expression of the borrow is a deref of a raw pointer, and the borrow after multiple Index/Field operations, is used to call a method with `#[rustc_no_implicit_autorefs]`, skip the lint. Remove the narrow `&*raw_ptr` pattern check added in PR14810. The old guard only matched the literal `&*raw_ptr`. Add Test `issue_17414` to `tests/ui/needless_borrow.rs` covering: - `(&(*tuple_slice).field).method()` — the reported case - `(&*raw_slice).len()` and `(&*raw_slice)[idx]` — direct deref - nested tuple field access through a raw pointer - array indexing via `Index::index` on a raw pointer - trait method dispatch (`dyn T`) through a raw pointer Problems Left: - Index Op of slice/array: `(*slice)[0]` can compile, but index method of Index trait has `#[rustc_no_implicit_autorefs]`. (See codes in tests/ui/needless_borrow.rs) - Is there any other operation between deref raw pointer and method with #[rustc_no_implicit_autorefs]? changelog: [`needless_borrow`]: do not contradict `dangerous_implicit_autorefs`.
|
Lintcheck changes for a74342f
This comment will be updated if you push new changes |
There was a problem hiding this comment.
community review: one small code change and one question, but otherwise looks good!
EDIT: Also, it's super annoying to read LLM-written MR descriptions, they're a hundred times too long for absolutely useless content, please don't impose that on us
needless variable Co-authored-by: Poliorcetics <poliorcetics@users.noreply.github.com>
One of Fix: `needless_borrow` do not contradict with `dangerous_implicit_autorefs`
1. add needless_borrow lint to a normal index operation. 2. skip this check for raw pointer because explicit borrow is necessary.
1. add needless_borrow lint to a normal index operation. 2. skip this check for raw pointer because explicit borrow is necessary.
1. add needless_borrow lint to a normal index operation. 2. skip this check for raw pointer because explicit borrow is necessary.
1. add needless_borrow lint to a normal index operation. 2. skip this check for raw pointer because explicit borrow is necessary.
1. add needless_borrow lint to a normal index operation. 2. skip this check for raw pointer because explicit borrow is necessary.
| #![expect(clippy::deref_by_slicing)] | ||
| #![allow(clippy::needless_borrow)] |
There was a problem hiding this comment.
| #![expect(clippy::deref_by_slicing)] | |
| #![allow(clippy::needless_borrow)] | |
| #![expect(clippy::deref_by_slicing, clippy::needless_borrow)] |
To follow the existing pattern and avoid cruft longer term
| @@ -1,5 +1,6 @@ | |||
| #![warn(clippy::redundant_slicing)] | |||
| #![expect(clippy::deref_by_slicing)] | |||
| #![allow(clippy::needless_borrow)] | |||
There was a problem hiding this comment.
If change to #![expect(clippy::deref_by_slicing, clippy::needless_borrow)] happens:
error: test got exit code: 1, but expected 0
--> tests/ui/redundant_slicing.fixed:2:37
|
2 | #![expect(clippy::deref_by_slicing, clippy::needless_borrow)]
| ^^^^^^^^^^^^^^^^^^^^^^^ after rustfix is applied, all errors should be gone, but weren't
|
full stderr:
error: this lint expectation is unfulfilled
--> tests\ui\redundant_slicing.fixed:2:37
|
LL | #![expect(clippy::deref_by_slicing, clippy::needless_borrow)]
| ^^^^^^^^^^^^^^^^^^^^^^^
|
= note: `-D unfulfilled-lint-expectations` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(unfulfilled_lint_expectations)]`
error: aborting due to 1 previous error
My opinion:
let _ = &(&*v)[..]; will trigger both redundant_slicing and needless_borrow.
redundant_slicing suggest: let _ = &(&*v)[..]; -> let _ = (&*v);
needless_borrow suggest: let _ = &(&*v)[..]; -> let _ = &(*v)[..];
When redundant_slicing.fixed accept first suggest, code change to let _ = (&*v);, and needless_borrow will not trigger but expect it, then error came.
I tried many times, putting needless_borrow in warn or expect all failed (only putting it in allow worked, of course!!). Or there are other better way I don't know??
The formatting initially made me think of LLMs, but the tone made me reconsider. If that really was written by an LLM, I'll have to recalibrate my radar. |
Sorry about this, I'm not a native English Speaker, And this is also my first PR. I can type some simple words, but if things go harder, I have to use LLM to translate and optimize. With reading and writing more in community, maybe I can do it by myself. Thanks for all these suggestions !!! :) |
We will have a policy on LLM use (see rust-lang/rust-forge#1040) of which the relevant part can be summed up as "don't llm generate PR descriptions/comments". Translation is one thing and fine (though it should be noted), but using it to "improve"/expand the writing significantly less so. |
Copy that !! |
Background
PR #14810 added a guard to
clippy::needless_borrowto avoid contradicting this lint: it suppressesneedless_borrowwhen the expression is&*rawptrin anunsafecontext.However, that guard only matches the literal
&*rawptrshape and misses references taken through field or index access on a raw pointer, which is exactly what Issue#17414 reports:Fix
Remove the narrow
&*rawptrpattern guard with a finer-grained check, add funcneed_explicit_ref, which mirrors the actual trigger condition of rustc'sdangerous_implicit_autorefslint rather than pattern-matching a single expression shape.The check has two parts:
Raw pointer source: starting from the borrowed
sub_expr, walk down throughField/Index/AddrOfuntil reaching aUnary(Deref, e), and verify thate's type is a raw pointer.#[rustc_no_implicit_autorefs]method call: starting from the outerexpr, walk up through use sites (viaexpr_use_sites) until finding aMethodCall, then check whether the callee'sDefIdcarries#[rustc_no_implicit_autorefs](e.g.slice::len,slice::is_empty).If both conditions hold,
needless_borrowis skipped, since removing the&would make rustc'sdangerous_implicit_autorefsfire.Test
Added
issue_17414totests/ui/needless_borrow.rscovering:(&(*tuple_slice).field).method()&is mandatory(&*raw_slice).len()slice::lenhas the attr(&*nested_tuple).a.b.method()and(&(*nested_tuple).a).b.method()(&*raw_array).index(0)Index::indexcarries the attr(&*dyn_trait)[0]/(&*dyn_trait).index(0)(&*raw_slice)[0](&*nested_tuple).1.1.first()slice::firstdoes NOT carry the attr — confirms we don't over-suppress(&slice).1.1.len()on a non-raw referenceRun:
WinPS:
$env:TESTNAME="needless_borrow"; cargo uiblessLinux:
TESTNAME=needless_borrow cargo uiblessOpen questions for reviewers
1. Over-suppression on
[]indexing of slice/array (Resolved)According to
rustc_lint::autoref.rs, different Index operation have different Adjustments!(*slice)[0]can compile, but index method of Index trait has#[rustc_no_implicit_autorefs]. Array is the same.Index operation of slice/array not callIndex::index?So,&in(&*slice)[0]may be needless. The test marks this case as// TODO needless, should lint.2. Other HIR nodes in the receiver chain
Is there any other operation from deref raw pointer to method call with #[rustc_no_implicit_autorefs]?
I only handle and skip Field/Index in
dereference.rs:773. I couldn't construct a counterexample but would appreciate a sanity check.3. specail case with redundant slicing
This case will trigger both reduntdant slicing and needless borrow :
changelog: [
needless_borrow]: do not contradictdangerous_implicit_autorefs.