Skip to content

Conversation

@profetia
Copy link
Contributor

@profetia profetia commented Feb 5, 2025

fixes #13934

I modified the part for checking if the map is used so that it can check field and index exprs.

changelog: [map_entry]: fix FP on struct member

@rustbot
Copy link
Collaborator

rustbot commented Feb 5, 2025

r? @llogiq

rustbot has assigned @llogiq.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 5, 2025
@profetia
Copy link
Contributor Author

profetia commented Feb 5, 2025

r? clippy

Comment on lines 565 to 579
fn is_map_or_parent_used(cx: &LateContext<'_>, map: &Expr<'_>, expr: &Expr<'_>) -> bool {
SpanlessEq::new(cx).eq_expr(map, expr)
|| matches!(map.kind, ExprKind::Field(inner, _) | ExprKind::Index(inner, _, _) if is_map_or_parent_used(cx, inner, expr))
}

Copy link
Member

@Centri3 Centri3 Feb 18, 2025

Choose a reason for hiding this comment

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

I believe for_each_expr would be nicer.

Note this usage of || is clever but hard to read at first.

Also, can you try to document this function a bit? My initial 2min look leaves me a bit confused on mostly the name. I'm not sure if it's misleading or I'm missing context not present in the diff.

I can reroll or look closer later but I believe just the refactoring I've suggested is what this needs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@profetia profetia force-pushed the issue13934 branch 3 times, most recently from 8d082dd to 1d3d43c Compare February 18, 2025 07:59
/// Check if the given expression is used for each sub-expression in the given map.
/// For example, in map `a.b.c.my_map`, The expression `a.b.c.my_map`, `a.b.c`, `a.b`, and `a` are
/// all checked.
fn is_each_expr_in_map_used<'tcx>(cx: &LateContext<'tcx>, map: &'tcx Expr<'tcx>, expr: &'tcx Expr<'tcx>) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

is_any_expr_in_map_used is closer to what it does, yes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fair

@Centri3 Centri3 added this pull request to the merge queue Feb 19, 2025
Merged via the queue into rust-lang:master with commit d8ecde0 Feb 19, 2025
11 checks passed
@profetia profetia deleted the issue13934 branch September 12, 2025 01:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Compile error after applying clippy::map_entry

4 participants