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

Provide context when ? can't be called because of Result<_, E> #116496

Merged
merged 4 commits into from
Dec 6, 2023

Conversation

estebank
Copy link
Contributor

@estebank estebank commented Oct 6, 2023

When a method chain ending in ? causes an E0277 because the expression's Result::Err variant doesn't have a type that can be converted to the Result<_, E> type parameter in the return type, provide additional context of which parts of the chain can and can't support the ? operator.

error[E0277]: `?` couldn't convert the error to `String`
  --> $DIR/question-mark-result-err-mismatch.rs:27:25
   |
LL | fn bar() -> Result<(), String> {
   |             ------------------ expected `String` because of this
LL |     let x = foo();
   |             ----- this has type `Result<_, String>`
...
LL |         .map_err(|_| ())?;
   |          ---------------^ the trait `From<()>` is not implemented for `String`
   |          |
   |          this can't be annotated with `?` because it has type `Result<_, ()>`
   |
   = note: the question mark operation (`?`) implicitly performs a conversion on the error value using the `From` trait
   = help: the following other types implement trait `From<T>`:
             <String as From<char>>
             <String as From<Box<str>>>
             <String as From<Cow<'a, str>>>
             <String as From<&str>>
             <String as From<&mut str>>
             <String as From<&String>>
   = note: required for `Result<(), String>` to implement `FromResidual<Result<Infallible, ()>>`

Fix #72124.

@rustbot
Copy link
Collaborator

rustbot commented Oct 6, 2023

r? @wesleywiser

(rustbot has picked a reviewer for you, use r? to override)

@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 Oct 6, 2023
@rust-log-analyzer

This comment has been minimized.

Comment on lines 13 to 20
LL | .map(|s| ())
| ----------- this can be annotated with `?` because it has type `Result<(), &str>`
LL | .map_err(|_| ())
| --------------- this can't be annotated with `?` because it has type `Result<(), ()>`
LL | .map(|()| "")?;
| ------------^ the trait `From<()>` is not implemented for `String`
| |
| this can't be annotated with `?` because it has type `Result<&str, ()>`
Copy link
Member

Choose a reason for hiding this comment

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

this seems really heavy for long chains -- do i really care about ?-ability 4 .maps down?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably not. I would like to add an extra check to only point at places where the E changed (like we do for assoc types in method call chains), but wanted to keep the code down to a reviewable size.

Copy link
Member

Choose a reason for hiding this comment

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

I'd rather just put a hard limit on the number of method chain to walk backwards. Maybe 2 is sensible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know what you think of the new output.

@compiler-errors
Copy link
Member

Also, this doesn't really fix #72124, does it? The problem in that issue is a shortcoming of closure inference causing us to forgo suggesting removing ; in a closure body?

@estebank
Copy link
Contributor Author

estebank commented Oct 6, 2023

Also, this doesn't really fix #72124, does it?

I'm adding some checks for the specific methods that take closures and that can change the type of Result<_, E>, but we still need the general context for any other cases that have underlying reasons beyond map_err calls.

@estebank estebank force-pushed the question-method-chain-context branch from bd7a587 to 19f23b6 Compare October 6, 2023 21:06
@compiler-errors
Copy link
Member

I'm adding some checks for the specific methods that take closures

For what it's worth, I think those checks should be implemented in a separate PR to separate concerns.

I'm mostly just noting that the PR shouldn't close (via fixes ### in the PR summary) that issue since there's additional work that needs to be done on that case specifically.

@estebank estebank force-pushed the question-method-chain-context branch from bb20f12 to b09ff86 Compare October 7, 2023 04:12
@bors

This comment was marked as resolved.

@estebank estebank force-pushed the question-method-chain-context branch from 9688267 to c55e70e Compare October 15, 2023 22:36
@bors

This comment was marked as resolved.

@estebank estebank force-pushed the question-method-chain-context branch from c55e70e to 0e8b80a Compare October 27, 2023 17:39
@bors

This comment was marked as resolved.

@compiler-errors
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Dec 5, 2023

📌 Commit cce82d8 has been approved by compiler-errors

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 Dec 5, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 5, 2023
…text, r=compiler-errors

Provide context when `?` can't be called because of `Result<_, E>`

When a method chain ending in `?` causes an E0277 because the expression's `Result::Err` variant doesn't have a type that can be converted to the `Result<_, E>` type parameter in the return type, provide additional context of which parts of the chain can and can't support the `?` operator.

```
error[E0277]: `?` couldn't convert the error to `String`
  --> $DIR/question-mark-result-err-mismatch.rs:27:25
   |
LL | fn bar() -> Result<(), String> {
   |             ------------------ expected `String` because of this
LL |     let x = foo();
   |             ----- this has type `Result<_, String>`
...
LL |         .map_err(|_| ())?;
   |          ---------------^ the trait `From<()>` is not implemented for `String`
   |          |
   |          this can't be annotated with `?` because it has type `Result<_, ()>`
   |
   = note: the question mark operation (`?`) implicitly performs a conversion on the error value using the `From` trait
   = help: the following other types implement trait `From<T>`:
             <String as From<char>>
             <String as From<Box<str>>>
             <String as From<Cow<'a, str>>>
             <String as From<&str>>
             <String as From<&mut str>>
             <String as From<&String>>
   = note: required for `Result<(), String>` to implement `FromResidual<Result<Infallible, ()>>`
```

Fix rust-lang#72124.
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Dec 5, 2023
…text, r=compiler-errors

Provide context when `?` can't be called because of `Result<_, E>`

When a method chain ending in `?` causes an E0277 because the expression's `Result::Err` variant doesn't have a type that can be converted to the `Result<_, E>` type parameter in the return type, provide additional context of which parts of the chain can and can't support the `?` operator.

```
error[E0277]: `?` couldn't convert the error to `String`
  --> $DIR/question-mark-result-err-mismatch.rs:27:25
   |
LL | fn bar() -> Result<(), String> {
   |             ------------------ expected `String` because of this
LL |     let x = foo();
   |             ----- this has type `Result<_, String>`
...
LL |         .map_err(|_| ())?;
   |          ---------------^ the trait `From<()>` is not implemented for `String`
   |          |
   |          this can't be annotated with `?` because it has type `Result<_, ()>`
   |
   = note: the question mark operation (`?`) implicitly performs a conversion on the error value using the `From` trait
   = help: the following other types implement trait `From<T>`:
             <String as From<char>>
             <String as From<Box<str>>>
             <String as From<Cow<'a, str>>>
             <String as From<&str>>
             <String as From<&mut str>>
             <String as From<&String>>
   = note: required for `Result<(), String>` to implement `FromResidual<Result<Infallible, ()>>`
```

Fix rust-lang#72124.
hir::intravisit::walk_expr(self, ex);
}
}
let hir_id = self.tcx.hir().local_def_id_to_hir_id(obligation.cause.body_id);
Copy link
Member

Choose a reason for hiding this comment

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

This needs a rebase.

Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

please amend commit 2faaea9 after rebasing because local_def_id_to_hir_id has moved onto TyCtxt.

@rustbot rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Dec 5, 2023
@compiler-errors
Copy link
Member

@bors r-

r=me after fixing up that first commit

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Dec 5, 2023
@compiler-errors
Copy link
Member

(also if you're already changing this, pls squash the formatting commits down into more relevant ones)

When a method chain ending in `?` causes an E0277 because the
expression's `Result::Err` variant doesn't have a type that can be
converted to the `Result<_, E>` type parameter in the return type,
provide additional context of which parts of the chain can and can't
support the `?` operator.

```
error[E0277]: `?` couldn't convert the error to `String`
  --> $DIR/question-mark-result-err-mismatch.rs:28:25
   |
LL | fn bar() -> Result<(), String> {
   |             ------------------ expected `String` because of this
LL |     let x = foo();
   |             ----- this can be annotated with `?` because it has type `Result<String, String>`
LL |     let one = x
LL |         .map(|s| ())
   |          ----------- this can be annotated with `?` because it has type `Result<(), String>`
LL |         .map_err(|_| ())?;
   |          ---------------^ the trait `From<()>` is not implemented for `String`
   |          |
   |          this can't be annotated with `?` because it has type `Result<(), ()>`
   |
   = note: the question mark operation (`?`) implicitly performs a conversion on the error value using the `From` trait
   = help: the following other types implement trait `From<T>`:
             <String as From<char>>
             <String as From<Box<str>>>
             <String as From<Cow<'a, str>>>
             <String as From<&str>>
             <String as From<&mut str>>
             <String as From<&String>>
   = note: required for `Result<(), String>` to implement `FromResidual<Result<Infallible, ()>>`
```

Fix rust-lang#72124.
@estebank estebank force-pushed the question-method-chain-context branch from cce82d8 to 23fa94f Compare December 5, 2023 22:22
@compiler-errors
Copy link
Member

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Dec 5, 2023

📌 Commit 23fa94f has been approved by compiler-errors

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 5, 2023
@estebank estebank force-pushed the question-method-chain-context branch from 23fa94f to 70fe624 Compare December 5, 2023 22:24
@estebank
Copy link
Contributor Author

estebank commented Dec 5, 2023

Pushed again (squashed the comment adding commit too)

@bors r=compiler-errors

@bors
Copy link
Contributor

bors commented Dec 5, 2023

📌 Commit 70fe624 has been approved by compiler-errors

It is now in the queue for this repository.

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 6, 2023
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#116496 (Provide context when `?` can't be called because of `Result<_, E>`)
 - rust-lang#117563 (docs: clarify explicitly freeing heap allocated memory)
 - rust-lang#117874 (`riscv32` platform support)
 - rust-lang#118516 (Add ADT variant infomation to StableMIR and finish implementing TyKind::internal())
 - rust-lang#118650 (add comment about keeping flags in sync between bootstrap.py and bootstrap.rs)
 - rust-lang#118664 (docs: remove rust-lang#110800 from release notes)
 - rust-lang#118669 (library: fix comment about const assert in win api)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f1e1804 into rust-lang:master Dec 6, 2023
11 checks passed
@rustbot rustbot added this to the 1.76.0 milestone Dec 6, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 6, 2023
Rollup merge of rust-lang#116496 - estebank:question-method-chain-context, r=compiler-errors

Provide context when `?` can't be called because of `Result<_, E>`

When a method chain ending in `?` causes an E0277 because the expression's `Result::Err` variant doesn't have a type that can be converted to the `Result<_, E>` type parameter in the return type, provide additional context of which parts of the chain can and can't support the `?` operator.

```
error[E0277]: `?` couldn't convert the error to `String`
  --> $DIR/question-mark-result-err-mismatch.rs:27:25
   |
LL | fn bar() -> Result<(), String> {
   |             ------------------ expected `String` because of this
LL |     let x = foo();
   |             ----- this has type `Result<_, String>`
...
LL |         .map_err(|_| ())?;
   |          ---------------^ the trait `From<()>` is not implemented for `String`
   |          |
   |          this can't be annotated with `?` because it has type `Result<_, ()>`
   |
   = note: the question mark operation (`?`) implicitly performs a conversion on the error value using the `From` trait
   = help: the following other types implement trait `From<T>`:
             <String as From<char>>
             <String as From<Box<str>>>
             <String as From<Cow<'a, str>>>
             <String as From<&str>>
             <String as From<&mut str>>
             <String as From<&String>>
   = note: required for `Result<(), String>` to implement `FromResidual<Result<Infallible, ()>>`
```

Fix rust-lang#72124.
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.

Rustc fails to suggest an easy fix; instead provides a confusing error message
6 participants