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

Support if- and while-let chains #45

Merged
merged 1 commit into from
Jan 31, 2022

Conversation

ChayimFriedman2
Copy link
Contributor

RFC 2497 https://github.com/rust-lang/rfcs/blob/master/text/2497-if-let-chains.md.

I'm not sure if this should be a major or minor version bump: it breaks users of rust.ungram but not general users of the crate.

@Veykril
Copy link
Member

Veykril commented Jan 31, 2022

r? @matklad

Basically let chains require use to parse let expressions as the faeture will allow if (let pat = expr) to be a thing(at least as it stands right now):

Quoting rust-lang/rust-analyzer#11320 (comment):

Parsing turns out to be quite complex. The problem is that the let syntax can appear inside parentheses (if (let pat = expr)). This is to facilitate for macros, as stated in the RFC.

The problem is especially tricky since we cannot know if let is allowed in this position until we finish parsing it, becauses parentheses can be also tuples: if (let true = true, false) {} is invalid, but if (let true = true) {} is valid.

rustc chose to implement this as an expression kind and later verify that let is indeed valid there.

We don't have a validation pass to the AST. But it may be necessary to introduce it.

What do you think? Do you have an other way?

I believe this should work for us as well but I'd rather have your input on this grammar change.

@ChayimFriedman2
Copy link
Contributor Author

@Veykril Turns out we do have a validation pass: rust-lang/rust-analyzer@1eb9d36.

@matklad
Copy link
Member

matklad commented Jan 31, 2022

Ok, so, contrary to the RFC, it seems like let's are now treated as expressions syntactically:

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=11396e0b7b3bd40cfb6ca2c20f4ad786

#![feature(let_chains)]

macro_rules! assert_item {
    ($i:item) => ()
}

assert_item!(fn f() { let _ = (let 1 = 1); });

Than, yeah, I agree we should just make Condition an Expr (and rewrite a bunch of stuff in ra TT), and add a LetExpr as a kind of Expr.

As a follow up, I think it'll than make sense to get rid of LetStmt, and just use ExprStmt in its place.

@matklad
Copy link
Member

matklad commented Jan 31, 2022

bors r+

@bors
Copy link
Contributor

bors bot commented Jan 31, 2022

Build succeeded:

@bors bors bot merged commit 4b6c84d into rust-analyzer:master Jan 31, 2022
@ChayimFriedman2 ChayimFriedman2 deleted the if-while-let-chains branch February 1, 2022 09:27
@ChayimFriedman2
Copy link
Contributor Author

ChayimFriedman2 commented Feb 1, 2022

As a follow up, I think it'll than make sense to get rid of LetStmt, and just use ExprStmt in its place.

rustc does not do that: let expressions, AST vs. let statements, AST; let expressions, HIR vs. let statements, HIR. But maybe this is related to the fact that in the MIR they're represented very differently, so we don't have to worry. However, there also other incompatibilities (e.g. the type of let expressions is treated as bool while let statements have no type). I'm not sure we can't do that, but it is worth remembering.

@ChayimFriedman2
Copy link
Contributor Author

Also:

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=73ac9fb223d5785f932cd3760036c29d

#![feature(let_chains)]

macro_rules! expr {
    ($e:expr) => ()
}
macro_rules! stmt {
    ($e:stmt) => ()
}


// expr!(let 1 = 1);
expr!((let 1 = 1));
stmt!(let 1 = 1);
stmt!((let 1 = 1));

The stmt!((let 1 = 1)) probably works because this is an ExpressionStatement but the stmt!(let 1 = 1) is probably a LetStatement, confirmed by the fact that expr!(let 1 = 1) does not compile.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants