Copy link

lebensterben commented 8 days ago •

I'm against the now-merged let-else RFC. But this one is even worse in terms of readability.

For example:

// let else else future possiblity
let Some(x) = a else b else c else { return; };

// with let expression
(let Some(x) = a) || (let Some(x) = b) || (let Foo(x) = bar) || { return; };

In a general case, if it's not Option::Some, but Foo instead:

The let-else syntax requires user to figure out Foo in let Foo(x) = a else .. is a Sum type and thus the binding is fallible. (Otherwise else doesn't make any sense.)

But with the new syntax, (let Foo(x) = a) || (let Bar(x) = b) || (let Baz(x) = c) || .., you're gonna process each and every identity on the left hand side of = and they must all be Sum types, with exception of the tail of the chain.

For example, this allows the following readability nightmare:

pub mod XXX {
    pub enum YYY { Foo(i32), Bar(u32) }
	pub struct Baz (i8);
}

use XXX::YYY::*;
use XXX::Baz;

fn nightmare () {
    let (a, b, c) = (Bar(0), Foo(0), Baz(0));

    // Here we go
    (let Foo(x) = a) || (let Bar(x) = b) || (let Baz(x) = c) || { return; };
}

The reader gotta process the entire expression and internalize that the first two bindings may fail, but then, wait, Baz is a Product type and the binding never fail. So there's no need for the { return; } at the end.

This is nowhere being any better for new learners. It's confusing and error-prone.

HKalbasi commented 8 days ago

@lebensterben I don't see your concern. The user can write this today:

if let Baz(x) = c { } else { }

and just get a warning for unreachable code, which is good in my opinion. Irrefutable patterns in let expressions are like x==x and refutable ones are like x==2 and you can use both of them in || and if and similar. I don't see any catastrophic thing in your example and it isn't semantically wrong or meaningless. It just has unreachable code in || { return; } which probably will be removed by a compiler warning, before being read.

text/0000-let-expression.md

Show resolved

text/0000-let-expression.md

Outdated

Show resolved

runiq commented 6 days ago

Isn't the || operator lazily evaluated? Can the rest of my code rely on a let binding behind a || operator?

HKalbasi commented 5 days ago

It is short circuited but since both side of || must have equal bindings (both in name and type), we can be sure that at least one of them is bound and it doesn't matter which one is. So rest of code can rely on them.

ids1024 commented 5 days ago •

Regarding "let as a bool expression", it's worth noting that this isn't necessarily the only or most obvious thing a let expression might evaluate to.

Expressions like x = 5 (without let) evaluate to ().

In Javascript x = 5 evaluates to 5, and similarly with x := 5 in Python. Though this behavior wouldn't be possible in Rust for non-Copy types.

Edit: Somehow I forgot Javascript also has a let keyword. Doesn't seem let is an expression there. In Python however there's not a specific keyword for defining new variables.

HKalbasi commented 5 days ago

@ids1024 your point is valid. I added it to drawbacks.

joew60 commented 5 days ago

If I submitted something like

(let Some(x) = a) || (let Some(x) = b) || (let Foo(x) = bar) || { return; };

to a code review; it would be rejected as unreadable. Phrases like "keep it simple" would be used. Even if I understand it today, am I going to understand it in six months?

The fact we can introduce this feature is not a justification for including it.

HKalbasi commented 5 days ago

@joew60 What is unclear in your example that you may not understand it in six months?

The fact we can introduce this feature is not a justification for including it.

Certainly. The justification for this RFC is that it unites features like if-let, if-let-chain and let-else, which are known to be useful, in a general and consistent manner, and then get some extra features for free (with no adding to grammar and concepts of the language). At the end there will be some complex usages (which I don't believe your example is among them) that code reviewers can reject.

If I submitted something like

(let Some(x) = a) || (let Some(x) = b) || (let Foo(x) = bar) || { return; };

to a code review; it would be rejected as unreadable. Phrases like "keep it simple" would be used. Even if I understand it today, am I going to understand it in six months?

That's how I feel about most of the examples in the RFC. I think this RFC is too focused on what is theoretically possible instead of what is useful, readable, practical, and realistic. Cramming more logic into a single statement is not an improvement. Code that looks like minified JavaScript is bad code.

I don't feel great about this RFC, but I might consider it more seriously if it had a more practical lens.

HKalbasi commented 3 days ago

@camsteffen I added some practical examples in this section and I probably add some more when I find.

Cramming more logic into a single statement is not an improvement. Code that looks like minified JavaScript is bad code.

This is minified JS: (picked from this page assets)

!function(){"use strict";function e(e){const t=[];for(const o of function(){try{return document.cookie.split(";")}catch(e){return[]}}()){const[n,r]=o.trim().split("=");e=

Is that example really looks like minified JS? What are their similarities? Just because it is in one line, it looks like minified JS? You can put it in multiple lines:

(let Some(x) = a)
|| (let Some(x) = b)
|| (let Foo(x) = bar)
|| { return; };

And the point of this is not count of lines. You can put current alternative in one line as well:

let x = if let Some(x) = a { x } else if let Some(x) = b { x } else if let Foo(x) = bar { x } else { return; };

Or in multiple lines:

let x = if let Some(x) = a {
    x
} else if let Some(x) = b {
    x
} else if let Foo(x) = bar {
    x
} else {
    return;
};

Either way, current solution has unnecessary noise, two times more x, and let expression solution has more cognitive load because it is a new syntax, but it is clear that what it wants to do.

Maybe you can explain your feelings and what you mean from minified JS by some examples.

Maybe you can explain your feelings and what you mean from minified JS by some examples.

JS is a very flexible language and allows you to write very dense code. Perhaps this is a better fit for the goals of JS. But I don't want to dwell on that comparison. The point is that the code is hard to read. That's just my own subjective feeling from looking at the examples.

HKalbasi commented 3 days ago

@camsteffen minified JS is a machine generated and unreadable by design code, Which doesn't related to that examples I think. JS is flexible but you can minify rust as well. Anyway, it is not our discussion, lets move on.

The point is that the code is hard to read. That's just my own subjective feeling from looking at the examples.

Can you please explain your feeling in more details? These questions may help:

• Is || inside if scrutinee hard to read? And if so, what is your feeling about existing if-let-chain proposal?
• Are ||, && in top level of a block hard to read? If so, does reading them as orelse, andalso like in standard ML make it easier? Problem is chaining them or single || is bad as well? And do you find else in let-else syntax easier to read than || in equivalent statement?
• Do () around let makes them hard to read? And changing precedence of operators help?
• Are consumed let expressions in places like assert! hard to read and if <expr> { true } else { false } is better?
• Are your feeling for real code examples, equal to you feeling about other ones? Real code examples have more context in them and their variables are not single letter, so maybe they are easier to read.

In each case, if it is applicable, please provide which is wrong, for example bindings are unclear, side effects are unclear, ... It isn't necessary and you can simply don't like anything you want.

IceSentry commented 3 days ago •

Using a top level || followed by a block is not necessarily hard to read, but it's definitely surprising because I've personally never seen that in any other language.

To me this:

(let Some(x) = a)
|| (let Some(x) = b)
|| (let Foo(x) = bar)
|| { return; };

is way harder/surprising to read than

if let Some(x) = a || let Some(x) = b || let Foo(x) = bar {
	return; 
}

Using a top level || followed by a block is not necessarily hard to read, but it's definitely surprising because I've personally never seen that in any other language.

|| followed by some command or block occurs quite often in bash scripts (and Perl too iirc).

How about adding and_then and or_else method to bool? Then we can write:

(let Some(x) = a)
  .or_else(let Some(x) = b)
  .or_else(let Foo(x) = bar)
  .or_else({ return; });

Which in my opinion is much more clear to read and ergonomic to write with autocomplete.

The only problem I can see is that this may potentially muck with operator precedence and some compiler optimizations.

@changhe3 What's the scope for x then?

@iago-lito The calling scope of course. I don't think function parameters are in their own scope, unless you explicitly use {}.

HKalbasi commented yesterday •

@changhe3 In this proposal, they are. And for good reason, If you allow function parameters to bind variables in parent block, then:

assert!(let Some(x) = foo);
println!("{}", x);

What would be the result of this program?

Do () around let makes them hard to read?

That is maybe the main problem for me. Each set of parenthesis causes me to pause and say "watch out, the operator precedence is being overriden here..." Lots of parenthesis is also a signal to me that factoring in a variable may be helpful for readability. But that isn't an option here. Rust currently enables you to be very conservative with parenthesis and I like that.

And changing precedence of operators help?

I don't think that is really an option? I have an expecation for the operator precedence of pat = x || y.

Another issue is that let is allowed anywhere in an expression. I like that in current Rust, bindings are generally introduced in predictable places - at the beginning of a statement, at the start of a match arm, or in function args. When reading code, the binding names "tell the story". So the readability of a function is highly affected by how quickly I can identify all the binding names defined. With this change, binding declarations are much less prominent.

HKalbasi commented 12 hours ago

@camsteffen

I don't think that is really an option? I have an expectation for the operator precedence of pat = x || y.

Your expectation isn't met always, and we changed it once in if-let-chain RFC.

I have a very vague and hacky idea to remove need to parenthesis without breaking change: let pat = x || y only is valid if pat and x and y types are bool. If it is possible to type check x and if it wasn't bool, rotate the parse tree to (let pat = x) || y, the need to parenthesis would be solved. If it isn't possible to change parse tree after type check, we can check if pat is a bool pattern without type checking, it should have format of ident, true, ident @ false, and | combination of this things, so it should be easy to detect it by parse level information. If we find that pat surely is not a bool pattern, we can rotate the parse tree. Negative point of the latter is that false positives like ident is possible and it still mandate parenthesis for let ident = x && y which x can be not bool, but it is better than mandating parenthesis everywhere. (Note that (let ident = x) || y is non sense and y is unreachable here.) With both solutions, there is no breaking change.

I don't know if this level of context sensitivity is feasible, and if something like this is done before, but if it is ok, (or ok with changes) let me know to put this in RFC. If it isn't, I should add parenthesis to drawbacks.

at the beginning of a statement, at the start of a match arm, or in function args

And macros. If the problem here is possibility of creating a mess, macros can do that very better, but if concern is that people accidentally make an unclear binding statement that make problem for code readers, I claim that 98% of meaningful binding statements have let at the beginning (or ((let) and it can be forced by lint, so you can keep looking at beginning of the statements for discovering bindings.

@fee1-dead

Do new learners actually expect let to be a bool expression?

I personally would expect that.

@camsteffen

I don't feel great about this RFC, but I might consider it more seriously if it had a more practical lens.

I don't understand the intricacies here, but as a +1: I would definitely find it nice to be able to write:

if let Some(x) = self.some_func() && let Some(y) = self.other_func() {}

instead of having now to do

if let Some(x) = self.some_func() {
    if let Some(y) = self.other_func() {}
}

It would reduce the number of braces, which is objectively better.

I realize this is possible sometimes, but it's not as readable

if let (Some(x), Some(y)) = (self.some_func(), self.other_func()) {}

It's not possible when the output of some_func needs to be piped into other_func. The workaround being:

if let Some((x, Some(y)) = self.some_func().map(|x| (x, self.other_func(x))) {} 

Which is not as readable as well.

Not a fan of:

... || {
    return Err("failed");
};

I feel like that should be a default clippy warning. It's bad style, but imo shouldn't block this feature.

fee1-dead commented 9 hours ago

as a +1: I would definitely find it nice to be able to write

That is not something coming from this RFC, see if-let-chains which is already an accepted RFC.