-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Drop unstable Option::contains
, Result::contains
, Result::contains_err
#108095
Conversation
(rustbot has picked a reviewer for you, use r? to override) |
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
@Mark-Simulacrum does this require an API Change Proposal, or is the tracking issue enough? |
Tagging as T-libs-api, because even if it's unstable, this is that domain not T-libs. |
r? libs-api |
I started an FCP to close #62358. |
@Amanieu thanks for having a look! I'd say my motivation is less about
as many people found the original ticket in the first place because they intuitively assumed that It's more about freeing up the name again such that it can be used without getting pestered by rustc/IDEs. |
Could we keep just 'Option::contains'? In #62358 which I think closed prematurely, I noted that 'Result::contains' has a ambiguity problem when the type is 'Result<T, T>', but I see no problem with 'Option::contains'. Additionally, we might have 'Result::ok_contains' (instead of the current 'Result::contains') and 'Result::err_contains'. I see no reason why we can't have both these functions and the closure based functions, and get the best of both worlds. |
The problem with For types that implemented |
Actually, my mistake, I misread #20063. It seems that |
@Amanieu I'm not interested in re-litigating I just want it gone so I can use |
I can't believe I forgot about |
Semantics weren't the issue, to be honest.
Take any of that away, and interest crumbles; I'd expect that |
@bors r+ |
☀️ Test successful - checks-actions |
Finished benchmarking commit (cdbbce0): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
|
This is a proposal to drop the three functions
Option::contains
,Result::contains
andResult::contains_err
.The discovery of
Option::is_some_with
/Result::is_ok_with
/Result::is_err_with
in #93051 obviates the need for these methods (non-stabilization tracked in #62358).An additional benefit of change is that it avoids spurious error messages in IDEs, when
contains
is supplied by a third-party library: