Issue function modifiers in the right order in manual_async_fn lint #10456
Conversation
|
r? @flip1995 (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
the
S-waiting-on-review
|
r? |
|
r? @Manishearth |
|
@Manishearth Is there anything more you'd like me to do on this one? |
clippy_lints/src/manual_async_fn.rs
Outdated
| @@ -74,11 +74,16 @@ impl<'tcx> LateLintPass<'tcx> for ManualAsyncFn { | |||
| if let Some(ret_pos) = position_before_rarrow(&header_snip); | |||
| if let Some((ret_sugg, ret_snip)) = suggested_ret(cx, output); | |||
| then { | |||
| let header_snip = if header_snip.starts_with("pub ") { | |||
There was a problem hiding this comment.
please use the Visibility on the function signature object here
There was a problem hiding this comment.
I've added the vis_span, as well as tests with pub(crate) and pub(self).
There was a problem hiding this comment.
No, please use the Visibility itself, not its span/snippet. It's a typed API
There was a problem hiding this comment.
Are you talking about cx.tcx.visibility(def_id), which returns a ty::Visibility? How would it help when what we are trying to achieve is to insert async at the right place in the function signature (right after the visibility)? I had the impression that ty::Visibility would give us semantic information that would need to be transformed back manually into a string to insert into the suggestion.
There was a problem hiding this comment.
We're not turning it back into a string, though, we're just checking if it's got a non-private visibility.
There was a problem hiding this comment.
Sorry for being obtuse, but we clearly do not understand each other, and I am fully conscious that the issue might be on my side.
Let me summarize this patch problem statement: given a function or method declaration, insert the async word between the function visibility in the source code, whatever it is, and the rest of the function signature (or right before the function signature if no visibility is present in the source).
All decisions to trigger the lint have been made before already, this patch only changes the place where async is inserted in the suggestion presented to the user. There is nothing to compare the ty::Visibility against, it wouldn't change our decision in any way, which is: insert async right after the visibility span if not empty, or before the function signature otherwise. I cannot envision any situation where we would ultimately end up doing anything different from this.
I'm afraid that checking for a non-private visibility could even be counter-productive when we get async trait functions in, as they will get a non-private visibility without any visibility span, so we would have to maintain the check on the span shown above.
I understand that you might not have a lot of time to explain to me why you think that using visibility semantic information would be beneficial over just looking at its span for this problem. I'll wait until someone else picks up this patch (either a contributor or another maintainer) as I do not understand how I can make any meaningful progress by following your suggestion to use semantic information when I can't see how it would benefit this patch.
There was a problem hiding this comment.
Ah I think the thing I missed on my first read-through was that we are reading the visibility as a string for the snippet anyway.
(the code changed a couple times and I missed it)
No, I have enough time, if I did not have time to review this at the typical speed I would have bounced the PR to someone else, I just don't have enough time to review stuff with very quick turnaround.
samueltardieu
force-pushed
the
issue-10450
branch
from
069819a
to
afe27ba
Compare
| @@ -70,15 +72,21 @@ impl<'tcx> LateLintPass<'tcx> for ManualAsyncFn { | |||
| "this function can be simplified using the `async fn` syntax", | |||
| |diag| { | |||
| if_chain! { | |||
| if let Some(vis_snip) = snippet_opt(cx, *vis_span); | |||
| if let Some(header_snip) = snippet_opt(cx, header_span); | |||
There was a problem hiding this comment.
nit: this line is unnecessary now
|
@bors r+ |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
Fixes #10450
changelog: [
manual_async_fn] output function modifiers in correct order