New qualified-* predicates can return true, nil, and false
Description
As of Clojure 1.9-alpha15, the new qualifed-keyword?, qualified-symbol?, and qualified-ident? functions can return true, nil, or false depending on the input passed to them, e.g.:
Returning nil rather than false for unqualified keywords has the following issues:
Many sources of Clojure documentation note that a trailing '?' in a Clojure function generally implies a true/false return so this violates a common expectation.
Returning true/false/nil complicates the ability of ClojureScript to optimize boolean returns. Theoretically, this might affect Clojure JIT optimization as well, turning a call taking the return value into polymorphic rather than bimorphic.
Returning true/false/nil can yield confusing results for functions that partition by value, like group-by
The prior implementation during development used `boolean` to coerce the return value to a null and that was removed for performance reasons (would require a call through another var).
Alternatives:
Perf benchmark (w/ criterium bench, Java 1.8, AOT, direct-linked)
Example: (bench (dotimes [_ 100] (qualified-keyword? :a)))
Impl | :a | :a/b | note |
|---|---|---|---|
(and (keyword? x) (namespace x) true) | 96.749127 ns | 96.412551 ns | current impl (returns nil for :a) |
(and (keyword? x) (some? (namespace x))) | 99.014677 ns | 96.149567 ns | uses existing preds |
(and (keyword? x) (not (nil? (namespace x)))) | 97.512916 ns | 95.008061 ns | nil? inlines |
(if (and (keyword? x) (namespace x)) true false) | 98.617369 ns | 97.866673 ns | if-based |
(if (keyword? x) (if (namespace x) true false) false) | 99.000727 ns | 99.474754 ns |
Note that these vary by less than 1 ns per invocation, so perf is not significantly different.
Approach: After discussion with Rich, have decided to use a boolean wrapper around the check: (boolean (and (keyword? x) (namespace x))).
Also, needed to move boolean implementation higher to colocate the qualified fns near the other fns.
Patch: clj-2141-3.patch
Environment
Activity
Btw, this is also an opportunity to improve the docstrings of those functions. Apart from the fact that for the millionth time they're missing a final `.`, now we can add something like `otherwise it returns false` or something like this. I assume this was originally omitted due the possibility to return either nil or false.
Re the docstrings, no plans to update those. The "otherwise..." was omitted because it's omitted on all of the other predicate docstrings as well. That makes sense (if the common implication is that the return value is false).
Perhaps. On a more general note - I really think some docstring standard should be imposed at some point. I find it odd that some docstrings are terminated with `.`, some are not. Not to mention it's really hard to figure out in docstrings what's a reference to a parameter, what's a reference to some other var, etc. And inconsistent docstrings are hard to format/present - e.g. in Elisp and CL it's common to have the first line of the docstring as a separate sentence, so you could have a good one-line overview of the thing it describes. Anyways, that's completely off-topic. One day I might blog/speak about this.
Comment made by: chrisoakman
Just adding a note to say that I agree with Nikita Prokopov's comments above.
This change is included in 1.9.0-alpha17.
Assignee
Reporter
Labels
Approval
Patch
Fix versions
Affects versions
Priority
