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

Missing test coverage for builtin Eq/Hash/Show impl of tuples #1186

Open
hackwaly opened this issue Nov 1, 2024 · 4 comments
Open

Missing test coverage for builtin Eq/Hash/Show impl of tuples #1186

hackwaly opened this issue Nov 1, 2024 · 4 comments
Labels
good first issue Good for newcomers

Comments

@hackwaly
Copy link
Contributor

hackwaly commented Nov 1, 2024

No description provided.

@hackwaly hackwaly added the good first issue Good for newcomers label Nov 1, 2024
@definfo
Copy link

definfo commented Nov 27, 2024

It seems that Eq tests already exist in tuple/tuple_test.mbt, while Compare/Hash tests are still missing.

@FlammeShadow
Copy link
Contributor

FlammeShadow commented Dec 1, 2024

According to the current implementation, it seems that the hash value of (1, 2, 3) and (1, (2, 3)) are the same, and so on. Is this a potential problem? Or it doesn't make sense?
I refered to the boost::hash_value and got different result on std::tuple (1, (2, 3)) and (1, 2, 3). I didn't test Swift because its tuple types don't conform to Hashable protocol.

@definfo
Copy link

definfo commented Dec 1, 2024

According to the current implementation, it seems that the hash value of (1, 2, 3) and (1, (2, 3)) is the same, and so on. Is this a potential problem? Or it doesn't make sense? I refered to the boost::hash_value and got different result on std::tuple (1, (2, 3)) and (1, 2, 3). I didn't test Swift because its tuple types don't conform to Hashable protocol.

I tested the hash implementation for tuple in Rust and got some weird results.

use std::hash::{DefaultHasher, Hash, Hasher};

fn calculate_hash<T: Hash>(t: &T) -> u64 {
    let mut s = DefaultHasher::new();
    t.hash(&mut s);
    s.finish()
}

fn main() {
    let tuple1 = (1, 2, 3);
    let tuple2 = (1, (2, 3));
    let tuple3 = (1 as u32, 2, 3);
    let tuple4 = (1 as i64, 2, 3);
    println!(
        "{} {} {} {}",
        calculate_hash(&tuple1), // 646939227381880718 
        calculate_hash(&tuple2), // 646939227381880718
        calculate_hash(&tuple3), // 646939227381880718
        calculate_hash(&tuple4), // 4417724211370468105
    ); 
    assert_eq!(calculate_hash(&tuple1), calculate_hash(&tuple2));
    assert_eq!(calculate_hash(&tuple1), calculate_hash(&tuple3));
    assert_ne!(calculate_hash(&tuple1), calculate_hash(&tuple4));
}

Moonbit's current implementation is consistent with Rust, except that multiple builtin types have not implemented Hash trait yet (eg. Int64, Array[Int] #938 ).

test "hash" {
    let tuple1 = (1, 2, 3)
    let tuple2 = (1, (2, 3))
    let tuple3 = (1U, 2, 3)
    // let tuple4 = (1L, 2, 3)
    inspect!(tuple1.hash() == tuple1.hash(), content="true")
    inspect!(tuple1.hash() == tuple2.hash(), content="true")
    inspect!(tuple1.hash() == tuple3.hash(), content="true")
    // inspect!(tuple1.hash() == tuple4.hash(), content="false") // Type Int64 does not implement trait Hash
}

@FlammeShadow
Copy link
Contributor

It seems that Eq tests alreay exist

Show tests seem to exist too. So I just added Compare/Hash tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants