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
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Jun 12, 2024
For E0277 suggest adding `Result` return type for function when using QuestionMark `?` in the body.

Adding suggestions for following function in E0277.

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

to

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

    return Ok(());
}
```

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

<!--
If this PR is related to an unstable feature or an otherwise tracked effort,
please link to the relevant tracking issue here. If you don't know of a related
tracking issue or there are none, feel free to ignore this.

This PR will get automatically assigned to a reviewer. In case you would like
a specific user to review your work, you can assign it to them by using

    r​? <reviewer name>
-->
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 12, 2024
…kingjubilee

Rollup of 16 pull requests

Successful merges:

 - rust-lang#123374 (DOC: Add FFI example for slice::from_raw_parts())
 - rust-lang#124514 (Recommend to never display zero disambiguators when demangling v0 symbols)
 - rust-lang#125978 (Cleanup: HIR ty lowering: Consolidate the places that do assoc item probing & access checking)
 - rust-lang#125980 (Nvptx remove direct passmode)
 - rust-lang#126187 (For E0277 suggest adding `Result` return type for function when using QuestionMark `?` in the body.)
 - rust-lang#126210 (docs(core): make more const_ptr doctests assert instead of printing)
 - rust-lang#126249 (Simplify `[T; N]::try_map` signature)
 - rust-lang#126256 (Add {{target}} substitution to compiletest)
 - rust-lang#126263 (Make issue-122805.rs big endian compatible)
 - rust-lang#126281 (set_env: State the conclusion upfront)
 - rust-lang#126286 (Make `storage-live.rs` robust against rustc internal changes.)
 - rust-lang#126287 (Update a cranelift patch file for formatting changes.)
 - rust-lang#126301 (Use `tidy` to sort crate attributes for all compiler crates.)
 - rust-lang#126305 (Make PathBuf less Ok with adding UTF-16 then `into_string`)
 - rust-lang#126310 (Migrate run make prefer rlib)
 - rust-lang#126314 (fix RELEASES: we do not support upcasting to auto traits)

r? `@ghost`
`@rustbot` modify labels: rollup
@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
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 12, 2024
Rollup merge of rust-lang#126187 - surechen:fix_125997, r=oli-obk

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

Adding suggestions for following function in E0277.

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

to

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

    return Ok(());
}
```

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

<!--
If this PR is related to an unstable feature or an otherwise tracked effort,
please link to the relevant tracking issue here. If you don't know of a related
tracking issue or there are none, feel free to ignore this.

This PR will get automatically assigned to a reviewer. In case you would like
a specific user to review your work, you can assign it to them by using

    r​? <reviewer name>
-->
bjorn3 pushed a commit to bjorn3/rust that referenced this pull request Jun 30, 2024
…kingjubilee

Rollup of 16 pull requests

Successful merges:

 - rust-lang#123374 (DOC: Add FFI example for slice::from_raw_parts())
 - rust-lang#124514 (Recommend to never display zero disambiguators when demangling v0 symbols)
 - rust-lang#125978 (Cleanup: HIR ty lowering: Consolidate the places that do assoc item probing & access checking)
 - rust-lang#125980 (Nvptx remove direct passmode)
 - rust-lang#126187 (For E0277 suggest adding `Result` return type for function when using QuestionMark `?` in the body.)
 - rust-lang#126210 (docs(core): make more const_ptr doctests assert instead of printing)
 - rust-lang#126249 (Simplify `[T; N]::try_map` signature)
 - rust-lang#126256 (Add {{target}} substitution to compiletest)
 - rust-lang#126263 (Make issue-122805.rs big endian compatible)
 - rust-lang#126281 (set_env: State the conclusion upfront)
 - rust-lang#126286 (Make `storage-live.rs` robust against rustc internal changes.)
 - rust-lang#126287 (Update a cranelift patch file for formatting changes.)
 - rust-lang#126301 (Use `tidy` to sort crate attributes for all compiler crates.)
 - rust-lang#126305 (Make PathBuf less Ok with adding UTF-16 then `into_string`)
 - rust-lang#126310 (Migrate run make prefer rlib)
 - rust-lang#126314 (fix RELEASES: we do not support upcasting to auto traits)

r? `@ghost`
`@rustbot` modify labels: rollup
surechen added a commit to surechen/rust that referenced this pull request Aug 22, 2024
… by rust-lang#126187

According to:
rust-lang#128084 (comment)
https://github.com/rust-lang/rust/pull/126187/files#r1634897691

I try to add a span field to `Block` which doesn't include brace '{' and '}', because I need to add a suggestion at the end of function's body, and this can help me find the right place. But this will make `Block` larger.

I don't want to break the original span field in `Block`, I'm worried that it will cause a lot of code failures and I think it may be more intuitive for span to include braces.
surechen added a commit to surechen/rust that referenced this pull request Aug 22, 2024
… by rust-lang#126187

According to:
rust-lang#128084 (comment)
https://github.com/rust-lang/rust/pull/126187/files#r1634897691

I try to add a span field to `Block` which doesn't include brace '{' and '}', because I need to add a suggestion at the end of function's body, and this can help me find the right place. But this will make `Block` larger.

I don't want to break the original span field in `Block`, I'm worried that it will cause a lot of code failures and I think it may be more intuitive for span to include braces.
surechen added a commit to surechen/rust that referenced this pull request Aug 23, 2024
… by rust-lang#126187

According to:
rust-lang#128084 (comment)
https://github.com/rust-lang/rust/pull/126187/files#r1634897691

I try to add a span field to `Block` which doesn't include brace '{' and '}', because I need to add a suggestion at the end of function's body, and this can help me find the right place. But this will make `Block` larger.

I don't want to break the original span field in `Block`, I'm worried that it will cause a lot of code failures and I think it may be more intuitive for span to include braces.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 25, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 25, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#129207 (Lint that warns when an elided lifetime ends up being a named lifetime)
 - rust-lang#129288 (Use subtyping for `UnsafeFnPointer` coercion, too)
 - rust-lang#129405 (Fixing span manipulation and indentation of the suggestion introduced by rust-lang#126187)
 - rust-lang#129518 (gitignore: ignore ICE reports regardless of directory)
 - rust-lang#129519 (Remove redundant flags from `lower_ty_common` that can be inferred from the HIR)
 - rust-lang#129544 (Removes dead code from the compiler)
 - rust-lang#129553 (add back test for stable-const-can-only-call-stable-const)
 - rust-lang#129590 (Avoid taking reference of &TyKind)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 25, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 26, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#129288 (Use subtyping for `UnsafeFnPointer` coercion, too)
 - rust-lang#129405 (Fixing span manipulation and indentation of the suggestion introduced by rust-lang#126187)
 - rust-lang#129518 (gitignore: ignore ICE reports regardless of directory)
 - rust-lang#129519 (Remove redundant flags from `lower_ty_common` that can be inferred from the HIR)
 - rust-lang#129525 (rustdoc: clean up tuple <-> primitive conversion docs)
 - rust-lang#129526 (Use `FxHasher` on new solver unconditionally)
 - rust-lang#129544 (Removes dead code from the compiler)
 - rust-lang#129553 (add back test for stable-const-can-only-call-stable-const)
 - rust-lang#129590 (Avoid taking reference of &TyKind)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 26, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#129288 (Use subtyping for `UnsafeFnPointer` coercion, too)
 - rust-lang#129405 (Fixing span manipulation and indentation of the suggestion introduced by rust-lang#126187)
 - rust-lang#129518 (gitignore: ignore ICE reports regardless of directory)
 - rust-lang#129519 (Remove redundant flags from `lower_ty_common` that can be inferred from the HIR)
 - rust-lang#129525 (rustdoc: clean up tuple <-> primitive conversion docs)
 - rust-lang#129526 (Use `FxHasher` on new solver unconditionally)
 - rust-lang#129544 (Removes dead code from the compiler)
 - rust-lang#129553 (add back test for stable-const-can-only-call-stable-const)
 - rust-lang#129590 (Avoid taking reference of &TyKind)

r? `@ghost`
`@rustbot` modify labels: rollup
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 26, 2024
Rollup merge of rust-lang#129405 - surechen:fix_span_x, r=cjgillot

Fixing span manipulation and indentation of the suggestion introduced by rust-lang#126187

According to comments:
rust-lang#128084 (comment)
https://github.com/rust-lang/rust/pull/126187/files#r1634897691
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