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

Tracking issue for eRFC 2497, "if- and while-let-chains, take 2" #53667

Open
8 of 14 tasks
Tracked by #1568
Centril opened this issue Aug 24, 2018 · 66 comments · Fixed by #94927
Open
8 of 14 tasks
Tracked by #1568

Tracking issue for eRFC 2497, "if- and while-let-chains, take 2" #53667

Centril opened this issue Aug 24, 2018 · 66 comments · Fixed by #94927
Labels
B-RFC-approved Feature: Approved by a merged RFC but not yet implemented. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. F-let_chains `#![feature(let_chains)]` S-tracking-ready-to-stabilize Status: This is ready to stabilize; it may need a stabilization report and a PR T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@Centril
Copy link
Contributor

Centril commented Aug 24, 2018

This is a tracking issue for the eRFC "if- and while-let-chains, take 2" (rust-lang/rfcs#2497).
For the tracking issue for the immediate edition changes, see #53668.

Steps:

Unresolved questions:

Collected issues:

Implementation history:

Unresolved problems

  • Can we be confident that the implementation is correct and well tested?
@Centril Centril added B-RFC-approved Feature: Approved by a merged RFC but not yet implemented. T-lang Relevant to the language team, which will review and decide on the PR/issue. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. labels Aug 24, 2018
@Centril Centril self-assigned this Mar 11, 2019
@Centril
Copy link
Contributor Author

Centril commented Mar 11, 2019

I'm working on implementing this in a 3+ PR step fashion based on discussions with @oli-obk.

@oli-obk
Copy link
Contributor

oli-obk commented Mar 11, 2019

for the record, the discussed step list is

  1. remove if from HIR and make lowering emit a match
  2. remove if let from AST and instead have a let expression
  3. "do the rest of the work" (might get split up further).

@alexreg
Copy link
Contributor

alexreg commented Mar 11, 2019

I can't imagine 3 would be a particularly large step (enough to merit splitting), but 3 steps sounds fair enough.

bors added a commit that referenced this issue Apr 17, 2019
[WIP] [let_chains, 1/6] Remove hir::ExprKind::If

Per #53667 (comment).

r? @oli-obk
bors added a commit that referenced this issue May 10, 2019
[let_chains, 1/6] Remove hir::ExprKind::If

Per #53667 (comment).

r? @oli-obk
bors added a commit that referenced this issue May 15, 2019
[let_chains, 2/6] Introduce `Let(..)` in AST, remove IfLet + WhileLet and parse let chains

Here we remove `ast::ExprKind::{IfLet, WhileLet}` and introduce `ast::ExprKind::Let`.
Moreover, we also:
+ connect the parsing logic for let chains
+ introduce the feature gate
+ do some AST validation
+ rewire HIR lowering a bit.

However, this does not connect the new syntax to semantics in HIR.
That will be the subject of a subsequent PR.

Per #53667 (comment).
Next step after #59288.

cc @Manishearth re. Clippy.

r? @oli-obk
bors added a commit that referenced this issue May 16, 2019
[let_chains, 2/6] Introduce `Let(..)` in AST, remove IfLet + WhileLet and parse let chains

Here we remove `ast::ExprKind::{IfLet, WhileLet}` and introduce `ast::ExprKind::Let`.
Moreover, we also:
+ connect the parsing logic for let chains
+ introduce the feature gate
+ do some AST validation
+ rewire HIR lowering a bit.

However, this does not connect the new syntax to semantics in HIR.
That will be the subject of a subsequent PR.

Per #53667 (comment).
Next step after #59288.

cc @Manishearth re. Clippy.

r? @oli-obk
bors added a commit that referenced this issue May 19, 2019
[let_chains, 2/6] Introduce `Let(..)` in AST, remove IfLet + WhileLet and parse let chains

Here we remove `ast::ExprKind::{IfLet, WhileLet}` and introduce `ast::ExprKind::Let`.
Moreover, we also:
+ connect the parsing logic for let chains
+ introduce the feature gate
+ do some AST validation
+ rewire HIR lowering a bit.

However, this does not connect the new syntax to semantics in HIR.
That will be the subject of a subsequent PR.

Per #53667 (comment).
Next step after #59288.

cc @Manishearth re. Clippy.

r? @oli-obk
@estk
Copy link
Contributor

estk commented May 20, 2019

@Centril I'm really jazzed about this feature, lmk if there is any work I can help with.

@kristof-mattei
Copy link
Contributor

I am not sure it has been discussed before, and I know it's a syntax which shouldn't be used, but isn't that a bit confusing?

let condition = false;
let some_other_condition = false;

if let false = condition && some_other_condition {
    println!("step 1"); // Does not enter here
}

if let false = (condition && some_other_condition) {
    println!("step 2"); // Enters here
}

The first notation is ambiguous, and maybe not everyone expect the precedence to go that way. At least, there should be some kind of warning, like “You better write it that way: if !condition && some_other_condition { … } if that's what you meant”. That warning may even be shown whenever one pattern matches a bool, regardless of the context.

Correct me if I'm wrong but the way I read the let statement is as follows: does the pattern match? let false = false matches. Just like let None = None. So now we have true && some_other_condition, which is false so we don't enter the if statement.

For the second one we have (false && false), which is false which matches the let false = false predicate.

@mark-i-m
Copy link
Member

mark-i-m commented Jun 8, 2023

I think people could reasonably interpret it either way, but it probably auto to be a warning or at least a clippy lint.

@RealRTTV
Copy link

RealRTTV commented Jul 13, 2023

Given that without let chains, the only syntactically valid code snippet is the second one, that to not cause breaking changes the only way is to make the first example the correct one?

@PoignardAzur
Copy link
Contributor

Any progress on this feature?

@rami3l
Copy link
Member

rami3l commented Nov 9, 2023

Any progress on this feature?

@PoignardAzur AFAIK this feature is still blocked on #103476.

A simple ping in this case does nothing. If you'd like to see this feature stabilized, I recommend you to:

try out let chains in your own code, and report bugs you find.

Originally posted by @est31 in #103476 (comment)

@est31
Copy link
Member

est31 commented Nov 10, 2023

The main/only blocker at this point that I see is #104843, for which there is #107251, which is blocked on a nice overview comment as asked for in #107251 (comment), then an FCP.

At this point, I don't think that #103476 is a blocker any more, as enough time has passed. For #104893, I think due to the passage of time more people have tried out let chains, at least grep.app shows plenty of users. I don't really think it's much of a blocker either at this point any more.

@pnkfelix
Copy link
Member

re todo item "resolve expr fragment specifier issue", see also rust-lang/rfcs#3531, "Macro matcher fragment specifiers edition policy".

@photino
Copy link

photino commented Jan 1, 2024

This feature is really helpful. Is it possible to use in the Edition 2024?

@slanterns
Copy link
Contributor

slanterns commented Jan 1, 2024

It's not related to edition.

@inquisitivecrystal
Copy link
Contributor

This feature is really helpful. Is it possible to use in the Edition 2024?

Hi!

No, I'm afraid not. Editions have to do with changes that might break old code; as far as I know, this is completely unrelated. Also, people generally prefer that this sort of question not be asked on the tracking issue, as it sends a notification out to everyone subscribed to the thread without providing new information. It's no big deal, just something to be careful about going forward.

I believe the current status is described in est31's last post above.

gregorycoppola pushed a commit to gregorycoppola/bayes-star that referenced this issue Jan 10, 2024
error: expected expression, found `let` statement
  --> src/common/run.rs:65:5
   |
65 |     let train_result = do_training(&mut storage);
   |     ^^^

error: expected expression, found statement (`let`)
  --> src/common/run.rs:65:5
   |
65 |     let train_result = do_training(&mut storage);
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: variable declaration using `let` is a statement

error[E0425]: cannot find function `train_on_example` in this scope
  --> src/common/run.rs:30:15
   |
30 |         match train_on_example(storage, &proposition, &backlinks) {
   |               ^^^^^^^^^^^^^^^^ not found in this scope
   |
help: consider importing this function
   |
1  | use crate::model::maxent::train_on_example;
   |

error[E0658]: `let` expressions in this position are unstable
  --> src/common/run.rs:65:5
   |
65 |     let train_result = do_training(&mut storage);
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: see issue #53667 <rust-lang/rust#53667> for more information

warning: unused import: `super::choose::compute_backlinks`
 --> src/model/maxent.rs:8:5
  |
8 | use super::choose::compute_backlinks;
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: `#[warn(unused_imports)]` on by default

warning: unused import: `initialize_weights`
  --> src/model/maxent.rs:11:58
   |
11 | use super::weights::{negative_feature, positive_feature, initialize_weights};
   |                                                          ^^^^^^^^^^^^^^^^^^

error[E0308]: mismatched types
  --> src/common/run.rs:27:43
   |
27 |         let backlinks = compute_backlinks(storage, &proposition)?;
   |                         ----------------- ^^^^^^^ types differ in mutability
   |                         |
   |                         arguments to this function are incorrect
   |
   = note: expected mutable reference `&mut Storage`
                      found reference `&Storage`
note: function defined here
  --> src/model/choose.rs:74:8
   |
74 | pub fn compute_backlinks(
   |        ^^^^^^^^^^^^^^^^^
75 |     storage: &mut Storage,
   |     ---------------------

error[E0061]: this function takes 2 arguments but 1 argument was supplied
  --> src/common/run.rs:65:24
   |
65 |     let train_result = do_training(&mut storage);
   |                        ^^^^^^^^^^^ ------------ an argument of type `Box<(dyn LogicalModel + 'static)>` is missing
   |
note: function defined here
  --> src/common/run.rs:12:8
   |
12 | pub fn do_training(model:Box<dyn LogicalModel>, storage: &Storage) -> Result<(), Box<dyn Error>> {
   |        ^^^^^^^^^^^ ---------------------------  -----------------
help: provide the argument
   |
65 |     let train_result = do_training(/* Box<(dyn LogicalModel + 'static)> */, &mut storage);
@sammysheep
Copy link

Nightly user here. My experience with this feature has been very good. I'm surprised with how concisely I can convey my intent.

I find I tend to use chains with 3 clauses for some reason, and the succinct transitive nature of it feels elegant.

10/10. Would recommend!

@Nilstrieb
Copy link
Member

Nilstrieb commented Mar 9, 2024

@joshtriplett has opened rust-lang/rfcs#3573 and it is not clear whether it makes sense to have both features (I personally don't think so).
If it was decided that this should be stabilized (either because the RFC is rejected or because the lang team would like to have both), then it would need someone familiar with the let chains implementation to confirm that they are confident the implementation is correct now.

@est31
Copy link
Member

est31 commented Mar 10, 2024

Yeah before we stabilize this we should in any case wait for the decision on that RFC, which in this instance can turn in any direction.

@roylaurie
Copy link

Yeah before we stabilize this we should in any case wait for the decision on that RFC, which in this instance can turn in any direction.

I think this should keep moving forward. Why trash all of this work unless the other RFC actually gets accepted?

The other idea has already been shot down before. The recent sentiment I've seen on Reddit and in the comments on Github don't paint a picture of anything changing in that regard.

@jpmckinney
Copy link

I also say keep moving forward. In terms of governance, I don’t think it makes sense to pause work on one RFC anytime another RFC suggests an alternative. That’ll just stall everything. (Also, that other RFC will not pause for this one.)

jmqd added a commit to jmqd/simul that referenced this issue Mar 22, 2024
I expect more arms to be added here later, but for the time being, let's
address the clippy complaint and swap this to an if let. It would be
nice if we could chain these ifs, it's not stabilized yet:
rust-lang/rust#53667
@TennyZhuang
Copy link
Contributor

TennyZhuang commented Apr 1, 2024

I do not think rust-lang/rfcs#3573 should block this RFC. is operator is an alternative to the existing feature if let, but not let-chains. The motivation of the new is operator is better readability from a left-to-right style, which means that it should also replace existing if let Some(x) = y to if y is Some(x). Regardless of whether the is operator is a more reasonable expression, we cannot remove the existing if let style, so extending it is still meaningful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-RFC-approved Feature: Approved by a merged RFC but not yet implemented. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. F-let_chains `#![feature(let_chains)]` S-tracking-ready-to-stabilize Status: This is ready to stabilize; it may need a stabilization report and a PR T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.