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

Emit warnings on parameter list in closures after { #27300

Open
muja opened this issue Jul 26, 2015 · 8 comments
Open

Emit warnings on parameter list in closures after { #27300

muja opened this issue Jul 26, 2015 · 8 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. P-low Low priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@muja
Copy link

muja commented Jul 26, 2015

Reproducing steps

This code compiles (and, in my opinion, it shouldn't):

let p = Some(45).and_then({|x|
    Some(x * 2)
});

While this doesn't:

let p = Some(45).and_then({|x|
    println!("doubling {}", x);
    Some(x * 2)
})

Error message 1 (using x)

The error message is absolutely unintuitive and won't help anyone:

<anon>:4:14: 4:15 error: unresolved name `x` [E0425]
<anon>:4         Some(x * 2)
                      ^
error: aborting due to previous error

Playpen link

Error message 2 (not using x)

When the last expression does not use a parameter from the list, the generated error message is even more frightening to newcomers (albeit in retrospect, this one makes more sense):

<anon>:2:22: 5:7 error: the trait `core::ops::FnOnce<(_,)>` is not implemented for the type `core::option::Option<_>` [E0277]
<anon>:2     let p = Some(45).and_then({|x|
<anon>:3         println!("doubling {}", x);
<anon>:4         Some(100 * 2)
<anon>:5     });
<anon>:2:22: 5:7 help: see the detailed explanation for E0277
<anon>:2:22: 5:7 error: the trait `core::ops::FnOnce<(_,)>` is not implemented for the type `core::option::Option<_>` [E0277]
<anon>:2     let p = Some(45).and_then({|x|
<anon>:3         println!("doubling {}", x);
<anon>:4         Some(100 * 2)
<anon>:5     });
<anon>:2:22: 5:7 help: see the detailed explanation for E0277
error: aborting due to 2 previous errors

Playpen link

Proposed solution

Emit a warning when encountering this fallible syntax.

Anecdotic background

Having used a lot of Ruby, I had grown way accustomed to the { |x| x * 2 } syntax. Although I had seen the proper Rust syntax a couple of times, it never was emphasized and the significance never stuck. I spent 30+ minutes trying to figure out why a println! statement breaks the build, generating a random error message that has no obvious connections to the actual bug at hand. Only after inquiring the IRC channel did a solution unveil itself. It shouldn't happen.

@Xirdus
Copy link

Xirdus commented Jul 26, 2015

I don't think we should make custom-tailored error messages for people trying to write code in their favorite language and then compile it with rustc. Just like we don't issue any special warning on assigning result of assignment to a variable, we shouldn't issue a warning when someone tries to write Ruby lambdas in Rust. People should learn that even if something looks similar, it doesn't mean that it works the same as in their old language.

@muja
Copy link
Author

muja commented Jul 26, 2015

@Xirdus nobody said anything about custom tailored messages and the problem is that the code from Ruby does compile in the first place (see the very first code snippet). Changing that, however, would be a breaking change, so instead, I proposed the solution as a warning that actually makes sense! Maybe don't even change the warning, just make a "note: did you mean |x| { ... }" ? It would help a lot of people and Rubyists are a target group of the Rust language.

@killercup
Copy link
Member

This code compiles (and, in my opinion, it shouldn't):

Just to clarify (for you and anyone who may stumble over this syntax in the future and find this issue):

let p = Some(45).and_then({|x|
    Some(x * 2)
});

is the same as

let p = Some(45).and_then({
    // This is a block with only one expression.
    // It returns this expression (the closure):
    |x| { Some(x * 2) } // braces optional
});

This is totally legitimate and should compile.

Your other example

let p = Some(45).and_then({|x|
    println!("doubling {}", x);
    Some(x * 2)
})

could be written as

let p = Some(45).and_then({
    |x| { println!("doubling {}", x) }; // braces optional
    Some(x * 2) // returns `Some(i32)`
})

I understand why rustc's error message is confusing. A solution would be to reformat your code shown in the error and annotating the blocks. (This might make stuff more confusing in other cases, though!)

@muja
Copy link
Author

muja commented Jul 26, 2015

@killercup thanks for the response! So you're saying we need rustfmt? I'm all for that and if that fixes the issue, I don't see a problem!

@killercup
Copy link
Member

@muja you can already use https://github.com/nrc/rustfmt! (That isn't integrated into the compiler, though, so the error snippets won't get pretty by themselves.)

@stevenblenkinsop
Copy link

In the first case, there could plausibly be a hint if you try to use a closure parameter in a subsequent statement, such as

<anon>:4:14: 4:15 error: unresolved name `x` [E0425]
<anon>:4         Some(x * 2)
                      ^
<anon>:2:33: 2:34 note: nearest candidate defined at 2:33...
<anon>:2    let p = Some(45).and_then({|x|
                                        ^
<anon>:3:35: 3:36 note: ...but goes out of scope at 3:35
<anon>:3        println!("doubling {}", x);
                                          ^
error: aborting due to previous error

For the second case, plausibly, when the expression for a closure isn't a block, you could have a hint:

<anon>:2:22: 5:7 error: the trait `core::ops::FnOnce<(_,)>` is not implemented for the type `core::option::Option<_>` [E0277]
<anon>:2     let p = Some(45).and_then({|x|
<anon>:3         println!("doubling {}", x);
<anon>:4         Some(100 * 2)
<anon>:5     });
<anon>:2:22: 5:7 help: see the detailed explanation for E0277
<anon>:4:9: 4:22 note: the expression at 4:9...
<anon>:4         Some(100 * 2)
                 ^~~~~~~~~~~~~
<anon>:2:32: 3:35 note: ...may have been intended to be part of closure expression at 2:32...
<anon>:2     let p = Some(45).and_then({|x|
<anon>:3         println!("doubling {}", x);
<anon>:3:34: 3:35 note: ...but closure expression ends at 3:34
<anon>:3         println!("doubling {}", x);
                                          ^
error: aborting due to previous error

@withoutboats
Copy link
Contributor

Note that if the unbound variable error hadn't arisen (e.g. because x was defined in the outer scope), this code would have not compiled because the block evaluates to Some(x * 2), which does not implement FnOnce(()) -> Option<U>. You just almost definitely never want to pass a block to any parameter that expects a function, so there's that.

There are a couple of different ways rustc could generate a more helpful error message here. If an unbound variable is found in an identifier in the same scope as a previous closure statement that uses that identifier, it could point out that the statement is not inside the closure. If a closure is the first statement of a block and not bound with a let statement, it could remind you of Rust's closure syntax. Both of these would be reasonable, and helpful to users confused about closure syntax, though I don't know how easy either of them is.

Just like we don't issue any special warning on assigning result of assignment to a variable

This is off-topic, but it would be undoubtedly more helpful if a user wrote e.g. let x = let y = 0i32 and then attempted to use x as an i32, the compiler contained a note explaining why x was assigned to () and not a value of type i32. It is fundamentally good for the compiler to help correct confusion about the semantics of the language when that confusion leads to errors.

@steveklabnik steveklabnik added the A-diagnostics Area: Messages for errors, warnings, and lints label Jul 29, 2015
@brson brson added 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. P-low Low priority labels Jan 26, 2017
@steveklabnik steveklabnik removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 9, 2017
@estebank
Copy link
Contributor

Given #47763, overly verbose, but educative output actually pointing at the problem:

warning: a closure's body is not determined by its enclosing block
  --> $DIR/block-enclosed-closure.rs:14:33
   |
14 |     let _p = Some(45).and_then({|x|
   |                                 ^^^ this closure's body is not determined by its enclosing block
   |
note: this is the closure's block...
  --> $DIR/block-enclosed-closure.rs:14:33
   |
14 |       let _p = Some(45).and_then({|x|
   |  _________________________________^
17 | |         println!("hi");
   | |______________________^
note: ...while this enclosing block...
  --> $DIR/block-enclosed-closure.rs:14:32
   |
14 |       let _p = Some(45).and_then({|x|
   |  ________________________________^
17 | |         println!("hi");
18 | |         Some(x * 2)
19 | |     });
   | |_____^
note: ...implicitely returns
  --> $DIR/block-enclosed-closure.rs:18:9
   |
18 |         Some(x * 2)
   |         ^^^^^^^^^^^
help: you should open the block *after* the closure's argument list
   |
14 |     let _p = Some(45).and_then(|x| {
   |                                ^^^^^

error[E0425]: cannot find value `x` in this scope
  --> $DIR/block-enclosed-closure.rs:18:14
   |
18 |         Some(x * 2)
   |              ^ not found in this scope

error[E0277]: the trait bound `std::option::Option<_>: std::ops::FnOnce<({integer},)>` is not satisfied
  --> $DIR/block-enclosed-closure.rs:14:23
   |
14 |     let _p = Some(45).and_then({|x|
   |                       ^^^^^^^^ the trait `std::ops::FnOnce<({integer},)>` is not implemented for `std::option::Option<_>`

error: aborting due to 2 previous errors

@crlf0710 crlf0710 added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 11, 2020
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 16, 2021
… r=nagisa

Point at argument instead of call for their obligations

When an obligation is introduced by a specific `fn` argument, point at
the argument instead of the `fn` call if the obligation fails to be
fulfilled.

Move the information about pointing at the call argument expression in
an unmet obligation span from the `FulfillmentError` to a new
`ObligationCauseCode`.

When giving an error about an obligation introduced by a function call
that an argument doesn't fulfill, and that argument is a block, add a
span_label pointing at the innermost tail expression.

Current output:

```
error[E0425]: cannot find value `x` in this scope
 --> f10.rs:4:14
  |
4 |         Some(x * 2)
  |              ^ not found in this scope

error[E0277]: expected a `FnOnce<({integer},)>` closure, found `Option<_>`
 --> f10.rs:2:31
  |
2 |       let p = Some(45).and_then({
  |  ______________________--------_^
  | |                      |
  | |                      required by a bound introduced by this call
3 | |         |x| println!("doubling {}", x);
4 | |         Some(x * 2)
  | |         -----------
5 | |     });
  | |_____^ expected an `FnOnce<({integer},)>` closure, found `Option<_>`
  |
  = help: the trait `FnOnce<({integer},)>` is not implemented for `Option<_>`
```

Previous output:

```
error[E0425]: cannot find value `x` in this scope
 --> f10.rs:4:14
  |
4 |         Some(x * 2)
  |              ^ not found in this scope

error[E0277]: expected a `FnOnce<({integer},)>` closure, found `Option<_>`
 --> f10.rs:2:22
  |
2 |     let p = Some(45).and_then({
  |                      ^^^^^^^^ expected an `FnOnce<({integer},)>` closure, found `Option<_>`
  |
  = help: the trait `FnOnce<({integer},)>` is not implemented for `Option<_>`
```

Partially address rust-lang#27300. Will require rebasing on top of rust-lang#88546.
estebank added a commit to estebank/rust that referenced this issue Oct 23, 2023
Detect if there is a potential typo where the `{` meant to open the
closure body was written before the body.

```
error[E0277]: expected a `FnOnce<({integer},)>` closure, found `Option<usize>`
  --> $DIR/ruby_style_closure_successful_parse.rs:3:31
   |
LL |       let p = Some(45).and_then({|x|
   |  ______________________--------_^
   | |                      |
   | |                      required by a bound introduced by this call
LL | |         1 + 1;
LL | |         Some(x * 2)
   | |         ----------- this tail expression is of type `Option<usize>`
LL | |     });
   | |_____^ expected an `FnOnce<({integer},)>` closure, found `Option<usize>`
   |
   = help: the trait `FnOnce<({integer},)>` is not implemented for `Option<usize>`
note: required by a bound in `Option::<T>::and_then`
  --> $SRC_DIR/core/src/option.rs:LL:COL
help: you might have meant to open the closure body instead of placing a closure within a block
   |
LL -     let p = Some(45).and_then({|x|
LL +     let p = Some(45).and_then(|x| {
   |
```

Detect the potential typo where the closure header is missing.

```
error[E0277]: expected a `FnOnce<(&bool,)>` closure, found `bool`
  --> $DIR/block_instead_of_closure_in_arg.rs:3:23
   |
LL |        Some(true).filter({
   |  _________________------_^
   | |                 |
   | |                 required by a bound introduced by this call
LL | |/         if number % 2 == 0 {
LL | ||             number == 0
LL | ||         } else {
LL | ||             number != 0
LL | ||         }
   | ||_________- this tail expression is of type `bool`
LL | |      });
   | |______^ expected an `FnOnce<(&bool,)>` closure, found `bool`
   |
   = help: the trait `for<'a> FnOnce<(&'a bool,)>` is not implemented for `bool`
note: required by a bound in `Option::<T>::filter`
  --> $SRC_DIR/core/src/option.rs:LL:COL
help: you might have meant to create the closure instead of a block
   |
LL |     Some(true).filter(|_| {
   |                       +++
```

Partially address rust-lang#27300.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 26, 2023
When expecting closure argument but finding block provide suggestion

Detect if there is a potential typo where the `{` meant to open the closure body was written before the body.

```
error[E0277]: expected a `FnOnce<({integer},)>` closure, found `Option<usize>`
  --> $DIR/ruby_style_closure_successful_parse.rs:3:31
   |
LL |       let p = Some(45).and_then({|x|
   |  ______________________--------_^
   | |                      |
   | |                      required by a bound introduced by this call
LL | |         1 + 1;
LL | |         Some(x * 2)
   | |         ----------- this tail expression is of type `Option<usize>`
LL | |     });
   | |_____^ expected an `FnOnce<({integer},)>` closure, found `Option<usize>`
   |
   = help: the trait `FnOnce<({integer},)>` is not implemented for `Option<usize>`
note: required by a bound in `Option::<T>::and_then`
  --> $SRC_DIR/core/src/option.rs:LL:COL
help: you might have meant to open the closure body instead of placing a closure within a block
   |
LL -     let p = Some(45).and_then({|x|
LL +     let p = Some(45).and_then(|x| {
   |
```

Detect the potential typo where the closure header is missing.

```
error[E0277]: expected a `FnOnce<(&bool,)>` closure, found `bool`
  --> $DIR/block_instead_of_closure_in_arg.rs:3:23
   |
LL |        Some(true).filter({
   |  _________________------_^
   | |                 |
   | |                 required by a bound introduced by this call
LL | |/         if number % 2 == 0 {
LL | ||             number == 0
LL | ||         } else {
LL | ||             number != 0
LL | ||         }
   | ||_________- this tail expression is of type `bool`
LL | |      });
   | |______^ expected an `FnOnce<(&bool,)>` closure, found `bool`
   |
   = help: the trait `for<'a> FnOnce<(&'a bool,)>` is not implemented for `bool`
note: required by a bound in `Option::<T>::filter`
  --> $SRC_DIR/core/src/option.rs:LL:COL
help: you might have meant to create the closure instead of a block
   |
LL |     Some(true).filter(|_| {
   |                       +++
```

Partially address rust-lang#27300. Fix rust-lang#104690.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Oct 27, 2023
Rollup merge of rust-lang#117106 - estebank:issue-27300, r=petrochenkov

When expecting closure argument but finding block provide suggestion

Detect if there is a potential typo where the `{` meant to open the closure body was written before the body.

```
error[E0277]: expected a `FnOnce<({integer},)>` closure, found `Option<usize>`
  --> $DIR/ruby_style_closure_successful_parse.rs:3:31
   |
LL |       let p = Some(45).and_then({|x|
   |  ______________________--------_^
   | |                      |
   | |                      required by a bound introduced by this call
LL | |         1 + 1;
LL | |         Some(x * 2)
   | |         ----------- this tail expression is of type `Option<usize>`
LL | |     });
   | |_____^ expected an `FnOnce<({integer},)>` closure, found `Option<usize>`
   |
   = help: the trait `FnOnce<({integer},)>` is not implemented for `Option<usize>`
note: required by a bound in `Option::<T>::and_then`
  --> $SRC_DIR/core/src/option.rs:LL:COL
help: you might have meant to open the closure body instead of placing a closure within a block
   |
LL -     let p = Some(45).and_then({|x|
LL +     let p = Some(45).and_then(|x| {
   |
```

Detect the potential typo where the closure header is missing.

```
error[E0277]: expected a `FnOnce<(&bool,)>` closure, found `bool`
  --> $DIR/block_instead_of_closure_in_arg.rs:3:23
   |
LL |        Some(true).filter({
   |  _________________------_^
   | |                 |
   | |                 required by a bound introduced by this call
LL | |/         if number % 2 == 0 {
LL | ||             number == 0
LL | ||         } else {
LL | ||             number != 0
LL | ||         }
   | ||_________- this tail expression is of type `bool`
LL | |      });
   | |______^ expected an `FnOnce<(&bool,)>` closure, found `bool`
   |
   = help: the trait `for<'a> FnOnce<(&'a bool,)>` is not implemented for `bool`
note: required by a bound in `Option::<T>::filter`
  --> $SRC_DIR/core/src/option.rs:LL:COL
help: you might have meant to create the closure instead of a block
   |
LL |     Some(true).filter(|_| {
   |                       +++
```

Partially address rust-lang#27300. Fix rust-lang#104690.
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 C-enhancement Category: An issue proposing an enhancement or a PR with one. P-low Low priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants