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

Explain editions when trying to use async block in 2015 edition #67204

Open
estebank opened this issue Dec 10, 2019 · 13 comments
Open

Explain editions when trying to use async block in 2015 edition #67204

estebank opened this issue Dec 10, 2019 · 13 comments

Comments

@estebank
Copy link
Contributor

@estebank estebank commented Dec 10, 2019

Trying to use an async block in 2015 edition doesn't mention the concept of editions nor pushes the user in the right direction:

error: expected one of `,` or `}`, found `(`
  --> file12.rs:11:18
   |
10 |     let fut = async {
   |               ----- while parsing this struct
11 |         make_unit()?;
   |                  ^ expected one of `,` or `}`

error[E0422]: cannot find struct, variant or union type `async` in this scope
  --> file12.rs:10:15
   |
10 |     let fut = async {
   |               ^^^^^ not found in this scope
@Centril

This comment has been minimized.

Copy link
Member

@Centril Centril commented Dec 11, 2019

Can you provide the full input used to produce ^--?

@csmoe

This comment has been minimized.

Copy link
Member

@csmoe csmoe commented Dec 11, 2019

fn foo() {
    let x = async {};
}
error[E0422]: cannot find struct, variant or union type `async` in this scope
 --> src/main.rs:3:12
  |
3 |    let x = async {}; 
  |            ^^^^^ not found in this scope

error: aborting due to previous error

For more information about this error, try `rustc --explain E0422`.
error: could not compile `playground`.

To learn more, run the command again with --verbose.

playground

@Centril

This comment has been minimized.

Copy link
Member

@Centril Centril commented Dec 11, 2019

That doesn't include:

error: expected one of , or }, found (

What does the parser error come from?

@Centril Centril added the A-resolve label Dec 11, 2019
@estebank

This comment has been minimized.

Copy link
Contributor Author

@estebank estebank commented Dec 11, 2019

fn main() {}
fn bar() -> Result<(), ()> {
    panic!()
}

fn foo() -> Result<(), ()> {
   let x = async {
       bar()?;
   }; 
   Ok(())
}
@estebank

This comment has been minimized.

Copy link
Contributor Author

@estebank estebank commented Dec 11, 2019

For what is worth, I think the parser should use the snapshot approach here when in 2015 mode and try both parses, if one succeeds and the other one fails, give a targeted error.

@Centril

This comment has been minimized.

Copy link
Member

@Centril Centril commented Dec 11, 2019

I think we could just do this in resolve instead of the parser as it seems like the parser is able to recover and we reach resolve.

@estebank

This comment has been minimized.

Copy link
Contributor Author

@estebank estebank commented Dec 11, 2019

We can, but if we also handle it in the parser we can make the output be

error: `async` blocks are only available in 2018 edition
 --> src/main.rs:7:11
  |
7 |     let x = async {
  | ____________^
8 ||        bar()?;
9 ||    }; 
  ||____^
  |
  = note: edit your `Cargo.toml` file and add `edition = 2018` to the `[package]` section if you wish to use async/await in your crate
  = note: for more information, visit <APPROPRIATE BOOK SECTION>

error: aborting due the previous errors

The reason the parser recovers is because when it finds a parse error it consumes the rest of the block.

@Centril

This comment has been minimized.

Copy link
Member

@Centril Centril commented Dec 12, 2019

I'm skeptical of the additional complexity here in doing it in the parser (which I suspect would be non-trivial, in an already complicated part of the parser). This would also have a perf cost in the happy path due to let snapshot = self.clone(); but in resolve we already know there's an error so no additional cost is paid in the happy path. I would suggest doing the initial work in resolve and then consider whether additional logic in the parser is necessary. At any rate, I would appreciate not landing any such PR while my parser refactoring PRs are unmerged.

@estebank

This comment has been minimized.

Copy link
Contributor Author

@estebank estebank commented Dec 12, 2019

I would appreciate not landing any such PR while my parser refactoring PRs are unmerged.

Fair.

but in resolve we already know there's an error so no additional cost is paid in the happy path

True, the cost there is verbosity/vertical space, and that is fine, just not ideal.

I think you're overestimating the added complexity on isolation (or you're extrapolating to the addition of all these checks throughout the parser, in which case, fair) and of the size and prevalence of the performance hit. We can check whether the struct ident is async and do our recovery and branching parsing only in that case. I feel the edition difference, although brilliant from a backwards compatibility point of view requires us to do this kind of bending backwards to provide a good experience.

With all of that, this is not necessarily something we need to rush into because

  • This was more critical during early 2019 when 2015 code was more prevalent
  • The case has low likelihood to be hit by a newcomer today (but can still happen)
  • The resolve fix would be enough to guide a user, then this is just a verbosity problem

The first point is something we've gotten better at, mainly extrapolating what are some common errors that will be caused by a new feature and preemptively handling them. Async await for example had some great checks even before hitting beta. We should continue to strive for that level of quality in our output for all new features.

@tmandry

This comment has been minimized.

Copy link
Contributor

@tmandry tmandry commented Dec 17, 2019

Visiting from triage. @estebank would you be willing to mentor this?

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

@nikomatsakis nikomatsakis commented Dec 17, 2019

Can we do something really simple like, in 2015 mode, if we see an error at the point where there is an async keyword, not try to parse ahead "properly" but just report a message about async not being part of Rust 2015 right there? I guess one tricky part might be recovery -- ideally we'd skip the body of the function. But I worry that parsing "for real" might cause us to fail recovery anyway, if the user has more than one error in the code.

I see I was a bit confused

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

@nikomatsakis nikomatsakis commented Dec 17, 2019

If we do this in resolve, does this mean that the output would include both the parse error and then a "nice" resolve error? If so, I think that's not great. Ideally, we'd produce just a nice async error -- but at minimum we should give a nice async error first.

That said, I think we could just look for parse errors that occur when parsing some "struct named async" and trigger the nice error in that case, I'm not quite sure why we have to do anything too smart to read ahead and see if it "really is" an async block. I guess I think that the probability of 2015 code that uses a struct named async is low.

We might be able to thread the needle with some kind of "note" or something to also give the user better info about the parse error.

(However, I also think that parsing it as an async-block would be "ok", presuming that's something we do fairly often -- and maybe my proposal is sort of violating the "Hippocratic oath" of nice errors to "first do no harm". i.e., it could be pretty confusing to a user who does have a struct named async =)

@estebank estebank added the E-mentor label Dec 17, 2019
@estebank

This comment has been minimized.

Copy link
Contributor Author

@estebank estebank commented Dec 17, 2019

@nikomatsakis I do think that long term there would be value in consider removing this discrepancy between 2015 and 2019 and make async usable in 2015, which would require deprecate async as a non-keyword in 2015. That being said, I think that the added parser complexity to handle this as you mention should be small (but every little bit adds up).

@tmandry I'm down for it.

@Centril Centril self-assigned this Dec 18, 2019
@Centril Centril removed the E-mentor label Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.