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

Add a support for immovable generators #2397

Closed
topecongiro opened this issue Jan 26, 2018 · 6 comments · Fixed by #2400
Closed

Add a support for immovable generators #2397

topecongiro opened this issue Jan 26, 2018 · 6 comments · Fixed by #2400
Labels
bug Panic, non-idempotency, invalid code, etc. good first issue Issues up for grabs, also good candidates for new rustfmt contributors

Comments

@topecongiro
Copy link
Contributor

For example, when I try to format the following code,

#![feature(generators)]

unsafe fn foo() {
    let mut g = static || {
        yield 3;
    };
}

rustfmt 0.3.6-nightly emits the following error:

error: expected expression, found keyword `static`
 --> test.rs:4:17
  |
4 |     let mut g = static || {
  |                 ^^^^^^

The procedure to fix this would be something like this:

  1. add tests to make sure that rustfmt handles immovable generators
  2. update rustc-ap-syntax and rustc-ap-rustc_errors to the latest version
  3. update expr.rs and closures.rs to support immovable generators

cc rust-lang/rust#45337

@topecongiro topecongiro added bug Panic, non-idempotency, invalid code, etc. good first issue Issues up for grabs, also good candidates for new rustfmt contributors labels Jan 26, 2018
@topecongiro
Copy link
Contributor Author

BTW it feels great that rustfmt was not affected by the breaking change in libsyntax 🙌

@csmoe
Copy link
Member

csmoe commented Jan 28, 2018

I wanna take this, but is it suitable for me with basic Rust learned from The Rust Programming Language(2nd) and Rust By Example ?

@nrc
Copy link
Member

nrc commented Jan 28, 2018

@csmoe probably! Have a pole around the code base, and see if the steps in the OP make sense, the first two are testing, and Cargo only. The third requires writing some code. Let us know how you get on - if it feels too overwhelming, have a play with the code in general - try refactoring some small things to get an idea how the code works and if this bug is fixed, ping us and we'll find you another!

@csmoe
Copy link
Member

csmoe commented Jan 29, 2018

@nrc thank you,I’ll try my best on this.

@csmoe
Copy link
Member

csmoe commented Jan 29, 2018

I have updated rustc-ap-syntax and rustc-ap-rustc_errors to the latest version.
But the latest rustc-ap-syntax = "26.0.0" introduced a new struct inside ast.rs:

pub struct Label {
    pub ident: Ident,
    pub span: Span,
}

to replace the function provided by syntax::codemap::Spanned in rustc-ap-syntax = "12.0.0".
So it broke some piece of code:

error[E0609]: no field `node` on type `syntax::ast::Label`
   --> src/expr.rs:140:53
    |
140 |                 Some(ident) => format!(" {}", ident.node),
    |                                                     ^^^^ unknown field
    |
    = note: available fields are: `ident`, `span`

error[E0609]: no field `node` on type `syntax::ast::Label`
   --> src/expr.rs:147:53
    |
147 |                 Some(ident) => format!(" {}", ident.node),
    |                                                     ^^^^ unknown field
    |
    = note: available fields are: `ident`, `span`

error[E0277]: the trait bound `str: std::marker::Sized` is not satisfied
   --> src/expr.rs:297:20
    |
297 |         .and_then(|expr_str| {
    |                    ^^^^^^^^ `str` does not have a constant size known at compile-time
    |
    = help: the trait `std::marker::Sized` is not implemented for `str`
    = note: all local variables must have a statically known size

error[E0308]: mismatched types
   --> src/expr.rs:755:57
    |
755 |             Some(ControlFlow::new_for(pat, cond, block, label, expr.span))
    |                                                         ^^^^^ expected struct `syntax::codemap::Spanned`, found struct `syntax::ast::Label`
    |
    = note: expected type `std::option::Option<syntax::codemap::Spanned<syntax::ast::Ident>>`
               found type `std::option::Option<syntax::ast::Label>`

It can be fixed with some modifications, is it ok to enclose these modifications into commits for this issue(I'm uncertain because this bug is introduced by rust-ap-syntax but unrelated to immovable_generator)?

@topecongiro
Copy link
Contributor Author

@csmoe It is perfectly ok to do so. If you have any trouble, please feel free to use this branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Panic, non-idempotency, invalid code, etc. good first issue Issues up for grabs, also good candidates for new rustfmt contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants