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

None

Activity

Show:
Bozhidar Batsov
April 6, 2017, 7:50 AM

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.

Alex Miller
April 7, 2017, 9:29 PM

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).

Bozhidar Batsov
April 11, 2017, 8:22 AM

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.

import
May 26, 2017, 7:12 PM

Comment made by: chrisoakman

Just adding a note to say that I agree with Nikita Prokopov's comments above.

Alex Miller
May 26, 2017, 9:28 PM

This change is included in 1.9.0-alpha17.

Completed

Assignee

Unassigned

Reporter

Daniel Compton

Labels

Approval

Ok

Patch

Code

Fix versions

Affects versions

Priority

Major
Configure