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

Tracking issue for RFC 2306, "Add core::convert::identity" #53500

Closed
Centril opened this Issue Aug 19, 2018 · 8 comments

Comments

@Centril
Copy link
Contributor

Centril commented Aug 19, 2018

This is a tracking issue for the RFC "Add fn identity<T>(x: T) -> T { x } to core::convert" (rust-lang/rfcs#2306).

Steps:

Unresolved questions:

None.

@Centril Centril self-assigned this Aug 19, 2018

Centril added a commit to Centril/rust that referenced this issue Aug 20, 2018

bors added a commit that referenced this issue Aug 20, 2018

Auto merge of #47562 - Centril:feature/core_convert_id, r=oli-obk
Add the identity function as core::convert::identity

## New notes

This implements rust-lang/rfcs#2306 (see #53500).

## Old notes (ignore this in new reviews)

Adds the identity function `fn id<T>(x: T) -> T { x }` to core::convert and the prelude.
Some motivations for why this is useful are explained in the doc tests.
Another is that using the identity function instead of `{ x }` or `|x| x` makes it clear that you intended to use an identity conversion on purpose.

The reasoning:
+ behind adding this to `convert` and not `mem` is that this is an identity *conversion*.
+ for adding this to the prelude is that it should be easy enough to use that the ease of writing your own identity function or using a closure `|x| x` doesn't overtake that.

I've separated this out into two feature gates so that the addition to the prelude can be considered and stabilized separately.

cc @bluss

@Centril Centril added the B-unstable label Sep 18, 2018

@shepmaster

This comment has been minimized.

Copy link
Member

shepmaster commented Sep 26, 2018

The documentation suggests:

#![feature(convert_id)]
use std::convert::identity;

let iter = vec![Some(1), None, Some(3)].into_iter();
let filtered = iter.filter_map(identity).collect::<Vec<_>>();
assert_eq!(vec![1, 3], filtered);

I'd argue this is actively non-idiomatic Rust. It is better to use flatten

let iter = vec![Some(1), None, Some(3)].into_iter();
let filtered = iter.flatten().collect::<Vec<_>>();
assert_eq!(vec![1, 3], filtered);

I'm bringing this up here because if we can't have good examples, perhaps the feature doesn't carry it's own weight.

@shepmaster

This comment has been minimized.

Copy link
Member

shepmaster commented Sep 26, 2018

The documentation suggests using identity to "do nothing among other interesting functions" and "that changes nothing in a conditional" (which are really the same example). I think this is very weak motivation because it cannot be used with a function of different arity:

#![feature(convert_id)]
use std::convert::identity;

fn manipulation(x: u32, y: u32) -> u32 {
    x + y
}

fn main() {
    let do_stuff = if true { manipulation } else { identity };
    let _results = do_stuff(1, 2);
}
error[E0308]: if and else have incompatible types
 --> src/main.rs:9:20
  |
9 |     let do_stuff = if true { manipulation } else { identity };
  |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ incorrect number of function parameters
  |
  = note: expected type `fn(u32, u32) -> u32 {manipulation}`
             found type `fn(_) -> _ {std::convert::identity::<_>}`
@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Sep 26, 2018

@shepmaster

So .flatten() was added to libcore as a direct consequence of discussing identity.

However, I think that:

let filtered = iter.filter_map(identity).collect::<Vec<_>>();

is more clear with respect to intent.

To know that .flatten() is equivalent to .filter_map(identity) one has to remember the behavior of Option<T> as IntoIterator so I would write the latter rather than the former. Note however that .flatten() is Iterator specific and may not exist for your random type. For example, there is no prop_flatten in Strategy. In such a case .flatten() can't be idiomatic because it does not exist.

I think this is very weak motivation because it cannot be used with a function of different arity:

Given that Rust is a typed language I am unsure as to why one would expect this to work with functions of different arity. This would be the same as expecting impl Fn(A) -> A to also support impl Fn(A, A) -> A. However, if you partially apply manipulation you can use identityand manipulation in a list of boxed closures.

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Dec 13, 2018

@SimonSapin How do you feel about stabilizing this?

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Dec 14, 2018

Let’s.

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Dec 14, 2018

Team member @SimonSapin has proposed to merge 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.

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Jan 4, 2019

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Jan 14, 2019

The final comment period, with a disposition to merge, as per the review above, is now complete.

Centril added a commit to Centril/rust that referenced this issue Jan 14, 2019

Rollup merge of rust-lang#57322 - Centril:stabilize-identity, r=Simon…
…Sapin

Stabilize core::convert::identity

r? @SimonSapin

fixes rust-lang#53500

This is waiting for FCP to complete but in the interim it would be good to review.

bors added a commit that referenced this issue Jan 14, 2019

Auto merge of #57322 - Centril:stabilize-identity, r=SimonSapin
Stabilize core::convert::identity

r? @SimonSapin

fixes #53500

This is waiting for FCP to complete but in the interim it would be good to review.

@bors bors closed this in #57322 Jan 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment