Allow Overloading || and && #2722
Conversation
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 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;
} |
|
@Nokel81 Please provide the full implementation of the traits for Also, shouldn't we follow precedent of @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 |
|
@KrishnaSannasi Good point, I like that better. |
|
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) |
The intermediate type allows us to minimize coupling between the two functions, for example we could implement 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 For comparison, here is the implementation for 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 |
| # Guide-level explanation | ||
| [guide-level-explanation]: #guide-level-explanation | ||
|
|
||
| This proposal starts with an enum definition and trait definitions for each of the operators: |
There was a problem hiding this comment.
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.)
|
|
||
| /// 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; |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
If we have a distinct long type (intermediate) then the short_circuit for Result would still just be rhs.
|
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 But given that the design has an intermediate value, it's a bit strange to require it to be the same type as |
Centril
added
T-lang
| 2. Could lead to similarities to C++'s Operator bool() which => truthiness and is undesirable. | ||
|
|
||
| # Rationale and alternatives | ||
| [rationale-and-alternatives]: #rationale-and-alternatives |
There was a problem hiding this comment.
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||
There was a problem hiding this comment.
I don't understand the two traits question. Is it really necessary to discuss why each operator should have its own trait?
|
How is this RFC going to be compatible with rust-lang/rust#53667 given that you want to desugar (I also think that allowing |
|
Another more meta point... ...How does this fit into the roadmap? |
@Centril In the sense of "nobody would ever write that" or in the sense of "I don't think 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 |
|
@Centril I thought that
would desugar to something like if let Some(x) = foo() {
if bar(x) {
}
}Because of the
Like @scottmcm said, we could special case certain types (
I find this just as strange as allowing |
|
|
@KrishnaSannasi As for the having a "short circuit" trait and then relying on the |
This being strange because it is an odd way to provide a default value? |
Co-Authored-By: kennytm <[email protected]>
The issue is that having both |
That's not what I meant, I meant that if someone wanted to not use short-circuit behavior they should use |
Neither of those reasons really.
I think that a binary operator like this taking an expression of a different type is peculiar and surprising. I'm aware that
This special casing would need to happen after you have type information. Right now, it is simply assumed in
Currently the compiler has an Rather, the problem here is drop order. More specifically, an expression At minimum I think the implementation of |
|
As repeatedly stated above, |
KodrAus
added
the
Libs-Tracked
| 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 |
There was a problem hiding this comment.
| 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")
There was a problem hiding this comment.
Some(3) || return
Some(3) || return == Some(3) similar to how Some(1) || panic!() == Some(1)
How about
OptiontoResulttype? And vice versa?
There are already functions to do that conversion, so it doesn't seem necessary.
| 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 |
There was a problem hiding this comment.
| 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.
| # Drawbacks | ||
| [drawbacks]: #drawbacks | ||
|
|
||
| 1. Leaves the `||` and the `&&` as not strictly boolean operators, which might hurt readability |
There was a problem hiding this comment.
| 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. |
| 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. |
There was a problem hiding this comment.
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
**kwargsimplicitness, 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
|
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 For instance, match a.branch() {
ControlFlow::Continue(c) => c,
ControlFlow::Break(_) => b
} |
|
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));
} |
|
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 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();as let x = try {
some_option?;
other_option?;
get_an_option()
} |
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 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 That said, they're not |
|
Now, regardless of my previous post, I do think this point from @PoignardAzur is important:
I agree that we don't yet know how a stable 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 (I don't know if that strictly means postpone or close, but whatever.) |
|
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. |
rfcbot
added
proposed-final-comment-period
|
@rfcbot reviewed |
rfcbot
added
final-comment-period
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
rfcbot
added
the
finished-final-comment-period
|
The final comment period, with a disposition to postpone, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. The RFC is now postponed. |
rfcbot
added
to-announce
postponed
Remove wrong note for short circuiting operators They *are* representable by traits, even if the short-circuiting behaviour requires a different approach than the non-short-circuiting operators. For an example proposal, see the postponed [RFC 2722](rust-lang/rfcs#2722). As it is not accurate, remove most of the note.
Remove wrong note for short circuiting operators They *are* representable by traits, even if the short-circuiting behaviour requires a different approach than the non-short-circuiting operators. For an example proposal, see the postponed [RFC 2722](rust-lang/rfcs#2722). As it is not accurate, remove most of the note.
This is an rfc for allowing the user-of-rust to overload
||and&&for their own types. And adding various implementations forOption<T>andResult<T, E>Rendered