Skip to content

fix suggestion error for [manual_is_ascii_check] with missing type #11988

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

Merged
merged 1 commit into from
May 1, 2024

Conversation

J-ZhengLi
Copy link
Member

@J-ZhengLi J-ZhengLi commented Dec 20, 2023

fixes: #11324
fixes: #11776

changelog: improve [manual_is_ascii_check] to suggest labeling type in closure, fix FP with type generics, and improve linting on ref expressions.

@rustbot
Copy link
Collaborator

rustbot commented Dec 20, 2023

r? @Alexendoo

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 20, 2023
@J-ZhengLi J-ZhengLi changed the title skip warning when generic evolved; [manual_is_ascii_check] skips generic fn param, and suggest inferred ty param in closure Dec 20, 2023
@J-ZhengLi J-ZhengLi changed the title [manual_is_ascii_check] skips generic fn param, and suggest inferred ty param in closure fix suggestion error for [manual_is_ascii_check] with missing type Jan 19, 2024
let ExprKind::Lit(lit) = range_expr.kind else {
return None;
};
let sugg_ty_span = path_to_local(arg).and_then(|id| map.get(&id)).copied()?;
Copy link
Member

Choose a reason for hiding this comment

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

If it works it would be good to use find_binding_init here rather than storing a map, would let us avoid any issues with nested closures

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately I couldn't get it to work...

but regarding nested closures, I now clear the map only when entering new modules rather than leaving another closure, which should be able to prevent it.

Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah that makes sense, there's no local/init to be found

What could be done is to get the parent of the local's id which should be a Param, for inferred types the ty_span is the same as the span

Copy link
Member Author

Choose a reason for hiding this comment

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

nice, this is the way to go, thanks~

@xFrednet
Copy link
Member

xFrednet commented Apr 1, 2024

Hey @Alexendoo, this is a ping from triage. 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

Comment on lines 198 to 207
diag.span_suggestion(span, "try", format!("{recv}.{sugg}()"), app);
if let Some((ty_span, ty_str)) = ty_sugg {
diag.span_suggestion(
ty_span,
"also make sure to label the correct type",
format!("{recv}: {ty_str}"),
app,
);
}
},
Copy link
Member

Choose a reason for hiding this comment

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

Might be nicer to use a multipart suggestion here

@bors
Copy link
Contributor

bors commented Apr 27, 2024

☔ The latest upstream changes (presumably #12624) made this pull request unmergeable. Please resolve the merge conflicts.

suggest explicit type when its inferred in closure
@Alexendoo
Copy link
Member

Thank you!

@bors r+

@bors
Copy link
Contributor

bors commented May 1, 2024

📌 Commit cbdc36a has been approved by Alexendoo

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented May 1, 2024

⌛ Testing commit cbdc36a with merge 852a64f...

@bors
Copy link
Contributor

bors commented May 1, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Alexendoo
Pushing 852a64f to master...

@bors bors merged commit 852a64f into rust-lang:master May 1, 2024
5 checks passed
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.

manual_is_ascii_check may produce invalid code Clippy suggestion gets rid of type inference, causes compiler error
5 participants