Unidiomatic API in forth

[Plecra]

rust/exercises/forth/src/lib.rs

Lines 19 to 21 in 2d163cf

	pub fn stack(&self) -> Vec<Value> {
	unimplemented!()
	}

Rust is designed to enable APIs to use the privacy of their implementation for soundness and stability. An interpreter's stack is rarely relevant to the public API, and (imo) wouldn't be found in a "good" Rust crate. It's an implementation detail.

I believe the issue is in the test structure: Since the state of the stack is an implementation detail, tests for its state are unit tests. Rust handles these using tests in the main crate:

pub struct Forth;

impl Forth {
    pub fn new() -> Forth {
        unimplemented!()
    }

    fn stack(&self) -> Vec<Value> {
        unimplemented!()
    }

    pub fn eval(&mut self, input: &str) -> ForthResult {
        unimplemented!("result of evaluating '{}'", input)
    }
}

#[test]
fn new_stack_is_empty() {
    assert_eq!(Forth::new().stack(), vec![]);
}

I'm also not a fan of this function signature in the exercise. It asks the user to return an owned vector from a reference to their interpreter. This only gives the user the option to clone whatever data structure they're using to represent their stack (which will typically just be self.stack.clone()).

As a "hard" task, can students attempting this exercise be expected to have a decent understanding of the language? If they know about existential types and iterators (covered by The Book), I think this API would be far less prescriptive:

fn stack(&self) -> impl Iterator<Item = Value>;

[coriolinus]

Fundamentally, you're right. Another approach we could take--since there's no plausible reason for an internal fn stack to exist--would be something like

#[cfg(test)]
pub fn stack(&self) -> &[Value] -> { ... }

However, we're fundamentally constrained by Exercism's design. Part of the interface contract that Exercism provides is that tests are external to student code: you can do anything you want within lib.rs, and as long as you don't mess with tests/forth.rs, the suite will still run properly (as long as you don't delete a function and cause a compilation failure). We're not going to convert the test suite here into unit tests.

You're also right that returning impl Iterator<Item=Value> (or <Item=&Value>) would allow for some more implementation flexibility. That said, I'm somewhat reluctant to go there; it's also unidiomatic design: it only makes sense when you don't know, or want to hide, what concrete implementation will be present; for any reasonable real Forth implementation, there will be some single data structure representing the stack, and fn stack should return the borrowed form of that.

I'd be inclined to respond positively to a PR which changed the interface either by changing the return value to &[Value], or by adding #[cfg(test)] to pub fn stack, or both. Depending on the argument, I might also be talked into supporting other modifications.

[PaulDance]

Hi @coriolinus!

I've tried to apply this change using #[cfg(test)] first, but it doesn't seem to work: cfg(test) is not set at this particular place when compiling the crate with cargo test handling things. Am I missing something here? It might be a bug, but I don't know for sure. A workaround would be to add a feature and use it for conditionally compiling stack like we wanted to, but it's not ideal.

Cheers,
Paul.

[coriolinus]

Doing a bit of digging, it appears that this is a known and long-standing issue.

My inclination at this point is to just drop the conditional compilation of fn stack. While it's true that a real library would either not have the function at all, or hide it behind a feature, there's not much point in using a feature here. For better or worse, our tests are integration tests, and therefore require the function to be public. We already have a pattern in this repo of using feature gates to partition optional "bonus objectives" that students aren't required to complete; using a feature gate here would break that pattern, because those tests aren't really optional. If we added a feature and then added the feature to the default-features set, then in the end it's just noise that students have to read past. That noise doesn't contribute to the students' education.

[PaulDance]

I quickly stumbled this issue as well, but avoided to post it here, as it seems slightly different to me from my understanding.

I agree with you on the general approach, I personally think exercises are not meant to be perfect and can afford leading students to think more about the idioms of the language and look deeper into its concepts. If I'm not wrong, I remember saying to myself when solving this exercise: "Oh okay, this surely is useless but is meant for the tests.". Obviously, it does not mean every student will necessarily come to this conclusion, but it means some probably will.

So I would suggest either @Plecra proposes a PR for a different approach or we close this as I don't feel like radically changing this exercise for a minor detail is a good idea.

[coriolinus]

I don't mind leaving this issue open, as it presented two proposals:

• feature-gate fn stack on the tests
• adjust the return type to &[Value] to reduce the amount of cloning required

The second of those is still a good first issue IMO. I just think that we can eliminate the first from serious consideration.