Fix lint warning #2
Conversation
…ersion lint - Renamed the `ui` module to `test` for clarity. - Removed unused mutex locks and assertions in tests. - Added a new test file `false_positive_iter.rs` to demonstrate a false positive scenario for the unnecessary conversion for trait lint. - Updated the corresponding stderr file to reflect the new test case warnings.
…nto fix-lint-warning
…nto fix-lint-warning
- Improved the lint handling in the `unnecessary_conversion_for_trait` by updating imports and adjusting method signatures. - Enhanced crate-level documentation for better clarity. - Modified the lint pass to utilize `unwrap()` for body retrieval. - Added clarifying comments in the `false_positive_iter.rs` test file regarding its purpose and the unnecessary conversion lint.
- Introduced a crate-level documentation comment for the `dylint` crate. - Updated import statements in `unnecessary_conversion_for_trait` to include `HirId` and removed redundant imports. - Modified the body retrieval in the lint pass to use `unwrap()` for better error handling. - Enhanced the `UsageVisitor` struct's nested visit map method for clarity. - Added a comment in the false positive test file to ensure accessibility and context.
…nto fix-lint-warning
Reviewer's Guide by SourceryThis pull request enhances the Sequence diagram for is_used_later functionsequenceDiagram
participant LateContext
participant UsageVisitor
participant Body
participant Stmt
participant Expr
LateContext->>Body: tcx.hir().enclosing_body_owner(hir_id)
LateContext->>Body: tcx.hir().body(body_id)
loop For each statement in body.value.stmts
Body->>Stmt: Get statement
alt stmt.span > call_span
Stmt->>UsageVisitor: visitor.visit_stmt(stmt)
UsageVisitor->>UsageVisitor: visit_stmt(stmt)
end
end
alt body.value.expr is Some and expr.span > call_span
Body->>Expr: Get expression
Expr->>UsageVisitor: visitor.visit_expr(expr)
UsageVisitor->>UsageVisitor: visit_expr(expr)
end
UsageVisitor->>LateContext: Return found
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @enfayz - I've reviewed your changes - here's some feedback:
Overall Comments:
- The new
UsageVisitorstruct inunnecessary_conversion_for_trait/src/lib.rsshould implementvisit_nestedto avoid recursing into nested items. - The test module name
uiinunnecessary_conversion_for_trait/src/lib.rswas changed totest, but the directory is still namedui.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -181,6 +184,31 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryConversionForTrait { | |||
| && let Some(input) = outer_fn_sig.inputs().get(i) | |||
| && let Param(param_ty) = input.kind() | |||
| { | |||
| // Check if the original collection is used later | |||
There was a problem hiding this comment.
suggestion (bug_risk): Validate the span-based check for later usage.
The check comparing stmt spans to maybe_call.span assumes that later usage always appears in later statements. Ensure that this logic covers all potential usage scenarios (e.g. nested expressions or blocks) or document the limitation.
| // Check if the original collection is used later | |
| // Check if the original collection is used later | |
| // NOTE: The current check assumes that the usage appears in subsequent top-level statements. | |
| // It does not account for usages nested within inner expressions or blocks. |
internal/src/testing.rs
Outdated
| let metadata = MetadataCommand::new() | ||
| .current_dir(workspace_root.as_ref()) | ||
| .no_deps() | ||
| .exec() | ||
| .unwrap(); |
There was a problem hiding this comment.
suggestion (bug_risk): Avoid potential panics with unwrap in cargo_dylint.
Using unwrap on the MetadataCommand execution might lead to panics in environments with unexpected cargo metadata. Consider propagating the error using '?' or providing a more descriptive error message.
| let metadata = MetadataCommand::new() | |
| .current_dir(workspace_root.as_ref()) | |
| .no_deps() | |
| .exec() | |
| .unwrap(); | |
| let metadata = MetadataCommand::new() | |
| .current_dir(workspace_root.as_ref()) | |
| .no_deps() | |
| .exec()?; |
There was a problem hiding this comment.
Hey @enfayz - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a comment explaining the purpose of the
UsageVisitorstruct and its fields. - The test module name was changed from
uitotest, but the test files are still located in theuidirectory. Should the test files be moved to atestdirectory, or should the module name be reverted?
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| // Check if the original collection is used later | ||
| let hir_id = maybe_arg.hir_id; | ||
| let mut is_used_later = false; | ||
| let body_id = cx.tcx.hir().enclosing_body_owner(hir_id); | ||
| let body = cx.tcx.hir().body(body_id).unwrap(); | ||
|
|
||
| for stmt in body.value.stmts.iter() { | ||
| if stmt.span > maybe_call.span { | ||
| let mut visitor = UsageVisitor { | ||
| hir_id, |
There was a problem hiding this comment.
suggestion: Review the logic for detecting later use of the collection.
The approach of comparing the statement spans (stmt.span > maybe_call.span) and then using a visitor to check usage seems reasonable, but verify that this method reliably covers all cases—especially when nested or non-linear control flows might be present.
| @@ -181,6 +184,31 @@ impl<'tcx> LateLintPass<'tcx> for UnnecessaryConversionForTrait { | |||
| && let Some(input) = outer_fn_sig.inputs().get(i) | |||
| && let Param(param_ty) = input.kind() | |||
There was a problem hiding this comment.
issue (complexity): Consider extracting the later-usage check into a helper function to reduce nesting and improve readability of the code.
You can reduce the inline nesting by extracting the later‑usage check into its own helper function. That way, the loop and visitor logic are isolated, which improves readability and comprehensibility. For example:
fn is_used_later(cx: &LateContext<'_>, hir_id: HirId, call_span: Span) -> bool {
let body_id = cx.tcx.hir().enclosing_body_owner(hir_id);
let body = cx.tcx.hir().body(body_id).unwrap();
for stmt in body.value.stmts.iter() {
if stmt.span > call_span {
let mut visitor = UsageVisitor { hir_id, found: false };
visitor.visit_stmt(stmt);
if visitor.found {
return true;
}
}
}
false
}And then in your main linting function, replace the inline logic with:
if is_used_later(cx, maybe_arg.hir_id, maybe_call.span) {
return;
}This change preserves functionality while reducing nested complexity.
- Introduced a `UsageVisitor` struct to check if a `HirId` is used later in the code. - Refactored the `is_used_later` function to utilize the new visitor for checking usage after a specified span. - Removed redundant code from the lint pass that previously handled usage checking. - Enhanced the overall structure and clarity of the lint implementation.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @enfayz - I've reviewed your changes - here's some feedback:
Overall Comments:
- The new
is_used_laterfunction could be extracted to a separate module for better organization. - Consider adding a comment to the new test case explaining why the iterator is necessary.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
|
|
||
| /// Checks if the given HirId is used later in the code after the specified span | ||
| fn is_used_later<'tcx>(cx: &LateContext<'tcx>, hir_id: HirId, call_span: rustc_span::Span) -> bool { |
There was a problem hiding this comment.
suggestion: DRY: Remove duplicated UsageVisitor instantiation in is_used_later.
Both loops in is_used_later create a fresh UsageVisitor to walk either statements or the return expression. Consider extracting a helper function that takes an expression (or statement) and returns whether the HirId is used. This will reduce duplication and simplify future modifications.
Suggested implementation:
/// Helper function that checks whether a given node (statement or expression) contains the HirId usage.
fn usage_found_in<T, F>(hir_id: HirId, node: &T, visit_fn: F) -> bool
where
F: FnOnce(&mut UsageVisitor, &T),
{
let mut visitor = UsageVisitor {
hir_id,
found: false,
};
visit_fn(&mut visitor, node);
visitor.found
}
/// Checks if the given HirId is used later in the code after the specified span
fn is_used_later<'tcx>(cx: &LateContext<'tcx>, hir_id: HirId, call_span: rustc_span::Span) -> bool {
let body_id = cx.tcx.hir().enclosing_body_owner(hir_id);
let body = cx.tcx.hir().body(body_id).unwrap();
// Check all statements in the body
for stmt in body.value.stmts.iter() {
if stmt.span > call_span {
if usage_found_in(hir_id, stmt, |v, s| v.visit_stmt(s)) {
return true;
}
}
}
// (Assuming similar duplicate logic exists for the return expression, apply the helper similarly)If there is another loop (or check) performing similar usage detection on a return expression, replace that duplicated visitor instantiation with a call to the helper function as well. This change will reduce duplication and simplify future modifications.
internal/src/testing.rs
Outdated
| let metadata = MetadataCommand::new() | ||
| .current_dir(workspace_root.as_ref()) | ||
| .no_deps() | ||
| .exec() | ||
| .unwrap(); |
There was a problem hiding this comment.
suggestion: Propagate errors in cargo_dylint instead of unwrap().
Inside cargo_dylint, the MetadataCommand::exec() result is unwrapped. Since the function already returns a Result, using the '?' operator here would provide more robust error handling.
| let metadata = MetadataCommand::new() | |
| .current_dir(workspace_root.as_ref()) | |
| .no_deps() | |
| .exec() | |
| .unwrap(); | |
| let metadata = MetadataCom F438 mand::new() | |
| .current_dir(workspace_root.as_ref()) | |
| .no_deps() | |
| .exec()?; |
| found: bool, | ||
| } | ||
|
|
||
| impl<'tcx> Visitor<'tcx> for UsageVisitor { |
There was a problem hiding this comment.
issue (complexity): Consider simplifying the visitor by using intravisit::walk_expr to handle recursion, removing the need for manual matching on control-flow constructs and reducing code complexity..
Consider simplifying the visitor by removing the custom matching on control-flow constructs. For example, you could short-circuit on a match and then delegate the full recursion to `intravisit::walk_expr`. This way, you preserve the early exit while reducing explicit branch handling:
```rust
impl<'tcx> Visitor<'tcx> for UsageVisitor {
type NestedFilter = nested_filter::OnlyBodies;
fn nested_visit_map(&mut self) -> Self::NestedFilter {
nested_filter::OnlyBodies
}
fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) {
if self.found { return }
if expr.hir_id == self.hir_id {
self.found = true;
return;
}
// Rely on the default walk, which covers all expression kinds uniformly.
intravisit::walk_expr(self, expr);
}
}This reduces the manual branching for blocks, ifs, matches, and loops while preserving functionality.
- Introduced a new `usage_check.rs` module containing the `UsageVisitor` struct and the `is_used_later` function to track HirId usage in code. - Updated `lib.rs` to include the new module and its functionality. - Modified the test file to ensure clarity on the necessity of using `.iter()` with collections, enhancing the test for false positive scenarios.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @enfayz - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider moving the
UsageVisitorandis_used_laterfunctions to a separate module for better organization. - The
testmodule could be renamed toteststo follow Rust conventions.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| found: bool, | ||
| } | ||
|
|
||
| impl<'tcx> Visitor<'tcx> for UsageVisitor { |
There was a problem hiding this comment.
issue (complexity): Consider refactoring the UsageVisitor to use a uniform early check and intravisit::walk_expr to reduce nesting and complexity.
Instead of manually matching on each `ExprKind`, you can simplify by doing a uniform early check and then handing off to `intravisit::walk_expr`. For example:
```rust
impl<'tcx> Visitor<'tcx> for UsageVisitor {
type NestedFilter = nested_filter::OnlyBodies;
fn nested_visit_map(&mut self) -> Self::NestedFilter {
nested_filter::OnlyBodies
}
fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) {
if self.found {
return;
}
if expr.hir_id == self.hir_id {
self.found = true;
return;
}
intravisit::walk_expr(self, expr);
}
}This refactoring reduces nesting and complexity while preserving the early exit on a match.
- Updated the comment in the `visit_expr` method to clarify that intravisit will handle recursion uniformly for all expression types. - Improved code readability by enhancing the comment's clarity regarding the walking of expression kinds.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @enfayz - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a comment explaining why the
COVERAGEandCHECK_INHERENTSvariables are being set in the tests. - The addition of
usage_check.rsis a good step towards preventing false positives.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
internal/src/testing.rs
Outdated
| let metadata = MetadataCommand::new() | ||
| .current_dir(workspace_root.as_ref()) | ||
| .no_deps() | ||
| .exec() | ||
| .unwrap(); |
There was a problem hiding this comment.
suggestion (bug_risk): Avoid using unwrap in production code paths.
Using unwrap on the result of MetadataCommand::exec() may lead to panics in error scenarios. It would be preferable to propagate the error with '?' to keep error handling consistent within this helper.
| let metadata = MetadataCommand::new() | |
| .current_dir(workspace_root.as_ref()) | |
| .no_deps() | |
| .exec() | |
| .unwrap(); | |
| let metadata = MetadataCommand::new() | |
| .current_dir(workspace_root.as_ref()) | |
| .no_deps() | |
| .exec()?; |
| } | ||
|
|
||
| /// Checks if the given HirId is used later in the code after the specified span | ||
| pub(crate) fn is_used_later<'tcx>( |
There was a problem hiding this comment.
issue (complexity): Consider inlining the usage check in is_used_later to avoid the helper closure and reduce abstraction and nesting complexity, while maintaining functionality.
The visitor abstraction looks more involved than needed. One way to simplify is to inline the usage check in is_used_later rather than going through a helper closure. In other words, create a visitor once, traverse both the statement and return expression, and early‐exit when the target is found. For example:
pub(crate) fn is_used_later<'tcx>(
cx: &LateContext<'tcx>,
hir_id: HirId,
call_span: rustc_span::Span,
) -> bool {
let body_id = cx.tcx.hir().enclosing_body_owner(hir_id);
let body = cx.tcx.hir().body(body_id).unwrap();
let mut visitor = UsageVisitor { hir_id, found: false };
// Traverse statements after call_span
for stmt in &body.value.stmts {
if stmt.span > call_span {
visitor.visit_stmt(stmt);
if visitor.found {
return true;
}
}
}
// Traverse the return expression if available
if let Some(expr) = body.value.expr {
if expr.span > call_span {
visitor.visit_expr(expr);
if visitor.found {
return true;
}
}
}
false
}By removing the usage_found_in helper and closure indirection, you reduce an extra layer of abstraction and nesting while keeping the functionality intact.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @enfayz - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a comment to
is_used_laterexplaining why you are only checking the current body.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| use rustc_middle::hir::nested_filter; | ||
|
|
||
| /// Visitor to check if a HirId is used in code | ||
| pub(crate) struct UsageVisitor { |
There was a problem hiding this comment.
issue (complexity): Consider adding a span filter to the visitor to avoid manual iteration and recursion control in is_used_later function, simplifying the traversal logic and reducing code duplication..
Consider unifying the traversal logic and reducing manual recursion control. For example, you could add a span filter into the visitor (instead of manually iterating over statements and expression in is_used_later), so that you only traverse nodes with a span later than call_span. This removes the need for a specialized closure and duplicated recursion logic.
For instance, add a field to capture the minimum span and update visit_expr accordingly:
pub(crate) struct UsageVisitor {
hir_id: HirId,
found: bool,
min_span: rustc_span::Span, // only consider expressions after this span
}
impl<'tcx> Visitor<'tcx> for UsageVisitor {
type NestedFilter = rustc_middle::hir::nested_filter::OnlyBodies;
fn nested_visit_map(&mut self) -> Self::NestedFilter {
rustc_middle::hir::nested_filter::OnlyBodies
}
fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) {
// Skip expressions before our min_span.
if expr.span <= self.min_span || self.found {
return;
}
if expr.hir_id == self.hir_id {
self.found = true;
} else {
intravisit::walk_expr(self, expr);
}
}
}Then simplify is_used_later to perform a single traversal over the body:
pub(crate) fn is_used_later<'tcx>(
cx: &LateContext<'tcx>,
hir_id: HirId,
call_span: rustc_span::Span,
) -> bool {
let body_id = cx.tcx.hir().enclosing_body_owner(hir_id);
let body = cx.tcx.hir().body(body_id).unwrap();
let mut visitor = UsageVisitor {
hir_id,
found: false,
min_span: call_span,
};
// Walk from the body value; internal span-checking filters nodes.
intravisit::walk_expr(&mut visitor, &body.value);
visitor.found
}This refactoring leverages the existing visitor traversal while filtering by span, reducing nesting and early return logic.
changes
Summary by Sourcery
Improve the unnecessary_conversion_for_trait lint to handle false posit 10000 ive cases, particularly with iterators and collection reuse
Enhancements:
Tests: