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

Very hard to read error when returning ? from closure not returning Result or Option #69657

Open
jyn514 opened this issue Mar 3, 2020 · 6 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix`. C-enhancement Category: An issue proposing an enhancement or a PR with one. D-papercut Diagnostics: An error or lint that needs small tweaks. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@jyn514
Copy link
Member

jyn514 commented Mar 3, 2020

The original code:

    // '[' expr ']' | '(' argument* ')' | '.' ID | '->' ID | '++' | '--'
    fn match_postfix_op(&mut self) -> SyntaxResult<Option<Locatable<impl Fn(Expr) -> ExprType>>> {
        let needs_id = |this: &mut Self, constructor: fn(Box<Expr>, InternedStr) -> ExprType| {
            let start = this.next_token().unwrap().location;
            let Locatable { data: id, location } = this.expect_id()?;
            let location = start.merge(&location);
            (move |expr| constructor(expr, id), location)
        };
        // prefix operator
        let (func, location) = match self.peek_token() {
            Some(Token::Dot) => needs_id(self, ExprType::Member)?,
            _ => return Ok(None),
        };
        Ok(Some(Locatable {
            data: move |e| func(Box::new(e)),
            location,
        }))
    }
error[E0277]: the `?` operator can only be used in a closure that returns `Result` or `Option` (or another type that implements `std::ops::Try`)
   --> src/expr.rs:237:52
    |
235 |           let needs_id = |this: &mut Self, constructor: fn(Box<Expr>, InternedStr) -> ExprType| {
    |  ________________________-
236 | |             let start = this.next_token().unwrap().location;
237 | |             let Locatable { data: id, location } = this.expect_id()?;
    | |                                                    ^^^^^^^^^^^^^^^^^ cannot use the `?` operator in a closure that returns `([closure@src/expr.rs:239:14: 239:47 constructor:_, id:_], data::lex::DefaultLocation)`
238 | |             let location = start.merge(&location);
239 | |             (move |expr| constructor(expr, id), location)
240 | |         };
    | |_________- this function should return `Result` or `Option` to accept `?`
    |
    = help: the trait `std::ops::Try` is not implemented for `([closure@src/expr.rs:239:14: 239:47 constructor:_, id:_], data::lex::DefaultLocation)`
    = note: required by `std::ops::Try::from_error`

error[E0277]: the `?` operator can only be applied to values that implement `std::ops::Try`
   --> src/expr.rs:243:33
    |
243 |             Some(Token::Dot) => needs_id(self, ExprType::Member)?,
    |                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the `?` operator cannot be applied to type `([closure@src/expr.rs:239:14: 239:47 constructor:_, id:_], data::lex::DefaultLocation)`
    |
    = help: the trait `std::ops::Try` is not implemented for `([closure@src/expr.rs:239:14: 239:47 constructor:_, id:_], data::lex::DefaultLocation)`
    = note: required by `std::ops::Try::into_result`

error: aborting due to 2 previous errors

The fix was to return Ok() at the end of the closure: Ok((move |expr| constructor(expr, id), location)). It would be much more helpful if the error message suggested returning Ok() at the end instead of spouting a wall of text at me.

@jyn514
Copy link
Member Author

jyn514 commented Mar 3, 2020

Minimized: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=fe170e5b26850bb57a1e82f0fcae117c

fn returns_err() -> Result<(), ()> {
    Err(())
}

fn match_postfix_op() -> Result<usize, ()> {
    let closure = || {
        let _ = returns_err()?;
        1
    };
    let i = closure()?;
    Ok(i)
}

@jyn514
Copy link
Member Author

jyn514 commented Mar 3, 2020

Related issue: the closure types just look like noise. Maybe rustc could only print the location, not all the arguments? closure@src/expr.rs:239:14

@LeSeulArtichaut
Copy link
Contributor

I think we can @rustbot modify labels: A-diagnostics D-papercut

@rustbot rustbot added A-diagnostics Area: Messages for errors, warnings, and lints D-papercut Diagnostics: An error or lint that needs small tweaks. labels Mar 3, 2020
@JohnTitor JohnTitor added A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix`. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 4, 2020
@estebank
Copy link
Contributor

I believe the current output is an improvement over the original report:

error[E0277]: the `?` operator can only be used in a closure that returns `Result` or `Option` (or another type that implements `FromResidual`)
 --> src/lib.rs:7:30
  |
6 |     let closure = || {
  |                   -- this function should return `Result` or `Option` to accept `?`
7 |         let _ = returns_err()?;
  |                              ^ cannot use the `?` operator in a closure that returns `{integer}`
  |
  = help: the trait `FromResidual<Result<Infallible, ()>>` is not implemented for `{integer}`

error[E0277]: the `?` operator can only be applied to values that implement `Try`
  --> src/lib.rs:11:13
   |
11 |     let i = closure()?;
   |             ^^^^^^^^^^ the `?` operator cannot be applied to type `{integer}`
   |
   = help: the trait `Try` is not implemented for `{integer}`

@jyn514
Copy link
Member Author

jyn514 commented Jun 30, 2023

it's better - "Try not implemented for integer" is the actual problem, but it's hard to spot. it would be nice if it pointed to line 8 with 1 as the reason the return type is inferred to be {integer}.

@estebank
Copy link
Contributor

I think that could be done. We already walk the hir tree to point at where inference kicks in.

@estebank estebank reopened this Jun 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix`. C-enhancement Category: An issue proposing an enhancement or a PR with one. D-papercut Diagnostics: An error or lint that needs small tweaks. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants