clojush.util/top-item does not check for nil argument

[Vaguery]

(Noted while Tom pointed out several instructions do not return push-state objects.)

Failing Midje test:

;; sanity check

(fact "the top-item utility function should not return :no-stack-item when passed a nil argument"
  (top-item :integer nil) =not=> :no-stack-item
  (top-item :foo (make-push-state)) =not=> :no-stack-item
  ) 

Expected behaviors:

• Raise an error when the indicated stack is not present in the keys of the map argument
• Raise an error when the push state has no keys

[thelmuth]

While I agree, your second test should have parentheses around make-push-state, no?

You need some tests for your tests :stuck_out_tongue_winking_eye:

[lspector]

I don't even think I agree.

My first reaction was that neither nil nor make-push-state is a push state (although (make-push-state) is), so you shouldn't be calling top-item on them and if you do then all bets are off. We could add type checks here (and in hundreds of other places), but unless we're going down that road there's no reason to think that top-item should behave reasonably when given inappropriate arguments.

But I disagree even if we replace the examples with (top-item :integer (make-push-state)) or (top-item :integer {}), or these with :foo instead of :integer. What is the top item of the named stack in those push states? There isn't one. So I think that :no-stack-item is a fine answer.

[Vaguery]

I understand your point, and agree that's its in keeping with the codebase's practices to date. I'm trying to improve the codebase for people who aren't as familiar with its history as we all are.

It is both helpful and sufficient to say that (top-item :integer (make-push-state)) should produce :no-stack-item (rather than nil, which I assume it replaces). It's pretty clear that the presence of that little explicit symbol means somebody somewhere was wrestling with a nil result and thought it was helpful to say something more informative.

But I'd like to also help prospective users, not surprise them, and so I don't think it's equally correct to insist that (top-item :enumerator (make-push-state)) should also return :no-stack-item if I have not correctly added that stack to the global directory. The only helpful responses I can think of are either (1) raising an exception or (2) returning a new :no-such-stack response if the key is not found in the argument.

I'd like to think small changes like this one will make it easier for users who want to work with the code to solve domain problems, and like several other key functions top-item is intentionally surfaced for the user to invoke in their error functions. As such, it should be rock solid and as close to transparent as possible, or it should be a private function for use inside the codebase, and a new "safe" public function should be used in error functions instead of top-item.

[lspector]

I agree that :no-such-stack would be a more helpful reply if there really is no such stack; that strikes me as a good change to make.

On the idea of raising an exception, I generally shy away from this and prefer some sort of silent failure for calls that are expected to occur during fitness evaluation, since random code will often be "crazy" in ways that we don't want to know about, and don't care about, and wouldn't want to stop a run for. I realize that this isn't a position that should be taken too generally, since it will sometimes be important for us to be notified that assumptions are being violated. But FWIW that's why my bias is generally against exceptions being raised in Push instructions or in other things (like top-item) that are called in error functions.

FYI the reason for a non-nil return value in the first place is that it's possible for nil to be a valid value on some stacks, at least the code and exec stacks. So if we returned nil for an empty stack then if you asked for the top-item of the code stack and got nil, you wouldn't know if there was a nil item or no item at all. :no-stack-item resolves this ambiguity, and it seemed clearest to return this for the case of an empty stack regardless of whether there could really be valid nils on the stack in question.