Skip to content

Allow Overloading || and && #2722

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Allow Overloading || and && #2722

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2dea918
initial doc
Nokel81 Jul 11, 2019
3d5746d
Update text/0000-overload-logic-operators.md
Nokel81 Jul 12, 2019
ec4d7ed
Added some more suggestions
Nokel81 Jul 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
228 changes: 228 additions & 0 deletions text/0000-overload-logic-operators.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,228 @@
- Feature Name: overload-logic-operators
- Start Date: 2019-07-04
- RFC PR: [rust-lang/rfcs#0000](https://github.com/rust-lang/rfcs/pull/0000)
- Rust Issue: [rust-lang/rust#0000](https://github.com/rust-lang/rust/issues/0000)

# Summary
[summary]: #summary

This feature would allow for the two short circuit operators `||` and `&&` to be overloadable by
users-of-rust and for the standard library (probably in core/alloc) to implement such an overload for
the `Option<T>` and `Result<T, E>` types for `||` and for the `Option<T>` type `&&` but not for
`Result<T, E>`.

# Motivation
[motivation]: #motivation

This idea was original floated as a way to clear up the differences between `.or(...)`, `.or_with(|| ...)`, `.and(...)`, `.and_with(|| ...)`, `.unwrap_or(...)`, and `.unwrap_or_with(|| ...)`. Not only was the requirement to remember that there were additional methods that are supposed to be used when you don't want to compute the value before the check (short circuiting). There was also a concern about the overhead of the additional closure.

This proposal is mostly about reducing the mental strain when chaining `Option<T>`'s and `Result<T, E>`'s. But has a very nice side effect of allowing users-of-rust the ability to overload these operators.

# Guide-level explanation
[guide-level-explanation]: #guide-level-explanation

This proposal starts with an enum definition and trait definitions for each of the operators:
Copy link
Member

@scottmcm scottmcm Jul 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is the appropriate start for a guide-level explanation. I think this section should look substantially more like the ? description in the book: describe a common need, describe how it can be done manually with if let, then describe how it's handled using the ||/&& operators to be more concise. I think this section should also emphasize the parallel with booleans -- how foo() && bar() is if foo() { true } else { bar() } and how that's the same pattern in the if lets seen here.

Some examples of the parallel are in IRLO. Maybe use an example about how you can just say i < v.len() && v[i] > 0 instead of if i < v.len() { false } else { v[i] > 0 }.

(It would probably not mention the trait definitions at all.)


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

trait LogicalOr<Rhs = Self>: Sized {
type Output;
type Intermediate;

/// 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, Intermediate>;

/// Complete the *logical or* in case it did not short-circuit.
/// Normally this would just return `rhs`.
fn logical_or(lhs: Intermediate, rhs: Rhs) -> Self::Output;
}

trait LogicalAnd<Rhs = Self>: Sized {
type Output;
type Intermediate;

/// Decide whether the *logical and* 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_and()` (normally this
/// means returning self back, but you can change the value).
fn short_circuit_and(self) -> ShortCircuit<Self::Output, Intermediate>;

/// Complete the *logical and* in case it did not short-circuit.
fn logical_and(lhs: Intermediate, rhs: Rhs) -> Self::Output;
}
```

With a matching desugaring:

```rust
<expr_a::<T>> || <expr_b::<T>>

==>

match expr_a.short_circuit_or() {
ShortCircuit::Short(res) => res,
ShortCircuit::Long(lhs) => logical_or(lhs, expr_b)
}
```

and

```rust
<expr_a::<T>> && <expr_b::<T>>

==>

match expr_a.short_circuit_and() {
ShortCircuit::Short(res) => res,
ShortCircuit::Long(lhs) => logical_and(lhs, expr_b)
}
```

From taking into consideration the current functions, and previous discussion on [internals](https://internals.rust-lang.org/t/pre-rfc-overload-short-curcuits/10460) it seems that the following makes the most sense in terms of outcomes.

#### For `Option<T>`:

```rust
fn foo() -> Option<i32> {
Some(3)
}

fn main() {
Some(4) || Some(5); // == Some(4)
None || Some(5); // == Some(5)
Some(4) || foo(); // == Some(4) (foo is *not* called)
None || foo(); // == Some(3) (foo is called)
None || 3; // == 3
Some(2) || 1; // == 2
Some(1) || panic!() // == Some(1) + These two are side effects from !
None || return // returns from function + and are the same to how boolean || works
Some(2) && Some(3) // Some(3)
None && Some(1) // None
Some(3) && None // None

Some(2) || Some("hello") // Error: LogicalOr<Option<&str>> not implemented for Option<i32>
Some(2) || 2 || 3 // Error: LogicalOr<i32> is not implemented for i32
Comment on lines +100 to +113
Copy link
Contributor

@pickfire pickfire Jul 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Some(4) || Some(5); // == Some(4)
None || Some(5); // == Some(5)
Some(4) || foo(); // == Some(4) (foo is *not* called)
None || foo(); // == Some(3) (foo is called)
None || 3; // == 3
Some(2) || 1; // == 2
Some(1) || panic!() // == Some(1) + These two are side effects from !
None || return // returns from function + and are the same to how boolean || works
Some(2) && Some(3) // Some(3)
None && Some(1) // None
Some(3) && None // None
Some(2) || Some("hello") // Error: LogicalOr<Option<&str>> not implemented for Option<i32>
Some(2) || 2 || 3 // Error: LogicalOr<i32> is not implemented for i32
// or
Some(4) || Some(5); // == Some(4)
None || Some(5); // == Some(5)
// or_else
Some(4) || foo(); // == Some(4) (foo is *not* called)
None || foo(); // == Some(3) (foo is called)
// unwrap_or
None || 3; // == 3
Some(2) || 1; // == 2
// weird
Some(1) || panic!() // == Some(1) + These two are side effects from !
None || return // returns from function + and are the same to how boolean || works
// and
Some(2) && Some(3) // Some(3)
None && Some(1) // None
Some(3) && None // None
// and_then (without taking parameter)
Some(4) && foo(); // None (foo is called)
None && foo(); // None (foo is **not** called)
Some(2) || Some("hello") // Error: LogicalOr<Option<&str>> not implemented for Option<i32>
Some(2) || 2 || 3 // Error: LogicalOr<i32> is not implemented for i32 
    Some(1) || panic!() // == Some(1)               + These two are side effects from !
    None || return      // returns from function    + and are the same to how boolean || works

Looks like magic, what are these? How about Some(3) || return?

How about Option to Result type? And vice versa?

Some(4) && Ok(5)
None && Ok(5)
Some(4) && Err("no")
None && Err("no")
Some(4) || Ok(5)
None || Ok(5)
Some(4) || Err("no")
None || Err("no")

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some(3) || return

Some(3) || return == Some(3) similar to how Some(1) || panic!() == Some(1)

How about Option to Result type? And vice versa?

There are already functions to do that conversion, so it doesn't seem necessary.

}
```

#### For `Result<T, E>`
```rust
struct MyError;

fn foo() -> Result<i32, MyError> {
Ok(3)
}

fn main() {
Ok(4) || Ok(5); // == Ok(4)
Err(MyError) || Ok(5); // == Ok(5)
Ok(4) || foo(); // == Ok(4) (foo is *not* called)
Err(MyError) || foo(); // == Ok(3) (foo is called)
Err(MyError) || 3; // == 3
Ok(2) || 1; // == 2
Comment on lines +126 to +131
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Ok(4) || Ok(5); // == Ok(4)
Err(MyError) || Ok(5); // == Ok(5)
Ok(4) || foo(); // == Ok(4) (foo is *not* called)
Err(MyError) || foo(); // == Ok(3) (foo is called)
Err(MyError) || 3; // == 3
Ok(2) || 1; // == 2
// or
Ok(4) || Ok(5); // == Ok(4)
Err(MyError) || Ok(5); // == Ok(5)
// or_else (without taking parameter)
Ok(4) || foo(); // == Ok(4) (foo is *not* called)
Err(MyError) || foo(); // == Ok(3) (foo is called)
// unwrap_or
Err(MyError) || 3; // == 3
Ok(2) || 1; // == 2
// unwrap_or_else (without taking parameter)
Ok(4) || bar(); // == 4 (foo is *not* called)
Err(MyError) || bar(); // == 3 (foo is called)

bar() returns i32, but I think this is kinda hard to distinguish if xxx in || xxx() is returning what in the first place. It do two tasks which are either unwrap_ or or_. Looks like a potential place for footguns.


Ok(2) || Ok("hello"); // Error: LogicalOr<Result<&str, _>> not implemented for Result<i32, _>
Ok(2) || 2 || 3; // Error: LogicalOr<i32> is not implemented for i32
}
```

The feature should be thought about as moving the logic from methods and into the current function.
It maps very seamlessly from using the methods and is equivalent in use without having to worry about
the naming convention of the short circuit methods. The same mental state of short circuit from
bools applies directly, without having any recourse to "truthiness" which is not a desirable trait.

This RFC also proposes to deprecate the `.or(...)`, `.or_with(|| ...)`, `.and(...)`, `.and_with(||
...)`, `.unwrap_or(...)`, and `.unwrap_or_with(|| ...)` methods on `Option<T>` and `.or(...)`,
`.or_with(|| ...)`, `.unwrap_or(...)`, and `.unwrap_or_with(|| ...)` methods on `Result<T, E>` since
using this feature renders them unneeded.
Comment on lines +143 to +146
Copy link
Contributor

@pickfire pickfire Jul 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it may be a good idea to allow binary operator overloading but I don't think it is a nice idea to mix both or_ and unwrap_ functions into the same operator, it may be hard to distinguish if it is behaving like which function since it does it implicitly, it also requires the developer to know the context on the output of the function in order to understand how is && and || behaving.

I think keeping && and || for only or and and without those unwrap_ might be a good idea for Option and Result. It reduces the load on the user mind to get to understand whether it behaves like or_ or unwrap_, you never know, you need to always double check, changing one function signature (Option<T> to T) might break all other parts of the code using && and || (chain of destruction).

Prior art (bad art):

  • python **kwargs implicitness, results in bad docs and mental load, similar to this in the sense that it does multiple stuff
class HOTP(OTP):
    """
    Handler for HMAC-based OTP counters.
    """
    def __init__(self, *args: Any, initial_count: int = 0, **kwargs: Any) -> None:
        """
        :param initial_count: starting HMAC counter value, defaults to 0
        """
        self.initial_count = initial_count
        super(HOTP, self).__init__(*args, **kwargs)

Guess what could you put for kwargs without clicking on the item below.

solution (you need to read the source code to find this) 
class OTP(object):
    """
    Base class for OTP handlers.
    """
    def __init__(
        self,
        s: str,
        digits: int = 6,
        digest: Any = hashlib.sha1,
        name: Optional[str] = None,
        issuer: Optional[str] = None
    ) -> None:
        """
        :param s: secret in base32 format
        :param digits: number of integers in the OTP. Some apps expect this to be 6 digits, others support more.
        :param digest: digest function to use in the HMAC (expected to be sha1)
        :param name: account name
        :param issuer: issuer
        """
        self.digits = digits
        self.digest = digest
        self.secret = s
        self.name = name or 'Secret'
        self.issuer = issuer


# Reference-level explanation
[reference-level-explanation]: #reference-level-explanation

The basis of this proposal are the two new traits. These should be implemented along the same lines
as other operator traits. However, the desugaring should follow the `match` output in the previous
section so as to obtain the desired short circuit operation.

Once the traits have been implemented, several trait implementations should be added to the std library and the methods marked as deprecated.

```rust
impl LogicalOr<Option<T>> for Option<T> {
type Output = Self;
...
}

impl LogicalOr<T> for Option<T> {
type Output = T;
}

impl LogicalAnd<Option<T>> for Option<T> {
type Output = Self;
...
}

impl LogicalOr<Result<T, E>> for Result<T, E> {
type Output = Self;
...
}

impl LogicalOr<T> for Result<T, E> {
type Output = T;
}

```

# Drawbacks
[drawbacks]: #drawbacks

1. Leaves the `||` and the `&&` as not strictly boolean operators, which might hurt readability
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
1. Leaves the `||` and the `&&` as not strictly boolean operators, which might hurt readability
1. Leaves the `||` and the `&&` as not strictly boolean operators, which might hurt readability.

2. Could lead to similarities to C++'s `operator bool()` which implies truthiness and is undesirable.

# Rationale and alternatives
[rationale-and-alternatives]: #rationale-and-alternatives
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two more things I'd like to see discussed in here

  • Why the method to allow combining the sides, vs just something like -> Option<Rhs>?

  • Why two traits, vs using one method that splits into the two parts, one kept on && and the other kept on ||

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the two traits question. Is it really necessary to discuss why each operator should have its own trait?


- This proposal has the two methods so that each of the questions: "should this short circuit?" and "how to combine when not short circuiting?" can be answered seperately. It could be possible to just return an `Option<Rhs>` for the `short_circuit` functions and then the desurgaring would not allow for any computation similar to `a | b` (using `BitOr`) when it does not short circuit.
- This design is the best because it does not rely on other traits, which are how the other operator traits work in Rust. It has the minimum overhead since it does not rely on closures.
- Some alternatives were discussed and shown to be inferior in either performance, explicitness, or design.
- It could be possible to just have the `short_circuit(&self)` method return `enum ShortCircuit { Short, Long }` and have the desugaring be:
```
expr_a || expr_b

===>

match expr_a.short_circuit() {
ShortCircuit::Short => expr_a,
ShortCircuit::Ling => expr_a.logical_or(expr_b)
}
```
This is a simpler option which does have the benefits of not being complicated by intermediate types, not having `bool` conversion functions, and a simpler (arguably more understandable) desugaring since the lhs cannot be modified during the `short_circuit` check.
- The first being "truthiness" conversion trait and then automatically allowing `||` and `&&` if both that trait and the `BitOr` or `BitAnd` traits were also implemented. This was discarded because we did not want to go down the javascript route of auto converting things to boolean (Rust already does not allow non-bools in check expressions) and auto traits are not something that Rust has so that was another reason not to go down this route.
- The second being a trait that accepts an `FnOnce` argument and then the second argument of the operator would be then implicitly hoisted into a closure. This was rejected both because hiding closures is not a zero-cost abstraction, it would break the similarity with the boolean operators because `None || return` would not return from the function unlike `false || return`. This also does not have any benifit over just using `or_with` directly except for a few characters.
- If this is not done it then the usability of Rust without having to go to the docs would stay the same.
- It was suggested that each of `LogicalOr` and `LogicalAnd` should have a type parameter `type Intermediate;` which is the value assigned to `ShortCircuit::Long`. This was to either fasilitate adding some more information coupling or statically declaring that no information should be sent between the `short_circuit` and `logical_***`. It was for this second reason that the `Intermediate` type was added. There are code smell reasons for sending extra data between those to functions but the gains of usability from being able to send named zero-sized type seems to outweight them.


# Prior art
[prior-art]: #prior-art

The only other language that seems to have implemented short circuit `||` and `&&` is C#.

C# did it with the combined auto trait and truthiness (bool() operator) functions. While this is similar to this proposal, it is thought that since auto converting to bool is not happening (just checking if it should short circuit) thinking about the functions as truthiness. C# already doesn't use lambdas (similar to closures) for its solution.

C++ also allows overloading these operators but without short circuiting which is undesirable.

# Unresolved questions
[unresolved-questions]: #unresolved-questions

No unresolved questions.

# Future possibilities
[future-possibilities]: #future-possibilities