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

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

Conversation

surechen
Copy link
Contributor

@surechen surechen commented Jun 9, 2024

Adding suggestions for following function in E0277.

fn main() {
    let mut _file = File::create("foo.txt")?;
}

to

fn main() -> Result<(), Box<dyn std::error::Error>> {
    let mut _file = File::create("foo.txt")?;

    return Ok(());
}

According to the issue #125997, only the code examples in the issue are targeted, but the issue covers a wider range of situations.

@rustbot
Copy link
Collaborator

rustbot commented Jun 9, 2024

r? @oli-obk

rustbot has assigned @oli-obk.
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 S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 9, 2024
@surechen surechen changed the title For E0277 suggest adding Result return type for function which using QuestionMark ? in the body. For E0277 suggest adding Result return type for function when using QuestionMark ? in the body. Jun 9, 2024
if let hir::ExprKind::Block(b, _) = body.value.kind
&& b.expr.is_none()
{
// The span will point to the closing curly brace `}` of the block.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add this information as a doc comment to the span field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please add this information as a doc comment to the span field.

Thank you.
Sorry, I didn't understand, what should I do?

Copy link
Contributor

Choose a reason for hiding this comment

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

Add documentation to https://doc.rust-lang.org/nightly/nightly-rustc/rustc_hir/hir/struct.Block.html#structfield.span stating that this span includes the squirly brackets around the block

{
// The span will point to the closing curly brace `}` of the block.
sugg_spans.push((
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.

@oli-obk
Copy link
Contributor

oli-obk commented Jun 12, 2024

Thanks for looking into the details!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jun 12, 2024

📌 Commit 0b3fec9 has been approved by oli-obk

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 12, 2024
@bors bors merged commit 519a322 into rust-lang:master Jun 12, 2024
6 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants