Add constraint to parser for empty :test conditions

[Zylox]

• Previously an empty test condition (aka [:test]) was valid to the
  parse and would generate pointless beta nodes. This change throws an
  exception when parsing that.

This is something we noticed when doing an internal code review for something else and it seemed like an easy enough fix. I can't think of a situation in which we would want an empty test condition. Additionally, it looks like the parser would have intercepted it had[:test] been intended as a type anyway, so i dont think this changes anything functionally there.

[mrrodriguez]

I discussed this with @Zylox previously, but the only comment I had here was that this only prevents the DSL from making rules with empty :test conditions. You could still builds rules with empty :test conditions by other means that target the underlying data model of rules.

However, this seems appropriate. Preventing it in the DSL is likely to be helpful to just avoid rules that were written incorrectly. Other sources of rules (not from the DSL) may have other reasons why they could end up generating empty :test conditions and perhaps it isn't a good idea to break those cases unnecessarily.

Also, I'm not sure it is mentioned in this PR, but an empty :test condition always evaluates truthy since it results in a condition of (and) which is always true.

[WilliamParker]

Is there a particular reason why you didn't use assert-ex-data here or were you just not aware of it? It has the advantages of:

• Using normal Clojure data equality semantics rather than messy string comparisons.
• Not failing if something else wraps the exception later since it walks the exception chain to look for a match.

Side note: Clojure has an IExceptionInfo interface, of which clojure.lang.ExceptionInfo is a concrete implementation. Any instance checks you do should generally use IExceptionInfo for all the usual reasons to favor coding against an interface rather than the implementation; in this particular case my understanding is that user programs might define implementations of IExceptionInfo themselves. The ex-data function just returns nil if the exception is of the wrong type so that can be used as well in many cases in logic like

(when (ex-data e) ...

[Zylox]

I wasnt aware of it. I copied a test elsewhere in this file and modified it for my needs. I'll work it out to use this.

[WilliamParker]

I think this might be clearer with a colon in front of the "test" to denote that it is a keyword, i.e.

"Empty :test conditions are not allowed."

I realize that the symbol "test" is allowed as well, but I suspect that :test is the more common usage since that is what the docs use.

Thoughts?

[Zylox]

Ya good point

[WilliamParker]

I propose fixing this by changing the fact type inserted here to something other than :test rather than eliminating the LHS of the rule. My reasoning is that a test for shadowing seems a bit stronger if we force the evaluation to wait until the insert call is made (and not at any time after the call to create the session). Not a big deal, but it is also an easy change.

[Zylox]

can do

[WilliamParker]

Throw-dsl-ex won't include useful info here like what the offending rule or query is, but that looks like a shortcoming of the current usage of throw-dsl-ex in this namespace that impacts other malformed productions as well, so I think changing that is out of scope here. I'll log another issue.

[Zylox]

For reference the issue logged is #351

[WilliamParker]

I made some comments on the changes to the test namespaces, but fundamentally I agree that this is a desirable change.