Rust trait implementation exercise

[coriolinus]

Add a new track-specific exercise to teach implementing traits.

Per the comment here, there's continued interest in a Rust-specific exercise focusing on teaching trait implementation. I've got some ideas for how to structure such an exercise, and want to stake a claim here in order to flesh them out. That said, I'm very interested in input from track maintainers and interested bystanders as to the specifics of how to go about this.

My idea is to have users implement an arbitrary-precision Decimal type[1], and ensure that standard math is possible for them. I.e. they must implement at least Add, Sub, Mul, Div, PartialEq, and PartialOrd; tests will ensure that the ==, +, -, *, /, >, and < operators all work as expected. Before I go too far in implementing this, though, I want to talk over a few questions with track maintainers:

1. Is this too big an exercise? Sure, common math operations aren't difficult to implement, but then we're asking people to implement six different traits.
2. Should we also require Eq and Ord in addition? After all, properly speaking, a numeric type should satisfy those conditions.
3. Do we care if people simply #[derive(PartialEq, Eq)]?

---

[1] We'd have to ask people not to simply import bigdecimal, which does exactly what we require.

[coriolinus]

Tests pass locally; waiting on Travis build. Assuming Travis is happy, this should be ready to merge.

Nobody wanted to talk about my questions above, so I just went ahead and did it as I felt best.

[coriolinus]

@petertseng @ijanos I don't know if there's a "maintainers" group I can ping, and you two have reviewed my work recently.

This PR has been open for two weeks, and passing in Travis for one, and there has still been no feedback whatsoever. If improvements need to be made, I can make them, but I don't want this to slip through the cracks.

[ijanos]

You are absolutely right and I'm sorry, my maintainership can be spotty at times. I like to be thorough
with my reviews and with larger PR-s, like this one, I have to allocate more time to it and that means it gets delayed a bit.

I'd like to thank you for the contribution and please do not worry, it will not be forgotten.

[petertseng]

I don't know if there's a "maintainers" group I can ping

There is. It's @exercism/rust, but there was little way for you to know that, since those who are not in the organisation cannot see https://github.com/orgs/exercism/teams nor https://github.com/orgs/exercism/teams/rust/members probably because GitHub doesn't allow enumeration attacks.

But it doesn't matter whether you mention the team or not. Everyone in the maintainers team is watching this repo, so they get notifications whether they are mentioned or not.

This PR has been open for two weeks, and passing in Travis for one, and there has still been no feedback whatsoever

It is courteous for open source maintainers (us) to mention when we will have time to review. I am sorry for being discourteous, because I don't know. So I am forced to continue being discourteous.

[petertseng]

Is this too big an exercise? Sure, common math operations aren't difficult to implement, but then we're asking people to implement six different traits.

Does it help if I say this exercise took me 170 minutes to do? Although the first 40 minutes were a doomed attempt to get this to work without BigInt that I scrapped as soon as I saw it was doomed.

Should we also require Eq and Ord in addition? After all, properly speaking, a numeric type should satisfy those conditions.

Do I understand correctly that because there is no difference in behaviour, we can only require them by making it a compile-time error to not have them? Doesn't seem like a particularly useful test, then.

Do we care if people simply #[derive(PartialEq, Eq)]?

Well, I guess I would say that if it makes sense to derive, they should. No sense to write code that could be replaced by derive.
Just for fun, I arrange so that my solution does #[derive(PartialOrd, Ord)] as well, because I felt like skipping writing the code for PartialOrd.

---

I wish there weren't so many compilation errors at first, but I thought of no good solution to this problem other than simply not compiling the tests.
Adding more stubs would kind of defeat the point of the exercise.

I'm wondering if test of multiplying with a negative number should be added. Maybe also multiply by zero.

I would probably ask to #[ignore] all tests except the first since all other exercises do that, but it doesn't solve the problem of many compilation errors

[petertseng]

decimal here twice - this implies that it should be removed from description.md, the generator automatically inserts one

[coriolinus]

Good catch; I'll fix that

[petertseng]

Is this too big an exercise? Sure, common math operations aren't difficult to implement, but then we're asking people to implement six different traits.

Does it help if I say this exercise took me 170 minutes to do? Although the first 40 minutes were a doomed attempt to get this to work without BigInt that I scrapped as soon as I saw it was doomed.

I know, I know, I didn't actually answer the question.

I consider my time well spent, and particularly think that keeping both Add and Sub will encourage students to think about code reuse or expressing one in terms of the other.

I hesitate to recommend removing Mul since seeing it in action is interesting, but by the time the other traits have been implemented we've already met the goal of "teach the student how to implement traits". I don't think I want to remove anything right now, but if I did, I would remove Mul.

In a similar vein, if I had to remove more, I would remove PartialOrd, but I guess that goes against the idea in #347 (comment) of having an exercise about implementing PartialOrd. Is the goal PartialOrd in specific, or traits in general?

[petertseng]

My solution is at https://github.com/petertseng/exercism-rust/tree/decimal but I don't know if this type of solution will be common enough to warrant any test cases that will catch common mistakes of implementors of this type of solution.

The important one would be ensuring that 1.0 < 1.1 or something similar.

[petertseng]

For the type of solution seen there, also testing that 0.1 + 0.02 == 0.12 prevents a mistake that I personally made.

[petertseng]

After fixing bugs, I conclude that the way I did is too bug-prone. I don't recommend the way I did it.

[petertseng]

Yeah my way doesn't work at all, comparisons involving negatives are broken. What was I thinking, really.

[petertseng]

there, fixed it

[coriolinus]

Does it help if I say this exercise took me 170 minutes to do? Although the first 40 minutes were a doomed attempt to get this to work without BigInt that I scrapped as soon as I saw it was doomed.

Three hours feels a bit long to me, but within the expected time for a meaty kind of exercise. There have been other Exercism exercises which took me that long or longer, though admittedly they were challenge 10 ones.

Do I understand correctly that because there is no difference in behaviour, we can only require them by making it a compile-time error to not have them? Doesn't seem like a particularly useful test, then.

There's no real difference between PartialEq and Eq; Eq just a marker trait which may be expanded on in the future. There is, however, a fairly significant difference between PartialOrd and Ord: only the latter works with Vec::sort and related methods. PartialOrd types require users to jump through the rather verbose my_vec.sort_by(|(a, b)| a.partial_cmp(b).unwrap_or(...)) hoop in order to sort their vectors.

In a similar vein, if I had to remove more, I would remove PartialOrd, but I guess that goes against the idea in #347 (comment) of having an exercise about implementing PartialOrd. Is the goal PartialOrd in specific, or traits in general?

AFAIK the intent of that comment was that we should focus on implementing traits in general, not PartialOrd in particular.

For my own implementation, I write two little macro_rules! macros, then expressed every required trait in terms of one or the other of them. Each additional trait implementation took exactly one additional line of code, so I don't feel that the implementation burden of each trait was excessive. I'd expect people comfortable in Rust to do something similar, as the trait implementations are intentionally very similar to each other, and amenable to this sort of solution. Do you think that I should explicitly hint that in the readme?

---

I liked the general ideas of the suggested additional tests, so I added all of them. More tests never hurt. Reference implementation still passes everything. I also added #[ignore] attributes to all tests except the first, to better fit the pattern. You still need at least unimplemented!() implementations of all the traits before the tests compile at all, but that kind of drives home the point of this exercise.

[petertseng]

Each additional trait implementation took exactly one additional line of code, so I don't feel that the implementation burden of each trait was excessive.
Do you think that I should explicitly hint that in the readme?

A little unsure. I have an unsubstantiated feeling that by the time students reach this exercise, there's already been enough of the idea of "reuse code when possible", such as in https://github.com/exercism/rust/blob/master/exercises/space-age/tests/space-age.rs . The fact that students have to write unimplemented!() for each trait ensures that students are aware of all they have to implement... and hopefully this drives students to think about "So I know I'm eventually going to have to do this work. How should I arrange it so that I can do as little work as possible?"

I don't think it is necessary. If, after this exercise is merged and submissions come in that look like they can benefit from the hint, add it then.

The strength of my opinion is 2/10. If you disagree with any strength >= 2 and want to add it now, I do not complain.

I liked the general ideas of the suggested additional tests, so I added all of them

Optional:
You may see all tests I used at petertseng@ddb8d67 and petertseng@6a77ec1. You are free to add any of those tests that you see fit.

However, note that a lot of the tests are biased toward solutions where integral and fractional part are stored separately. You may not find all of them useful to add.

Of those tests, the ones that might be most interesting to add are additive identity, subtractive identity, multiplicative identity.

---

After your decision on whether to add the hint and any additional tests, I think we will have said all we need to say, so it would be time to merge then.

[petertseng]

any specific reason it needs to be 1.8.0?

[coriolinus]

No; I'll replace that with a link to the general-purpose book.

[petertseng]

what about linking to the second edition? #356 is discussing it, and the corresponding page would be https://doc.rust-lang.org/book/second-edition/ch10-02-traits.html it seems.

[coriolinus]

As of right now, I can't find a section in the second edition which talks specifically about deriving traits, which is what the hint addresses. For now, the first edition's link speaks more specifically about what I want to talk about.

[petertseng]

Note to self: Yes, I did check that the README is exactly as generated.

[coriolinus]

@petertseng I've updated the README's docs link and added the bulk of the additional tests you suggested. Assuming travis is happy with the new commits, I think this is ready to merge.

---

WRT the macros, I agree with you that we shouldn't hint the use of macros, though my reasoning is slightly different: right now macros are simply out of scope for this exercise. People can use them or not per their own familiarity with Rust, but the tests don't care one way or the other. However, if we were to hint that their use, people would tend to feel that the only "correct" solutions were those that used them; it would bring them into scope. The exercise is better when it's about trait implementation, not about traits and macros.

That said, having an exercise which is about macros would probably be a good thing. After this is merged, I might be able to dedicate some time to writing such an exercise, though I can't promise it because my work schedule is in flux at the moment. Do you have any ideas right now about how we might structure such a thing? We'd want a task which is plausible but repetitive. My first instinct is to have users implement some trait on generic array types, but RFC 2000 was recently accepted, which means that the exercise would go stale as soon as the implementation makes it into stable Rust.

[petertseng]

I agree with you that we shouldn't hint the use of macros

My agreement had the intended effect (the hint was not added), but had an unintended interpretation.

The hint that I advised against giving would not have mentioned macros at all. It simply would have said something along the lines of "You should strive to reuse as much code as possible".

That said, having an exercise which is about macros would probably be a good thing.
Do you have any ideas right now about how we might structure such a thing?

One time I felt the need to use macros was on an old version of the space age exercise, before they were merged in #177. The tests as they were back then can be seen in https://github.com/IanWhitney/xrust/blob/16cc6b04c2e68f69559174faa1b30bdec2797893/exercises/space-age/tests/space-age.rs, and in short they expected MercuryAge::from(1234).years(), VenusAge::from(1234).years(), etc. I threw together a solution using macros (https://gist.github.com/petertseng/0b3928f0ee8881e94bf654d3f2153df9) because I was not pleased at having to define each of MercuryAge, VenusAge, etc.

I don't know if that gives any ideas, but it's the only thing remotely resembling an idea I have right now.

[petertseng]

I'm ready. I hope I don't overstep my bounds by adding those two commits. I am sure the alwaye -> always is right, but I guess I could have missed something in that trailing comma; can you confirm whether the trailing comma is necessary? I figure if tests still pass without it, then it should be fine, right?

[coriolinus]

Right, that's fine. Thanks for catching the typo!

Regarding trailing commas, Rust permits them, and in multi-line form for array or Vec literals encourages them. However, in that case, it was in single line form, so you were right to remove it. I appreciate it; it's good to keep the code looking as clean as possible before it goes public.

[petertseng]

Nice. Thanks for the work!