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

Keyless Entry #1533

Closed
wants to merge 1 commit into from
Closed

Keyless Entry #1533

wants to merge 1 commit into from

Conversation

gereeter
Copy link

@gereeter gereeter commented Mar 6, 2016

Fixes #1203.

@killercup
Copy link
Member

Rendered

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Mar 7, 2016
@pczarn
Copy link

pczarn commented Mar 10, 2016

All possible changes have tradeoffs. For me, breaking backwards compatibility is a non-starter.

Perhaps there is a way of extending the current Entry API with genericity and conversion traits like so:

enum Entry<'a, K, V, Q = K> {
    Occupied(OccupiedEntry<'a, K, V>),
    Vacant(VacantEntry<'a, K, V, Q>),
}
// method on HashMap
fn entry<Q>(&mut self, key: Q) -> Entry<K, V, Q>
    where K: Borrow<Q>
// method on VacantEntry
fn or_insert<W>(self, value: W) -> &'a mut V
    where (K, V): From<(Q, W)>

Today's Entry is a special case of this interface with (K, V) == (Q, W). For keyless entry, you'd have a conversion from (Placeholder, (K, V)) to (K, V), and uglier invocations of entry.or_insert((k, v)) as the only disadvantage. If trait coherency is a problem, I suppose there could be a new type for key-value pairs. The biggest issue with genericity is always complexity of documentation and much more confusing error messages.

edit 30.06: fixed a bound

@pczarn
Copy link

pczarn commented Mar 15, 2016

Let's reconsider what needs to be done.

The current Entry API is insufficient in two scenarios:

  • I own a key, and want to retain that key if the entry is occupied.
  • I have a reference to a key, and I'm not sure if I will need to clone that key to insert it into a vacant entry.

The problem in the first scenario is simple, because it only concerns OccupiedEntry.
The problem in the second scenario concerns fn entry and VacantEntry, so it's basically orthogonal to the first problem.

OccupiedEntry doesn't own a key simply because there was no internal need for a retained key, until now. However, the user may want to retain a key, so we modify OccupiedEntry and add a method for key recovery. This fixes the first scenario, which appears in my refactoring PR.

struct OccupiedEntry<'a, K, V> {
    key: Option<K>,
    elem: FullBucket<...>
}

impl<'a, K, V> OccupiedEntry<'a, K, V> {
    pub fn take_key(&mut self) -> Option<K> {
        self.key.take()
    }
}

@Stebalien
Copy link
Contributor

The original purpose of the entry API was to prevent double lookups. This can't fix that while entry_or_clone can.

@gereeter
Copy link
Author

All possible changes have tradeoffs. For me, breaking backwards compatibility is a non-starter.

I completely agree - I included the backwards incompatible alternative just for completeness. The main proposal is not backwards incompatible in any way.

Perhaps there is a way of extending the current Entry API with genericity and conversion traits like so:

This sort of API would probably be ideal from a usability standpoint, but I don't see any way that it can work. You take Q by value to ensure compatibility with passing a by-value key, but this doesn't work with implementations of Borrow, as, e.g., str: Borrow<String>, not &str, so you would have to pass a str by value, which is impossible. I don't think that any other trait would fare any better, though I can't completely rule that out.

There may also be compatibility issues with broken type inference, but I don't know how much of a problem that would be.

The current Entry API is insufficient in two scenarios:

  • I own a key, and want to retain that key if the entry is occupied.
  • I have a reference to a key, and I'm not sure if I will need to clone that key to insert it into a vacant entry.

Exactly. This RFC aims to solve only the second problem.

The original purpose of the entry API was to prevent double lookups. This can't fix that while entry_or_clone can.

I'm confused as to why you think that - not only can this API be used to prevent double lookups, but entry_or_clone can be implemented in terms of this API:

fn entry_or_clone(&mut self, key: &K) -> Entry<K, V> where K: Clone {
    match self.keyless_entry(key) {
        KeylessEntry::Occupied(entry) => Entry::Occupied(entry)
        KeylessEntry::Vacant(entry) => Entry::Vacant(entry.with_key(key.clone()))
    }
}

@Stebalien
Copy link
Contributor

I'm confused as to why you think that - not only can this API be used to prevent double lookups, but entry_or_clone can be implemented in terms of this API

Sorry, I didn't read the drawbacks. IMO, not being able to guarantee that the key is the same is way too much of a foot gun to be acceptable.

@codyps
Copy link

codyps commented Mar 16, 2016

@Stebalien

could you clarify what you mean by

not being able to guarantee that the key is the same

@Stebalien
Copy link
Contributor

@jmesmon A user could write:

self.keyless_entry(&key1).or_insert_with(||(key2, 0)) += 1;

and end up with key2 in key1's slot.

@comex
Copy link

comex commented Mar 16, 2016

Alternate alternative: combine this proposal and the entry_or_clone one. Use an alternate Entry type (or just extend the existing one to support the new functionality), but have its insert method call to_owned() on the key rather than allowing it to be specified explicitly, reducing footguniness at the cost of some flexibility. (to_owned() should be preferred over clone() because of things like searching a HashMap<String, X> with &str.)


- Replace the `Entry` API entirely. This would get rid of the inherent duplication of having two `Entry` APIs,
but would be backwards incompatible.
- Create an `OccupiedKeylessEntry` that is separate from `OccupiedEntry` to help keep to two different `Entry`s
Copy link

Choose a reason for hiding this comment

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

Please add a note saying this alternative is an addition to the main proposal.

(or make three categores of alternatives -- backwards compatible, incompatible, and additions to the main proposal)

@aturon aturon self-assigned this Mar 23, 2016
@pczarn
Copy link

pczarn commented Jul 2, 2016

I haven't given up on this issue. It can be decomposed into two essential steps.

  • I have a borrowed key, and want a keyless entry.
  • I have a vacant keyless entry, and want to fill it by computing a key with one of these actions:
    1. I compute and give my owned key to the entry. (I am trusted to provide a key that is semantically equal to the previous one.)
    2. I want the entry to use a clone of the key it borrows.
    3. The entry already owns an key, and nothing needs to be done.

Let's focus on the first step.

An API with ideal usability has to take a key that is either borrowed or owned. Cow is a natural fit for this purpose and is passed into entry2 in hashmap2. However, we would like to keep backwards compatibility without creating a separate component. Adding Into to the mix might allow that:

pub fn entry<'a, Q, B>(&mut self, key: Q) -> Entry<K, V, B>
    where K: Into<Cow<'a, B>>,
          Q: Into<Cow<'a, B>>,
          B: Eq + Hash + ToOwned + 'a,
{
    self.reserve(1);
    let key_cow: Cow<'a, B> = key.into();
    self.search_mut(&key_cow).into_entry(key_cow).expect("unreachable")
}

The compiler will complain about K: Clone not being satisfied in various places. Yes, we broke backward compatibility. I gave up on trying to work around that. Instead, we can write simpler bounds with Borrow in a similar manner. This will require a method with default type parameters that drive inference.

pub fn entry<Q = K, B: ?Sized = K>(&mut self, key: Q) -> Entry<K, V, Q>
    where Q: Borrow<B>,
          K: Borrow<B>,
          B: Eq + Hash,
{
    self.reserve(1);
    self.search_mut(Borrow::<B>::borrow(&key)).into_entry(key).expect("unreachable")
}

These default type parameters are necessary, because some types have two different implementations of Borrow<…> and Borrow<Self>. We ask the compiler's inference to choose Borrow<Self>. This way, backward compatibility is maintained for the following code:

fn foo<K>(map: HashMap<String,>, key: String) {
    map.entry(key);
    // Types can be inferred in two ways:
    //   first
    map.entry::<String, String>(key);
    //   second, due to `String: Borrow<str>`
    map.entry::<String, str>(key);
}

Unfortunately, default_type_parameter_fallback is unstable as a gated feature.

As for the second step, the Entry API should be able to provide a function that transforms an Entry<K, V, Q> into Entry<K, V, K>.

@cristicbz
Copy link

So I've got a suggestion (playground) which may not require an additional method. The idea is to introduce a new IntoOwned trait which unlike ToOwned takes self by-value:

pub trait IntoOwned<T>{
    fn into_owned(self) -> T;
}

impl<T> IntoOwned<T> for T {
    fn into_owned(self) -> T { self }
}

impl<'a, T> IntoOwned<T> for &'a T where T: Clone {
    fn into_owned(self) -> T {
        (*self).clone()
    }
}

impl<'a> IntoOwned<String> for &'a str {
    fn into_owned(self) -> String {
        self.to_owned()
    }
}

/* ... */

The entry method's signature can then look like this:

    // Entry would have to gain Q: IntoOwned<K> as a type param with default Q=K.
    fn entry<'a, Q>(&'a mut self, key: Q) -> Entry<'a, K, V, Q>
        where K: Hash + Eq,
              Q: Hash + Eq + IntoOwned<K>
    {
        // Build an `Entry` which stores `key` and calls `into_owned`
        // on vacant insertion only.     
    }

The trouble with this is that we'd need to duplicate all the existing ToOwned impl-s (with some &'a-s in there). This is because

  impl<'a, T> IntoOwned<T::Owned> for &'a T where T: ToOwned

conflicts with impl<T> IntoOwned<T> for T due to the T::Owned == T possibility. Had we gone this route to begin with (and not have ToOwned) this version might have been more appealing.

As it stands, we could keep IntoOwned unstable and provide impl-s for T: Clone and the std smart pointers. It'd still greatly improve the situation, but the lack of extensibility for any Borrow/ToOwned pairs is disappointing.

@cristicbz
Copy link

Just to bring in some conversation from IRC. The only coherence trouble the impl runs into is of the form impl<T, U> X<T> for U where T != U. The typical arguments against negative reasoning don't apply here (at first glance anyway), so assuming that where clause was expressible the following ought to work:

impl<T> IntoOwned<T> for T {
    fn into_owned(self) -> T { self }
}

impl<'a, T> IntoOwned<T::Owned> for &'a T
    where T: ToOwned,
          T::Owned != &'a T
{
    fn into_owned(self) -> T::Owned {
        (*self).to_owned()
    }
}

@cristicbz
Copy link

I worked around the coherence issues an wrote up an RFC #1769

@strega-nil
Copy link

ping @gereeter @aturon

Status? Should this be closed, given the existence of #1769?

@aturon
Copy link
Member

aturon commented Jan 7, 2017

I've just proposed a move to FCP on #1769. I'm going to propose to close this RFC as part of the same decision.

@rfcbot fcp close

@rfcbot
Copy link
Collaborator

rfcbot commented Jan 7, 2017

Team member @aturon has proposed to close this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, 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.

@gereeter
Copy link
Author

gereeter commented Jan 7, 2017

I approve of this being closed - I think #1769 is a better solution.

@alexcrichton
Copy link
Member

Ok, all checkboxes checked, closing in favor of #1769

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.