Github Allow Overloading || and && by Nokel81 · Pull Request #2722 · rus...

Copy link

Contributor

    /// Decide whether the *logical or* should short-circuit
    /// or not based on its left-hand side argument. If so,
    /// return its final result, otherwise return the value
    /// that will get passed to `logical_or()` (normally this
    /// means returning self back, but you can change the value).
    fn short_circuit_or(self) -> ShortCircuit<Self::Output, Self>;

If we're going to have such an elaborate setup with an intermediate value, might as well allow the intermediate value to be a different type, so that short_circuit_or can provide arbitrary information to logical_or. Something like:

trait ShortCircuitOr<Rhs = Self>: Sized {
    type Intermediate: LogicalOr;
    fn short_circuit_or(self) -> ShortCircuit<Intermediate::Output, Intermediate>;
}
trait LogicalOr<Rhs = Self>: Sized {
    type Output;
    fn logical_or(self, rhs: Rhs) -> Self::Output;
}

RustyYato commented on Jul 11, 2019 •

@Nokel81 Please provide the full implementation of the traits for Option<T> and Result<T, E> in the "Reference-level explanation" section

Also, shouldn't we follow precedent of Option::and and Option::or to allow Option<T> && Option<U> and similarly for Result<T, E>

@comex We can simplify to

trait LogicalOr<Rhs = Self>: Sized {
    type Output;
    type Intermediate;
    
    fn short_circuit_or(self) -> ShortCircuit<Self::Output, Self::Intermediate>;
    fn logical_or(intermediate: Self::Intermediate, rhs: Rhs) -> Self::Output;
}

We don't need two traits, that seems like needless bloat. If you don't want short-circuiting behavior then you should overload & and | instead of && and ||

@KrishnaSannasi Good point, I like that better.

Nokel81 commented on Jul 11, 2019

Ah so, just have a short circuit trait? If that is the case, then why have the custom intermediate type?

Also, as mentioned in the rfc we discussed auto traits which is not how rust does things (Add is not required for AddAssign)

RustyYato commented on Jul 11, 2019 •

Ah so, just have a short circuit trait? If that is the case, then why have the custom intermediate type?

The intermediate type allows us to minimize coupling between the two functions, for example we could implement LogicalOr for bool like so

pub struct BoolShortCircuitFailure(());

impl LogicalOr for bool {
    type Output = bool;
    type Intermediate = BoolShortCircuitFailure;
    
    fn short_circuit_or(self) -> ShortCircuit<Self::Output, Self::Intermediate> {
        if self {
            ShortCircuit::Short(true)
        } else {
            ShortCircuit::Long(BoolShortCircuitFailure(()))
        }
    }
    
    fn logical_or(_: Self::Intermediate, rhs: Rhs) -> Self::Output {
        rhs
    }
}

Here since ShortCircuitBool is zero-sized and can only be made by bool, we can force short-circuit behavior and make the second step more efficient/easier to optimize by eliding checks.

For comparison, here is the implementation for bool with the current LogicalOr

impl LogicalOr for bool {
    type Output = bool;
    
    fn short_circuit_or(self) -> ShortCircuit<Self::Output, Self> {
        if self {
            ShortCircuit::Short(true)
        } else {
            ShortCircuit::Long(false)
        }
    }
    
    fn logical_or(self, rhs: Rhs) -> Self::Output {
        assert!(!self, "Failure to use `short_circuit_or` before calling `logical_or` is a bug");
        rhs
    }
}

The assert is technically unnecessary, but will catch bugs. But if we used an intermediate type, we can statically prevent these kinds of bugs and elide these checks.

Well, I was thinking that determining whether to short circuit might require performing some expensive calculation, which might involve generating intermediate values which could be reused in determining the final output value.

...One might argue that that use case is uncompelling because having || perform an expensive calculation is a code smell anyway.

But given that the design has an intermediate value, it's a bit strange to require it to be the same type as Self for no real reason. Especially with @KrishnaSannasi's version that doesn't require a whole other trait to allow the customization.

Centril commented on Jul 12, 2019 •

How is this RFC going to be compatible with rust-lang/rust#53667 given that you want to desugar && to a match and then dispatch to traits? Presumably this will interfere with the compiler's ability to understand if let Some(x) = foo() && bar(x) { as introducing bindings in x. In particular, the proposal here is to lower && to match which means that you cannot make semantic choices such as for let ps = e based on type information (e.g. bool) of the LHS and RHS. Moreover, using match + dispatch to trait methods seems like the sort of thing that would regress compile time performance non-trivially.

