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

make [manual_map] ignore types that contain dyn #12712

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

J-ZhengLi
Copy link
Member

fixes: #12659

[manual_map] and [manual_filter] shares the same check logic, but this issue doesn't seems like it could affect manual_filter (?)


changelog: make [manual_map] ignore types that contain dyn

@rustbot
Copy link
Collaborator

rustbot commented Apr 25, 2024

r? @dswij

rustbot has assigned @dswij.
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 Apr 25, 2024
@J-ZhengLi J-ZhengLi marked this pull request as draft April 26, 2024 01:33
@dswij dswij added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels May 4, 2024
@J-ZhengLi J-ZhengLi force-pushed the issue12666 branch 3 times, most recently from f01faac to 00bb73f Compare May 10, 2024 08:23
@J-ZhengLi J-ZhengLi marked this pull request as ready for review May 10, 2024 08:35
@J-ZhengLi
Copy link
Member Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels May 10, 2024
@xFrednet
Copy link
Member

Hey, this is a ping from triage. @dswij can you give this PR a review? It's totally fine if you don't have the time right now, you can reassign the PR to a random team member using r? clippy.

@rustbot ready

@Alexendoo
Copy link
Member

I'll take this one since I looked into it previously, r? me

@rustbot
Copy link
Collaborator

rustbot commented Jun 21, 2024

Failed to set assignee to me: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

@Alexendoo Alexendoo assigned Alexendoo and unassigned dswij Jun 21, 2024
@J-ZhengLi J-ZhengLi force-pushed the issue12666 branch 2 times, most recently from ceec8fa to a03f382 Compare June 25, 2024 10:29
@J-ZhengLi
Copy link
Member Author

J-ZhengLi commented Jun 29, 2024

I need more time on this

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jun 29, 2024
@J-ZhengLi J-ZhengLi force-pushed the issue12666 branch 3 times, most recently from b386427 to a5fcaea Compare July 2, 2024 08:09
@J-ZhengLi J-ZhengLi added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jul 2, 2024
Comment on lines 287 to 289
if expr.span.from_expansion() {
return false;
}
Copy link
Member

Choose a reason for hiding this comment

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

Is this to catch something specific? It could lead to some FPs though it might be fairly rare

Copy link
Member Author

Choose a reason for hiding this comment

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

hmmm, I forgot why I left it there, but... you're right 😂

Copy link
Member Author

@J-ZhengLi J-ZhengLi Sep 8, 2024

Choose a reason for hiding this comment

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

Oh I think I got it, it was a lazy solution to pass this test:

match Some(0) {
    Some(x) => Some(vec![x]),
     None => None,
};
// ^ help: try: `Some(0).map(|x| vec![x])`

The coercion in this case is totally irrelevant, but I don't know how to filter it out

Copy link
Member Author

@J-ZhengLi J-ZhengLi Sep 13, 2024

Choose a reason for hiding this comment

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

Ok, so... I think I know where the problem was.

technically, the above case is not a coercion site(?) according to the coercion doc:

...these are typically places where the desired type is explicit or can be derived by propagation from explicit types (without type inference).

Thus using map here actually works. However, because I have a type adjustment check at first, which causing it to return early as soon as any expr's ty was adjusted (and this expr in vec! was indeed adjusted), causing FN on the above test case.

Ignore what I said earlier that it was totally irrelevant, here is a counter example which might not seems rare at all:

let x: Option<Vec<&[u8]>> = match Some(()) {
    Some(_) => Some(vec![b"1234"]),
    None => None,
};

With that from_expansion check, this will surely cause FP, so it's definitely a "no". And I also need the type adjustment check, otherwise this counter example wouldn't work.

Now I'm out of options, do I need to bring back the expr_has_explicit_ty function from the beginning?

clippy_lints/src/matches/manual_utils.rs Outdated Show resolved Hide resolved
clippy_lints/src/matches/manual_utils.rs Outdated Show resolved Hide resolved
clippy_lints/src/matches/manual_utils.rs Outdated Show resolved Hide resolved
clippy_lints/src/matches/manual_utils.rs Outdated Show resolved Hide resolved
@Alexendoo Alexendoo added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jul 18, 2024
@J-ZhengLi J-ZhengLi force-pushed the issue12666 branch 2 times, most recently from 8996cc9 to 0e970e0 Compare September 8, 2024 17:59
clippy_utils/src/lib.rs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

manual_map sugg with dyn causes error
5 participants