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.

Sign up for GitHub

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

For E0277 suggest adding Result return type for function when using QuestionMark ? in the body. #126187

Merged
merged 1 commit into from
Jun 12, 2024
Merged

For E0277 suggest adding Result return type for function when using QuestionMark ? in the body. #126187

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions compiler/rustc_hir/src/hir.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1010,6 +1010,7 @@ pub struct Block<'hir> {
pub hir_id: HirId,
/// Distinguishes between `unsafe { ... }` and `{ ... }`.
pub rules: BlockCheckMode,
/// The span includes the curly braces `{` and `}` around the block.
pub span: Span,
/// If true, then there may exist `break 'a` values that aim to
/// break out of this block early.
Expand Down
41 changes: 41 additions & 0 deletions compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4586,6 +4586,47 @@ impl<'tcx> TypeErrCtxt<'_, 'tcx> {
_ => "/* value */".to_string(),
})
}

fn suggest_add_result_as_return_type(
&self,
obligation: &PredicateObligation<'tcx>,
err: &mut Diag<'_>,
trait_ref: ty::PolyTraitRef<'tcx>,
) {
if ObligationCauseCode::QuestionMark != *obligation.cause.code().peel_derives() {
return;
}

let node = self.tcx.hir_node_by_def_id(obligation.cause.body_id);
if let hir::Node::Item(item) = node
&& let hir::ItemKind::Fn(sig, _, body_id) = item.kind
&& let hir::FnRetTy::DefaultReturn(ret_span) = sig.decl.output
surechen marked this conversation as resolved.
Show resolved Hide resolved
&& self.tcx.is_diagnostic_item(sym::FromResidual, trait_ref.def_id())
&& let ty::Tuple(l) = trait_ref.skip_binder().args.type_at(0).kind()
&& l.len() == 0
&& let ty::Adt(def, _) = trait_ref.skip_binder().args.type_at(1).kind()
&& self.tcx.is_diagnostic_item(sym::Result, def.did())
{
let body = self.tcx.hir().body(body_id);
let mut sugg_spans =
vec![(ret_span, " -> Result<(), Box<dyn std::error::Error>>".to_string())];

if let hir::ExprKind::Block(b, _) = body.value.kind
&& b.expr.is_none()
{
sugg_spans.push((
// The span will point to the closing curly brace `}` of the block.
b.span.shrink_to_hi().with_lo(b.span.hi() - BytePos(1)),
Copy link
Contributor

Choose a reason for hiding this comment

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

since we know there must be at least one statement, you can instead do b.stmts.last().unwrap().span.shrink_to_hi(), this way you'll get a span right after the last statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since we know there must be at least one statement, you can instead do b.stmts.last().unwrap().span.shrink_to_hi(), this way you'll get a span right after the last statement.

Thank you. I tried this way. But when the last stmt is a macro call like println!();, it's span will point to std
like: D:\source\rust\rust\library\std\src\macros.rs:85:6: 85:6 (#8).

Copy link
Contributor

Choose a reason for hiding this comment

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

ah indeed. hmm... Maybe we should change the value of Block::span to not include the brackets around it. We can already obtain the "with brackets" span by taking the span of the outer expression. That's a bit more involved though, as you'd probably need to touch the parser.

I don't think the - BytePos(1) will work in case the block itself is created by a macro or other expansion. Please add some tests for async functions, which should show us any problematic behaviour. And try to generate an entire function with a macro and see how your suggestion behaves on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah indeed. hmm... Maybe we should change the value of Block::span to not include the brackets around it. We can already obtain the "with brackets" span by taking the span of the outer expression. That's a bit more involved though, as you'd probably need to touch the parser.

I don't think the - BytePos(1) will work in case the block itself is created by a macro or other expansion. Please add some tests for async functions, which should show us any problematic behaviour. And try to generate an entire function with a macro and see how your suggestion behaves on that.

Hello, thank you very much.
I tried async and declaring macros and it handles the case of declaring macros correctly.
For async functions, now the current logic will skip it. I added the corresponding conditions and check a async function like:

async fn test2() {
    let mut file = File::create("foo.txt")?;
    println!();
}

The fix result will be :

async fn test2() -> Result<(), Box<dyn std::error::Error>> {
    Ok(())
}

The span seems point to the body correct. But after fixed the expr let mut file = File::create("foo.txt")?; and println!(); in the body will be overwriten.

Because the current logic does not handle the closure situation, and I have not thought of an effective way to repair the closure, I think we can skip it in this PR.

"\n Ok(())\n}".to_string(),
));
}
err.multipart_suggestion_verbose(
format!("consider adding return type"),
sugg_spans,
Applicability::MaybeIncorrect,
);
}
}
}

/// Add a hint to add a missing borrow or remove an unnecessary one.
Expand Down
4 changes: 4 additions & 0 deletions compiler/rustc_trait_selection/src/traits/error_reporting/type_err_ctxt_ext.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -612,6 +612,10 @@ impl<'tcx> TypeErrCtxt<'_, 'tcx> {
&mut err,
trait_predicate,
);
self.suggest_add_result_as_return_type(&obligation,
&mut err,
trait_ref);