(I also think that allowing Some(2) || 1 is rather semantically strange.)

Centril commented on Jul 12, 2019

Another more meta point...

...How does this fit into the roadmap?

scottmcm commented on Jul 12, 2019

(I also think that allowing Some(2) || 1 is rather semantically strange.)

@Centril In the sense of "nobody would ever write that" or in the sense of "I don't think || 0 is a good way to provide a default value for an Option<i32>"?

As for #53667, doesn't it also understand the scoping of bindings by desugaring the control flow? Is there an indication that this control flow desugar would be different from that one? (Also, the compiler can special-case && for bool the same way it special-cases + for i32 and [] for arrays.)

@Centril I thought that

if let Some(x) = foo() && bar(x) {

would desugar to something like

if let Some(x) = foo() {
    if bar(x) {
    }
}

Because of the let ... binding's desugaring would take precedence over the normal && desugaring.

Moreover, using match + dispatch to trait methods seems like the sort of thing that would regress compile time performance non-trivially.

Like @scottmcm said, we could special case certain types (bool, maybe others) to improve compile times, so I don't see this as a big issue.

(I also think that allowing Some(2) || 1 is rather semantically strange.)

I find this just as strange as allowing true || false, so I'm curious as to why you think that it is strange.

Nokel81 commented on Jul 12, 2019

Also, shouldn't we follow precedent of Option::and and Option::or to allow Option<T> && Option<U> and similarly for Result<T, E>

Option<T>::or does not allow calling with Option<U>. However, since and does, then yes we should

Nokel81 commented on Jul 12, 2019

@KrishnaSannasi As for the having a "short circuit" trait and then relying on the & and | does not follow from the rest of the operator precidents. Add does not automatically imply AddAssign.

Nokel81 commented on Jul 12, 2019

How is thing RFC going to be compatible with rust-lang/rust#53667 given that you want to desugar && to a match and then dispatch to traits? Presumably this will interfere with the compiler's ability to understand if let Some(x) = foo() && bar(x) { as introducing bindings in x. In particular, the proposal here is to lower && to match which means that you cannot make semantic choices such as for let ps = e based on type information (e.g. bool) of the LHS and RHS. Moreover, using match + dispatch to trait methods seems like the sort of thing that would regress compile time performance non-trivially.

(I also think that allowing Some(2) || 1 is rather semantically strange.)

This being strange because it is an odd way to provide a default value?

kennytm commented on Jul 12, 2019

(I also think that allowing Some(2) || 1 is rather semantically strange.)

I find this just as strange as allowing true || false, so I'm curious as to why you think that it is strange.

The issue is that having both (Option<T> || Option<T>) → Option<T> and (Option<T> || T) → T is semantically strange. I don't see how true || false ((bool || bool) → bool) is relevant.

As for the having a "short circuit" trait and then relying on the & and | does not follow from the rest of the operator precidents. Add does not automatically imply AddAssign.

That's not what I meant, I meant that if someone wanted to not use short-circuit behavior they should use & and |, and not use && and ||.

Centril commented on Jul 12, 2019 •

@Centril In the sense of "nobody would ever write that" or in the sense of "I don't think || 0 is a good way to provide a default value for an Option<i32>"?

Neither of those reasons really.

I find this just as strange as allowing true || false, so I'm curious as to why you think that it is strange.

I think that a binary operator like this taking an expression of a different type is peculiar and surprising. I'm aware that + et. al allows Rhs to be differently typed but that's mostly to allow similar-ish types and not something entirely different like T as compared to Option<T>. @kennytm also echoes my sentiments here.

Also, the compiler can special-case && for bool the same way it special-cases + for i32 and [] for arrays.)

Like @scottmcm said, we could special case certain types (bool, maybe others) to improve compile times, so I don't see this as a big issue.

This special casing would need to happen after you have type information. Right now, it is simply assumed in fn check_binop that each sides of the operands are coercible to bool rather than using overloading. However, type checking match also happens in the same phase of the compiler. If you want to use match then you'll need to do a desugaring in fn lower_expr which is before the type information you need is available. To get around this you would need to avoid lowering to match and instead insert special logic into the type checker instead to handle it as the other overloaded operators are. This would presumably be substantially more complicated.

As for #53667, doesn't it also understand the scoping of bindings by desugaring the control flow? Is there an indication that this control flow desugar would be different from that one?

@Centril I thought that

if let Some(x) = foo() && bar(x) {

would desugar to something like

if let Some(x) = foo() {
    if bar(x) {
    }
}

Because of the let ... binding's desugaring would take precedence over the normal && desugaring.

Currently the compiler has an ast::ExprKind::Let to encode let ps = e syntactically. However, this is not really the issue.

Rather, the problem here is drop order. More specifically, an expression a && b && c associates as (a && b) && c but we drop temporaries here as b c a and not a b c. Moreover, if $cond { ... } and while $cond { ... } have the particular semantics that they make the $cond a terminating scope. Because of these things combined, if you take something like if a && b and lower it to match as above then you will alter the drop order of existing things to a b c instead (which is breaking) which I currently believe is necessary to have bindings work. If you instead desugar (a && b) && c as desugar(desugar(a && b) && c) I think you would instead not have bindings work. (Here is a few examples wrt. what happens with the drop orders, https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=1170f09002025c40d77cd017717e1424) My current theory is therefore that if + let + && are not actually implementable with match + DropTemps and that hir::ExprKind::{Let, If} is needed. (Sorry about the dense implementation oriented description, but I don't have the time right now to elaborate in a more high-level fashion.)

At minimum I think the implementation of let_chains needs to be finished before starting and accepting any work on this RFC.

Those are both comments about rust drawing heavily on functional languages.

I think foo() && { .. break; .. } && bar() would normally be quite poor style.

nox commented on Jul 31, 2019

Yes I feel that it can get wordy but it is also not possible to control the control flow of the enclosing function from within the closure.

That's a feature, I don't want to navigate code bases where I need to wonder what's the return type of a simple && or || expression, and I don't want to have to visually scan its right-hand side for control flow.

phaylon commented on Jul 31, 2019 •

I would be against adding the ability to overload logical operators.

Looking at std::ops as it looks currently, the traits in there can be categorized as:

• Math-operations: Add, AddAssign, Div, DivAssign, Mul, MulAssign, Neg, Rem,
  RemAssign, Sub, SubAssign
• Bit-operations: BitAnd, BitAndAssign, BitOr, BitOrAssign, BitXor, BitXorAssign
• Shift-operations: Shl, ShlAssign, Shr, ShrAssign
• Smart-pointers: Deref, DerefMut
• Destructors: Drop
• Closures: Fn, FnMut, FnOnce
• Indexed element access: Index, IndexMut
• Negation: Not
• Range abstraction: RangeBounds
• Related traits that aren't used directly: CoerceUnsized, DispatchFromDyn
• I have no idea what's std::ops about this: Generator
• Propagation conversion: Try

In general, this matches my preferences. Overloaded operations are there to bring
custom user types in line with standard or builtin types or to provide an abstract
interface for operations when there are no fundamentally inherent types to the operation.

I would argue that this is not true for an overloaded ||. It would be in line if instead there
was a Truth trait that allowed overloading truthiness itself, which I would also consider a
bad idea.

I believe that logical operations should (at least strive to) uphold certain properties. For
example:

if a {
  if b {
    c()
  }
}

and its relation to

if a && b {
  c()
}

which would be broken by this RFC. I have similar feelings towards the math operations and
consider overloading + for "putting things together" a mistake.

Since this change wouldn't allow overloading the truthiness of values, but only their usage
with certain logical operations, it would turn a set of purely logical operations into general
control flow operations that no longer relate to boolean logic. As was noted earlier in the thread,
I'd agree that things like some_condition || return are less clear than their full if based
counterparts and should be avoided. It seems to me that the change proposed in this RFC would
instead encourage coding like that. In this sense, .or_else(|| ...) avoiding control flow
alterations can be seen as a feature.

There is a std::ops::Not, but that doesn't really deal with boolean logic by itself. It just
happens to also work with bools. To be honest, I never used ! outside of booleans. I'd probably
consider using ! outside of a boolean context as misleading, and would .not() instead.

I'd also like to note that I consider (paraphrased) "The std::ops module is already big"
not an argument for making it bigger. This sentiment to me looks like embracing the slippery
slope full-on. It's becoming a more commonly seen argument these days and I wish we'd stay clear
of it. I'm also not sure it even works in this case, as std::ops contains a bit more than just overloadable operator traits.

In general it is my view that overloading operations always makes code less obvious and logic
less evident, except in the cases where we bring new types into a family of types. Like being
able to give big integer implementations the ability to do usual math operations.

I do want to say that I understand the general sentiment of wanting a feature like this. I just
consider making existing logical operations more "vague" to be an unfit solution. I believe
something like postfix macros would be a much better fit for this functionality.

Nokel81 commented on Jul 31, 2019

@phaylon That is a very well thought out argument. And yes, I do appreciate your understanding for wanting a feature like this. I do agree that postfix macros would fix this because then we could do a.or!(b).or!(c). But even then we would need some traits to make it generic enough to be useful (instead of a.opt_or!(b).opt_or!(c)).

nox commented on Jul 31, 2019

But even then we would need some traits to make it generic enough to be useful (instead of a.opt_or!(b).opt_or!(c)).

There is nothing wrong with making traits for combinators around Option<T> and Result<T, E>, just don't make those traits overload boolean operators.

Short circuit operators at the very least still should be added for at the very least custom types. Making a library to perform external operations, say BLAS or so, being forced to use something like .or(|| ...) is not at all remotely clean when the math and language of it dictates short-circuiting &&/|| and so forth. Whether people use them for other purposes should be left up to them and their own decision abilities.

nox commented on Aug 1, 2019

Short circuit operators at the very least still should be added for at the very least custom types. Making a library to perform external operations, say BLAS or so, being forced to use something like .or(|| ...) is not at all remotely clean when the math and language of it dictates short-circuiting &&/|| and so forth. Whether people use them for other purposes should be left up to them and their own decision abilities.

You can just be making your own local macros for whatever control flow structures you want to implement BLAS, that sounds like a bad argument to me, and the vast majority of people out there are not reimplementing BLAS anyway.

By that argument, I should have gotten a let … else expression years ago given how much I could use that in Servo.

By that argument, I should have gotten a let … else expression years ago given how much I could use that in Servo.

I'm guessing for matching failures? Wouldn't if let ... else ... be functionally the same, or do you mean for some other features.

In addition, this isn't for implementing new syntax, it's just for allowing existing syntax to work on custom types instead of being exceedingly specific enough to be near useless for all but the trivial cases.

After just reading the title and not the whole discussion on this RFC, I'd vote against that feature.

This results in even more cognitive overhead when using the language. That is exactly the overhead I tried to avoid when going for Rust instead of C++. The language has enough complexity already, this just allows the user to increase congnitive load when reading and writing Rust and does not help with readability and easy of use.

Edit: After reading the RFC more carefully, I can only say this even louder: I'm against this change in the language because it does increase the cognitive load and the "special-character/alphabet-character" ratio (which is undersirable). Having named functions for these tasks makes the language more expressive and more readable (as in 'I can read the code like english'). Changing this therefore will (IMO) result in less readability, more "having to keep things in mind"yiness and more confusion for beginners and maybe even intermediate Rust programmers.

Long story short: Please don't do this.

I'm still of the mind that this isn't a bad change, but should not be encouraged by the community, perhaps not even a default implementation on option/result, it should be kept for DSEL's.

Nokel81 commented on Aug 19, 2019

I would also like to add that the function based system is not a zero cost abstraction. As I have thought about it longer (and listening to the people who do not like this idea) I have realized that postfix macros would be another alternative to this problem. They would be a zero cost abstraction, there would be no need to have the two methods which would also reduce the cognitive load. A factor that many people have brought up. Postfix macros would need to solve the type problem (a macro on both Result<T, E> and Option<T> using the same name)

Example:

let x = cmd_line
    .args()
    .get("-o")
    .or!(config.get("output"))
    .unwrap_or!("terminal");

Might expand to something like the following:

let x = {
    if let Some(res) = cmd_line
        .args()
        .get("-o") {
        res
    } else {
        if let Some(res) = config.get("output") {
            res
        } else {
            "terminal"
        }
    }
}

As repeatedly stated above, f().and_then(|| ..) must be a zero cost because otherwise rust's zero cost abstraction story falls apart everywhere, like iter.map(|| ..).

scottmcm commented 12 days ago •

Coming back to this after #3058, some thoughts. (Without taking a position on whether we should or shouldn't do this.)

I see in this RFC the following:

enum ShortCircuit<S, L> {
    Short(S),
    Long(L),
}

trait LogicalOr {
    type Output;
    type Intermediate;
    fn short_circuit_or(self) -> ShortCircuit<Self::Output, Intermediate>;
}

And that leaps out to me as being nearly identical to the now-accepted

enum ControlFlow<B, C> {
    Break(B),
    Continue(C),
}

trait Try {
    type Output;
    type Residual;
    fn branch(self) -> ControlFlow<Self::Residual, Self::Output>;
}

(Yes, I've left some details out from both, to focus things.)

Is this parallel meaningful? Certainly many of the examples here are on ?-supporting types like Option and Result.

For instance, a.or_else(|_| b) on those two types is equivalent, using ?'s new underlying trait, to

match a.branch() {
    ControlFlow::Continue(c) => c,
    ControlFlow::Break(_) => b
}

H2CO3 commented 12 days ago

The parallel certainly exists; I'm however still not sure how one would desugar the following to this form:

fn foo(x: bool) -> u32 {
    let _: bool = x || return 1337;
    42
}

fn main() {
    println!("{}", foo(false));
    println!("{}", foo(true));
}

PoignardAzur commented 11 days ago •

Skimming through the debate, I think the arguments for and against this feature boil down to one trade off:

Keeping the language easy to learn vs Making a specific kind of code easier to read once you know the language.

(where "easy to learn" is relative, because, well, this is rust)

I think logical operator overloading has strong benefits for visibility, if people are already used to them. YMMV, but when you read this code:

foo.bar().or(baz());
foo.bar() || baz;

The first line has four identifiers that the brain needs to "parse"; it needs to register that "or" has a different meaning than the other identifiers, and represents "structure".

In the second line, the structure is easier to read at a skim. It's clear that "foo.bar()" and "baz()" are two terms of an operation, and people familiar with || immediately understand the sort-circuiting part.

On the other hand, adding logical operators means adding another small bit of trivia that someone has to learn before they can understand any code they read. For a language concerned with feature creep, that's no small thing.

I was initially for this feature, but re-framing things this way is making me change my mind. I don't think logical operators are orthogonal enough with other features to justify the added complexity.

Besides, I think most of the use cases for logical operators are covered by try blocks. Eg I think you could rewrite

let x = some_option && other_option && get_an_option();

let x = try {
  some_option?;
  other_option?;
  get_an_option()
}

scottmcm commented 11 days ago

On the other hand, adding logical operators means adding another small bit of trivia that someone has to learn before they can understand any code they read.

While that's certainly true in the abstract, it's not enough on its own, for me. How does it actually weigh out in practice for Rust code? Is it actually worse than learning "well, there's an or method, but you probably don't want .or(foo())"? Even || is another bit of trivia that people have to learn; it doesn't really need to exist either. a || b could just be if a { true } else { b } -- it's not horrible that way. (Or one could use the corresponding match, since if isn't needed either.)

Many languages have come to the conclusion that something here is valuable, like

And while those do work on null, the C# one at least works on Nullable<_> too. They all could have been functions in those languages too (AFAIK? C# definitely could; I'm not certain about the rest), but the operator was deemed worth it.

That said, they're not || and &&, so it's not a perfect parallel. But it's also apparently helpful enough that it was added even without the "people familiar with || immediately understand" advantage you mentioned.

scottmcm commented 11 days ago

Now, regardless of my previous post, I do think this point from @PoignardAzur is important:

Besides, I think most of the use cases for logical operators are covered by try blocks.

I agree that we don't yet know how a stable try {} would impact desires, design, or usefulness of these things. I agree that try { (a?, b?) } might be surprisingly nice, and conveniently avoids a bunch of the design questions here -- for example, try { a? & b? } would also work -- so I think we need to learn more about that before we decide anything here.

As such, let's postpone considering this for now:

@rfcbot fcp postpone

And, for clarity, I'm not saying that this exact RFC should necessarily come back. Maybe try will cover everything and nothing will be needed. Maybe something very similar to this (perhaps updated to ControlFlow instead of ShortCircuit) will end up being a good choice. Maybe it'll be a proposal for a different operator/macro/something.

(I don't know if that strictly means postpone or close, but whatever.)

rfcbot commented 11 days ago •

Team member @scottmcm has proposed to postpone this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

nikomatsakis commented 10 days ago

@rfcbot reviewed

rfcbot commented 4 days ago

This is now entering its final comment period, as per the review above.