Skip to content
GitHub no longer supports this web browser. Learn more about the browsers we support.
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

Implement :maps.keys/1 #277

Merged
merged 1 commit into from Sep 2, 2019
Merged

Implement :maps.keys/1 #277

merged 1 commit into from Sep 2, 2019

Conversation

@QuinnWilton
Copy link
Contributor

QuinnWilton commented Aug 31, 2019

This resolves #249

I'm not entirely sure if I've done this right, but my (admittedly weak) property tests pass. I haven't coded Rust before, so it's likely that this isn't very idiomatic. I'm happy to receive any feedback, no matter how nitpicky it is :)

I also wasn't sure whether I was supposed to register this BIF in liblumen_eir_interpreter. Doing so would require making the native function public in the BIF, which seems to go against how most of the other BIFs are implemented, so I held off on that for now. Let me know if you want me to do that, and I'm happy to make that change!

@@ -47,6 +47,13 @@ impl Map {
self.value.contains_key(&key)
}

pub fn keys(&self) -> Vec<Term> {

This comment has been minimized.

Copy link
@QuinnWilton

QuinnWilton Aug 31, 2019

Author Contributor

Is it correct for this function to be returning Vec<Term> rather than a list directly? That's the impression I got from reading some of the other code, but I just want to verify that this is the interface you're intending to expose

This comment has been minimized.

Copy link
@KronicDeth

KronicDeth Sep 1, 2019

Collaborator

Yes because making a list would require passing in the Process or HeapAlloc to store the Cons cells inside, so this way separates getting the data from putting it in a format in the process in case runtime or alloc internal code doesn't really need a list in the process.

use liblumen_alloc::erts::term::atom_unchecked;

#[test]
fn returns_list_of_keys() {

This comment has been minimized.

Copy link
@QuinnWilton

QuinnWilton Aug 31, 2019

Author Contributor

I'm not a huge fan of this property test. I think that ideally I'd be generating a list of terms, and then using that list to construct the map.

I was running into two issues with that approach though:

  1. Since :maps.keys/1 doesn't guarantee any ordering, I wasn't sure of what the most idiomatic way would be to compare the two lists at the end (since just doing a straight equality check may be flaky)
  2. I'm not clear yet on the cleanest ways of manipulating maps / lists / terms in general from Rust, and specifically how I'd actually create the map from a list of keys.

If you have any advice on making this test more thorough, I'm happy to hear it :)

This comment has been minimized.

Copy link
@KronicDeth

KronicDeth Sep 1, 2019

Collaborator

🤷‍♂ this is good enough. I've also not had a good approach for these order independent invariants either. You could use proptest::collection::vec(strategy::term(), 0..3) to get a vector of 0-3 terms and then sort both the vector and the keys that are output to make sure they match, but it is fine without that.

If you're not going to go that complex, I would add an additional test that checks that a map with no keys returns an empty list (Term::NIL) as that is an important edge case.

let mut key_vec: Vec<Term> = Vec::new();
key_vec.extend(self.value.keys());

key_vec

This comment has been minimized.

Copy link
@archseer

archseer Sep 1, 2019

This entire function can be simplified into:

pub fn keys(&self) -> Vec<Term> {
  self.value.keys().into()
}

This comment has been minimized.

Copy link
@KronicDeth

KronicDeth Sep 1, 2019

Collaborator

@ShaneWilton switch to @archseer's recommendation.

This comment has been minimized.

Copy link
@QuinnWilton

QuinnWilton Sep 1, 2019

Author Contributor

Thanks for the suggestion! I tried making this change, and it looks like trait isn't implemented for the types in question:

51 |         self.value.keys().into()
   |                           ^^^^ the trait `std::convert::From<hashbrown::map::Keys<'_, erts::term::term::Term, erts::term::term::Term>>` is not implemented for `std::vec::Vec<erts::term::term::Term>`
   |
   = help: the following implementations were found:
             <std::vec::Vec<T> as std::convert::From<&[T]>>
             <std::vec::Vec<T> as std::convert::From<&mut [T]>>
             <std::vec::Vec<T> as std::convert::From<std::borrow::Cow<'a, [T]>>>
             <std::vec::Vec<T> as std::convert::From<std::boxed::Box<[T]>>>
           and 5 others

This comment has been minimized.

Copy link
@archseer

archseer Sep 1, 2019

Hmm, seems like hash brown's traits don't match the std ones. Try: self.value.keys().into_iter().into(), I think From is implemented for vec.

This comment has been minimized.

Copy link
@QuinnWilton

QuinnWilton Sep 1, 2019

Author Contributor

I'll give that a try when I'm back at a computer! Thanks for your patience with my questions, I really appreciate the help :)

use liblumen_alloc::erts::term::atom_unchecked;

#[test]
fn returns_list_of_keys() {

This comment has been minimized.

Copy link
@KronicDeth

KronicDeth Sep 1, 2019

Collaborator

🤷‍♂ this is good enough. I've also not had a good approach for these order independent invariants either. You could use proptest::collection::vec(strategy::term(), 0..3) to get a vector of 0-3 terms and then sort both the vector and the keys that are output to make sure they match, but it is fine without that.

If you're not going to go that complex, I would add an additional test that checks that a map with no keys returns an empty list (Term::NIL) as that is an important edge case.

@QuinnWilton

This comment has been minimized.

Copy link
Contributor Author

QuinnWilton commented Sep 1, 2019

Thank you for the review! I'll make these changes today.

Do you mind also confirming that #255 is about :maps.values/1 (note the s)? If so, I'll implement that as part of a separate PR once I finish this -- the code should be fairly similar.

@QuinnWilton QuinnWilton force-pushed the QuinnWilton:maps_keys branch from 5024a0f to bbd9e07 Sep 1, 2019
@QuinnWilton

This comment has been minimized.

Copy link
Contributor Author

QuinnWilton commented Sep 1, 2019

I just pushed up a new version with both comments addressed! I had to modify @archseer's suggestion a little bit, but hopefully this version is cleaner than what I had originally.

@QuinnWilton QuinnWilton force-pushed the QuinnWilton:maps_keys branch from bbd9e07 to 35512b5 Sep 1, 2019
@KronicDeth KronicDeth merged commit 184efeb into lumen:develop Sep 2, 2019
15 checks passed
15 checks passed
ci/circleci: check_formatted Your tests passed on CircleCI!
Details
ci/circleci: wasm32_chrome_examples_spawn_chain_test Your tests passed on CircleCI!
Details
ci/circleci: wasm32_chrome_lumen_web_test Your tests passed on CircleCI!
Details
ci/circleci: wasm32_firefox_examples_spawn_chain_test Your tests passed on CircleCI!
Details
ci/circleci: wasm32_firefox_lumen_web_test Your tests passed on CircleCI!
Details
ci/circleci: wasm32_linux_examples_spawn_chain_build Your tests passed on CircleCI!
Details
ci/circleci: wasm32_linux_examples_spawn_chain_deploy Your tests passed on CircleCI!
Details
ci/circleci: wasm32_linux_lumen_web_build Your tests passed on CircleCI!
Details
ci/circleci: x86_64_linux_build Your tests passed on CircleCI!
Details
ci/circleci: x86_64_linux_liblumen_alloc_test Your tests passed on CircleCI!
Details
ci/circleci: x86_64_linux_liblumen_arena_test Your tests passed on CircleCI!
Details
ci/circleci: x86_64_linux_liblumen_beam_test Your tests passed on CircleCI!
Details
ci/circleci: x86_64_linux_liblumen_core_test Your tests passed on CircleCI!
Details
ci/circleci: x86_64_linux_liblumen_eir_interpreter_test Your tests passed on CircleCI!
Details
ci/circleci: x86_64_linux_lumen_runtime_test Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.