Suggest adding a return type for async functions #107685
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Nilstrieb (or someone else) soon. Please see the contribution instructions for more information. |
rustbot
added
S-waiting-on-review
| // Check if async function's return type was omitted. | ||
| // Don't emit suggestions if the found type is `impl Future<...>`. | ||
| debug!("suggest_missing_return_type: found = {:?}", found); | ||
| if self.get_impl_future_output_ty(found).is_none() { |
There was a problem hiding this comment.
can you use is_suggestable instead? https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/trait.IsSuggestable.html#tymethod.is_suggestable
There was a problem hiding this comment.
This seems to return false if I call found.is_suggestable(self.tcx, false), so it doesn't emit the diagnostic if I use is_suggestable instead of self.get_impl_future_output_ty(found).is_none() unfortunately. Test case is
async fn hello() {
Ok(())
}Is a Result<(), _> with an unknown Err type not suggestable?
EDIT: nvm I'm dumb, have to permit type inference variables, using found.is_suggestable(self.tcx, true) works.
| @@ -723,7 +723,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { | |||
| // Check if async function's return type was omitted. | |||
| // Don't emit suggestions if the found type is `impl Future<...>`. | |||
| debug!("suggest_missing_return_type: found = {:?}", found); | |||
| if self.get_impl_future_output_ty(found).is_none() { | |||
| if found.is_suggestable(self.tcx, true) { | |||
There was a problem hiding this comment.
Using is_suggestable with false is actually consistent with other diagnostics, which don't suggest anything if there's inference variables. See:
fn foo() {
Ok(())
}
which does not suggest -> Result<(), _>, since that's not exactly a complete type.
I'm not sure if we should be suggesting something when there are inference variables in the return type -- you could probably just come up with a different example instead of Ok(()) in the UI test you added.
Either that, or the regular "return type is missing" suggestion should be modified in the non-async case, but then the subdiagnostic applicability would need to be bumped down to "has-placeholders" from "machine-applicable".
There was a problem hiding this comment.
Yeah that makes sense, I will change it back and modify the test. Not suggesting an incomplete type makes more sense to me.
There was a problem hiding this comment.
Thanks! Maybe worth adding a test for something that's "not suggestable" too, like a closure.
|
Can you squash these commits? Then I can approve, thanks. |
Squashed, thanks for the review |
|
Thanks! r? @compiler-errors @bors r+ rollup If you'd like to look into making this suggestion + the non-async version do something for inference variable cases too (maybe suggest a "HasPlaceholders" level, and make the wording weaker, like suggesting filling in the |
|
📌 Commit 74d9d4900c716cca9953b62de500f48c87b5d577 has been approved by It is now in the queue for this repository. |
bors
added
S-waiting-on-bors
I'll take a look at how to do that, sure |
|
D'oh I forgot to remove the try adding a semicolon thing, sorry about that |
|
@bors r+ |
Rollup of 5 pull requests Successful merges: - rust-lang#107553 (Suggest std::ptr::null if literal 0 is given to a raw pointer function argument) - rust-lang#107580 (Recover from lifetimes with default lifetimes in generic args) - rust-lang#107669 (rustdoc: combine duplicate rules in ayu CSS) - rust-lang#107685 (Suggest adding a return type for async functions) - rust-lang#107687 (Adapt SROA MIR opt for aggregated MIR) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Fixes #90027.