-
Notifications
You must be signed in to change notification settings - Fork 1.6k
RFC: Imply Option #2180
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
RFC: Imply Option #2180
Conversation
My RFC for adding some function implimentation for the Option type akin to the `implies` operation from predicate logic.
Formatted poorly displayed code and pseudo-code to display correctly and be legible.
I am not familiar with predicate logic. Can this RFC be described as “add a pair of constructors to |
Yes @SimonSapin that is the idea in layman's terms. "If some boolean is |
This would be a useful addition, but I'd personally want to see it as a method on // These methods are just copied from `Option`
impl bool {
fn xxx<T>(self, b: T) -> Option<T>; // This one is missing on `Option`
fn map<T, F>(self, f: F) -> Option<T> where F: FnOnce() -> T;
fn and<T>(self, optb: Option<T>) -> Option<T>;
fn and_then<T, F>(self, f: F) -> Option<T> where F: FnOnce() -> Option<T>;
}
let my_opt_value = my_bool.xxx(get_value());
let my_opt_value = my_bool.map(|| get_value());
let my_opt_value = my_bool.and(get_opt_value());
let my_opt_value = my_bool.and_then(|| get_opt_value()); |
Previous discussion on internals: https://internals.rust-lang.org/t/bool-into-option-closure-yields-none-or-some-closure/1729 The general consensus seemed to be to use the |
@petrochenkov While I agree they should be methods on |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typos & such improvements (mostly)
text/0000-imply-option.md
Outdated
# Motivation | ||
[motivation]: #motivation | ||
|
||
This addition will increase the legability of code segments and assist in defining the thought processes and motivations of programmers through code. The use cases of this addition are solutions which are expressable in the following predicate form: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
legability
-> legibility
.
text/0000-imply-option.md
Outdated
None | ||
} | ||
``` | ||
The outcome of this addition will reduce repeated code which introduces bugs during refactoring and present the thought process of the programmer in a clearer fasion through their code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fasion
-> fashion
text/0000-imply-option.md
Outdated
|
||
The `Option` type is extremely useful when your code may or may not yield a return value. | ||
Such code may looks similar to this: | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extremely useful
is probably true, but also too much. useful
is probably sufficient.
text/0000-imply-option.md
Outdated
``` | ||
But the `else` branch is required for returning `None` value if `x == 0` evaluates to false. | ||
Fortunately Rusts `Option` type has functionality get rid of the unecessary code: | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing a to
between functionality
and get
.
text/0000-imply-option.md
Outdated
|
||
Option::on_pred(x == 0, x) | ||
``` | ||
This code performs the exact same function as our original `if` statement however our code is compressed to a single line and our intentions are just as clear. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to: "This code has the exact same behaviour as our original if
statement. Our code is however compressed to a single line and our intentions are just as clear."
text/0000-imply-option.md
Outdated
} | ||
} | ||
``` | ||
This implementation covers the use cases I've proposed in my earlier examples and any others of similar form without any external dependancies; this should make the implementation stable as Rust continues to develope. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dependancies
->dependencies
- depersonalize
develope
->develop
text/0000-imply-option.md
Outdated
# Drawbacks | ||
[drawbacks]: #drawbacks | ||
|
||
This is a functionality which has functional programming and monads in mind with its design and this may make it another stepping stone to be learned for programmers which are new to Rust or functional programming concepts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rust already has a lot of FP concepts and actual monadic bind in Option::and_then
, Iterator::flat_map
in libcore - I don't see why adding a small addition like this would be anything new FP-wise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not saying it would be anything new, rather it would be just one more little thing which needs to be learned when considering new programmers.
text/0000-imply-option.md
Outdated
# Rationale and alternatives | ||
[alternatives]: #alternatives | ||
|
||
The implementation I've proposed is clear and easily documented and is the minimal ammount of code and change necessary to add this into the Rust language without sacrificing any of the advantages of the `if/else` blocks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Depersonalize
text/0000-imply-option.md
Outdated
Option::lazy_pred(true, || x) | ||
``` | ||
It is very little boiler plate code compared to the `if/else` alternative but it is suboptimal from an execution standpoint and a more obtuse implementation for new Rust programmers to learn. | ||
- Not including the `lazy_pred` function. However, as discussed, this leaves the `on_pred` function at a disadvantage when the equivilant `if` block is computationally intesive as it wastes computation on a value which may simply be discarded. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
equivilant
-> equivalent
text/0000-imply-option.md
Outdated
``` | ||
It is very little boiler plate code compared to the `if/else` alternative but it is suboptimal from an execution standpoint and a more obtuse implementation for new Rust programmers to learn. | ||
- Not including the `lazy_pred` function. However, as discussed, this leaves the `on_pred` function at a disadvantage when the equivilant `if` block is computationally intesive as it wastes computation on a value which may simply be discarded. | ||
- Providing syntax support for this implementation in Rust (similar to the `?` operator for the `Result` type). However I argue that pushing the abstraction of the logic this far reduces the clarity of the code and the expression of the programmers intention. Additionally discussion has yet to addiquately cover syntax support for both the `on_pred` and `lazy_pred` functions in a meaningful manner and removing either one is disadvantages as discussed above. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addiquately
->adequately
disadvantages
->disadvantageous
@eddyb and |
"I’ve sometimes written code that this would have made simpler." is IMO not a good argument to add a completely new operator to the language. It needs to be used really really often for that to be the case. If there is one special area where the code would benefit from it, just define a macro. |
@est31 Was an actual operator proposed (with sigils and stuff...) or just a method? The bar for the former is of course significantly higher. But the bar is still high for methods. |
It’s a methods. (Or rather, two.) |
Fixed some spelling errors spotted by @Centril
I do feel that @est31 has a point in saying that a macro could be the appropriate measure to take (I do often forget about macros). An implementation such as the following could possibly be better:
This has the additional benefits of being inherently
|
@SimonSapin @Centril @Dynisious oh sorry, I've only skimmed the proposal and got confused a little. Disregard my comment then! Adding a method is entirely reasonable. 👍 |
@Centril Yeah, I was implying those definitions for |
@eddyb That sounds like a non-starter ;) |
Could the syntax for Strawman proposal for the latter:
Improvement opportunities for the strawman:
Non-serious proposal for the struct False;
struct True;
impl ops::Try for bool {
type Ok = True;
type Error = False;
fn into_result(self) -> Result<True, False> {
if self {
Ok(True)
} else {
Err(False)
}
}
fn from_ok(_: True) -> Self { true }
fn from_error(_: False) -> Self { false }
} |
@oli-obk I have looked into the boolinator crate after you've mentioned it and I do recognise the similarities. However, the discussion here is about implementing similar functionality in the standard rust libraries; if the consensus of the community is that the full implementation of |
@petrochenkov Implementing this functionality as methods on
|
It seems better to just add my_bool.into().map(|_| my_value) I don't think you need an RFC to add that impl just a PR with a final comment period. |
@leodasvacas I think that |
I'm pretty sure this won't pass type inference. |
My suggestion was mistaken, I can restate my sugestion as adding a method to bool that would make the conversion, such as my_bool.optionalize().map(|_| value)
Option::lazy_pred(my_bool, |_| value) |
I agree that it could be advantageous to convert |
@leodasvacas |
@Centril ...both because "isomorphism" is a very jargon-y word that I'd actually never heard before (and I've been programming in languages no more functional than Python for over 15 years) and because abbreviating it to "iso" is like having a crate named "api-binding". There are simply too many things "iso" could be short for. |
The usual problem with generalizing the return type to a generic is that it needs to be inferred from context. For this case specifically, the strawman posted wouldn't even work with |
@scottmcm Fair enough =) |
Added the Legal Double Reference RFC.
I think adding the methods on the This RFC's title seems very misleading however. |
What's the status of this RFC? I've often wanted similar functionality — is the main blocker here deciding upon method names? |
I'm unassigning myself and nominating for libs team discussion. |
Just for completeness. The following works on nightly. let pred = true;
let filtered = Some("test").filter( |_| pred ); |
@nielsle I don't think that quite covers the use-case. Specifically, consider the case where the pred.if_true(|| make_expensive_thing()) |
Hey been a while since I did anything with this. |
@rfcbot fcp postpone This RFC is unfortunately a little old at this point and has languished for awhile. At this point I think it's clear that we don't have the bandwidth or motivation at this point to push the bikeshed here to completion to figure out what names these methods would have in the standard library. In the meantime there's a crate on crates.io for these methods which isn't great in terms of ergonomics but provides a good way to test out the eventual method names! I propose we postpone this RFC to a later date when the bikeshed can be driven to completion. Otherwise we're not necessarily benefitting much from having this RFC bake in the queue. |
Team member @alexcrichton has proposed to postpone this. The next step is review by the rest of the tagged teams: No concerns currently listed. Once a majority of reviewers approve (and none object), 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. |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
The final comment period, with a disposition to postpone, as per the review above, is now complete. By the power vested in me by Rust, I hereby postpone this RFC. |
I've opened a follow up RFC here: #2757. |
Specification
My RFC for adding some function implementation for the
Option
type akin to theimplies
or->
operation from predicate logic. Useful for getting rid of boiler plate code and making the programmers intention clear in their code.Updated Introduction
The conversation has been going on and things have changed in the specification; I have created a more appropriate introduction further down but left this one unchanged to keep the context of the earlier conversation.