if self.suggest_add_reference_to_arg(
&obligation,
&mut err,
Expand Down
42 changes: 42 additions & 0 deletions tests/ui/return/return-from-residual-sugg-issue-125997.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
//@ run-rustfix

#![allow(unused_imports)]
#![allow(dead_code)]

use std::fs::File;
use std::io::prelude::*;

fn test1() -> Result<(), Box<dyn std::error::Error>> {
let mut _file = File::create("foo.txt")?;
//~^ ERROR the `?` operator can only be used in a function

Ok(())
}

fn test2() -> Result<(), Box<dyn std::error::Error>> {
let mut _file = File::create("foo.txt")?;
//~^ ERROR the `?` operator can only be used in a function
println!();

Ok(())
}

macro_rules! mac {
() => {
fn test3() -> Result<(), Box<dyn std::error::Error>> {
let mut _file = File::create("foo.txt")?;
//~^ ERROR the `?` operator can only be used in a function
println!();

Ok(())
}
};
}

fn main() -> Result<(), Box<dyn std::error::Error>> {
let mut _file = File::create("foo.txt")?;
//~^ ERROR the `?` operator can only be used in a function
mac!();

Ok(())
}
34 changes: 34 additions & 0 deletions tests/ui/return/return-from-residual-sugg-issue-125997.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
//@ run-rustfix

#![allow(unused_imports)]
#![allow(dead_code)]

use std::fs::File;
use std::io::prelude::*;

fn test1() {
let mut _file = File::create("foo.txt")?;
//~^ ERROR the `?` operator can only be used in a function
}

fn test2() {
let mut _file = File::create("foo.txt")?;
//~^ ERROR the `?` operator can only be used in a function
println!();
}

macro_rules! mac {
() => {
fn test3() {
let mut _file = File::create("foo.txt")?;
//~^ ERROR the `?` operator can only be used in a function
println!();
}
};
}

fn main() {
let mut _file = File::create("foo.txt")?;
//~^ ERROR the `?` operator can only be used in a function
mac!();
}
86 changes: 86 additions & 0 deletions tests/ui/return/return-from-residual-sugg-issue-125997.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
error[E0277]: the `?` operator can only be used in a function that returns `Result` or `Option` (or another type that implements `FromResidual`)
--> $DIR/return-from-residual-sugg-issue-125997.rs:10:44
|
LL | fn test1() {
| ---------- this function should return `Result` or `Option` to accept `?`
LL | let mut _file = File::create("foo.txt")?;
| ^ cannot use the `?` operator in a function that returns `()`
|
= help: the trait `FromResidual<Result<Infallible, std::io::Error>>` is not implemented for `()`
help: consider adding return type
|
LL ~ fn test1() -> Result<(), Box<dyn std::error::Error>> {
LL | let mut _file = File::create("foo.txt")?;
LL |
LL +
LL + Ok(())
LL + }
|

error[E0277]: the `?` operator can only be used in a function that returns `Result` or `Option` (or another type that implements `FromResidual`)
--> $DIR/return-from-residual-sugg-issue-125997.rs:15:44
|
LL | fn test2() {
| ---------- this function should return `Result` or `Option` to accept `?`
LL | let mut _file = File::create("foo.txt")?;
| ^ cannot use the `?` operator in a function that returns `()`
|
= help: the trait `FromResidual<Result<Infallible, std::io::Error>>` is not implemented for `()`
help: consider adding return type
|
LL ~ fn test2() -> Result<(), Box<dyn std::error::Error>> {
LL | let mut _file = File::create("foo.txt")?;
LL |
LL | println!();
LL +
LL + Ok(())
LL + }
|

error[E0277]: the `?` operator can only be used in a function that returns `Result` or `Option` (or another type that implements `FromResidual`)
--> $DIR/return-from-residual-sugg-issue-125997.rs:31:44
|
LL | fn main() {
| --------- this function should return `Result` or `Option` to accept `?`
LL | let mut _file = File::create("foo.txt")?;
| ^ cannot use the `?` operator in a function that returns `()`
|
= help: the trait `FromResidual<Result<Infallible, std::io::Error>>` is not implemented for `()`
help: consider adding return type
|
LL ~ fn main() -> Result<(), Box<dyn std::error::Error>> {
LL | let mut _file = File::create("foo.txt")?;
LL |
LL | mac!();
LL +
LL + Ok(())
LL + }
|

error[E0277]: the `?` operator can only be used in a function that returns `Result` or `Option` (or another type that implements `FromResidual`)
--> $DIR/return-from-residual-sugg-issue-125997.rs:23:52
|
LL | fn test3() {
| ---------- this function should return `Result` or `Option` to accept `?`
LL | let mut _file = File::create("foo.txt")?;
| ^ cannot use the `?` operator in a function that returns `()`
...
LL | mac!();
| ------ in this macro invocation
|
= help: the trait `FromResidual<Result<Infallible, std::io::Error>>` is not implemented for `()`
= note: this error originates in the macro `mac` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider adding return type
|
LL ~ fn test3() -> Result<(), Box<dyn std::error::Error>> {
LL | let mut _file = File::create("foo.txt")?;
LL |
LL | println!();
LL ~
LL + Ok(())
LL + }
|

error: aborting due to 4 previous errors

For more information about this error, try `rustc --explain E0277`.
10 changes: 10 additions & 0 deletions tests/ui/try-trait/try-operator-on-main.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,16 @@ LL | std::fs::File::open("foo")?;
| ^ cannot use the `?` operator in a function that returns `()`
|
= help: the trait `FromResidual<Result<Infallible, std::io::Error>>` is not implemented for `()`
help: consider adding return type
|
LL ~ fn main() -> Result<(), Box<dyn std::error::Error>> {
LL | // error for a `Try` type on a non-`Try` fn
...
LL | try_trait_generic::<()>();
LL +
LL + Ok(())
LL + }
|

error[E0277]: the `?` operator can only be applied to values that implement `Try`
--> $DIR/try-operator-on-main.rs:10:5
Expand Down
Loading