Clojure

New qualified-* predicates can return true, nil, and false

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: Release 1.9
  • Fix Version/s: Release 1.9
  • Component/s: None
  • Labels:
  • Patch:
    Code
  • Approval:
    Ok

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

=> (qualified-keyword? ::abc)
true
=> (qualified-keyword? :abc)
nil
=> (qualified-keyword? 'abc)
false

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

  1. clj-2141.patch
    03/Apr/17 12:19 PM
    1 kB
    Alex Miller
  2. clj-2141-2.patch
    12/May/17 9:41 PM
    1 kB
    Stuart Halloway
  3. clj-2141-3.patch
    26/May/17 8:21 AM
    3 kB
    Alex Miller

Activity

Hide
Didier A. added a comment -

Agreed, either make it truthy/falsey and drop the ?, or keep the convention, one of the fee ones that still does not have an exception to its rule.

Show
Didier A. added a comment - Agreed, either make it truthy/falsey and drop the ?, or keep the convention, one of the fee ones that still does not have an exception to its rule.
Hide
Alex Miller added a comment -

Moving to vetted so we remember to look at this before 1.9 releases.

Show
Alex Miller added a comment - Moving to vetted so we remember to look at this before 1.9 releases.
Hide
Nikita Prokopov added a comment - - edited

Just wanted to add my 2cents:

  • Even if nobody ever promised explicitly that predicates strictly return true/false, it has become an established convention, people rely on it.
  • Performance benefits are debatable at best. The function itself might become slightly faster but the coercion to boolean is still happening at the call site (e.g. inside if/when condition).
  • I think problem is important enough that it must get at least a second look. If there’s no good reason to break the convention, it’ll be better for everybody if we keep following it.
  • Having predicates that return three possible values is, well, weird by all means.
  • Even worse is that they look exactly like the old predicates. There’s no way to tell. It adds nuance and complexity to otherwise pretty simple and straightforward part of Clojure

Sorry but I really think this was overlooked. I only wish best for Clojure ind its future. Please at least consider this.

Thanks!

Nikita.

Show
Nikita Prokopov added a comment - - edited Just wanted to add my 2cents:
  • Even if nobody ever promised explicitly that predicates strictly return true/false, it has become an established convention, people rely on it.
  • Performance benefits are debatable at best. The function itself might become slightly faster but the coercion to boolean is still happening at the call site (e.g. inside if/when condition).
  • I think problem is important enough that it must get at least a second look. If there’s no good reason to break the convention, it’ll be better for everybody if we keep following it.
  • Having predicates that return three possible values is, well, weird by all means.
  • Even worse is that they look exactly like the old predicates. There’s no way to tell. It adds nuance and complexity to otherwise pretty simple and straightforward part of Clojure
Sorry but I really think this was overlooked. I only wish best for Clojure ind its future. Please at least consider this. Thanks! Nikita.
Hide
Ertuğrul Çetin added a comment -

Totally agreed!

Show
Ertuğrul Çetin added a comment - Totally agreed!
Hide
Murat Berk added a comment -

It should NOT violate a common expectation/convention.

Show
Murat Berk added a comment - It should NOT violate a common expectation/convention.
Hide
Bozhidar Batsov added a comment -

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.

Show
Bozhidar Batsov added a comment - 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.
Hide
Alex Miller added a comment -

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

Show
Alex Miller added a comment - 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).
Hide
Bozhidar Batsov added a comment -

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.

Show
Bozhidar Batsov added a comment - 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.
Hide
Chris Oakman added a comment -

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

Show
Chris Oakman added a comment - Just adding a note to say that I agree with Nikita Prokopov's comments above.
Hide
Alex Miller added a comment -

This change is included in 1.9.0-alpha17.

Show
Alex Miller added a comment - This change is included in 1.9.0-alpha17.

People

Vote (91)
Watch (29)

Dates

  • Created:
    Updated:
    Resolved: