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

Allow Overloading || and && #2722

Closed
wants to merge 3 commits into from
Closed
Changes from 2 commits
Commits
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
211 changes: 211 additions & 0 deletions text/0000-overload-logic-operators.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,211 @@
- 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

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;

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

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

Choose a reason for hiding this comment

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

These definitions seem to discard information, so seem like they'd be less than optimal when writing an impl.

For example, if short_circuit_or returns Short for an Ok, then logical_or still gets passed the whole Result, and would need to do unimplemented!() or something in the Ok arm instead of only being passed the Err part.

If it were, instead, -> ShortCircuit<Self::Short, Self::Long>, then it could be logical_or(Self::Long, Rhs). But of course then short_circuit becomes exactly the same as Try::into_result...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we have a distinct long type (intermediate) then the short_circuit for Result would still just be rhs.

}

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

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

/// Complete the *logical and* in case it did not short-circuit.
fn logical_and(self, 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) => lhs.logical_or(expr_b)
}
```

and

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

==>

match expr_a.short_circuit_and() {
ShortCircuit::Short(res) => res,
ShortCircuit::Long(lhs) => lhs.logical_and(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

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")

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)
Nokel81 marked this conversation as resolved.
Show resolved Hide resolved
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

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

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 trait LogicalOr<T> for Option<T> {
Nokel81 marked this conversation as resolved.
Show resolved Hide resolved
type Output = T;
}

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

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

impl trait 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 => truthiness and is undesirable.
Nokel81 marked this conversation as resolved.
Show resolved Hide resolved

# 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 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.
- Two alternatives were discussed and shown to be inferior in either performance, explicitness, or design.
- 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.
Nokel81 marked this conversation as resolved.
Show resolved Hide resolved
- 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.

Nokel81 marked this conversation as resolved.
Show resolved Hide resolved
# 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