<< Back to previous view

[CLJ-2141] New qualified-* predicates can return true, nil, and false Created: 31/Mar/17  Updated: 26/May/17  Resolved: 26/May/17

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.9
Fix Version/s: Release 1.9

Type: Defect Priority: Major
Reporter: Daniel Compton Assignee: Unassigned
Resolution: Completed Votes: 91
Labels: function

Attachments: Text File clj-2141-2.patch     Text File clj-2141-3.patch     Text File clj-2141.patch    
Patch: Code
Approval: Ok


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)
=> (qualified-keyword? :abc)
=> (qualified-keyword? 'abc)

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


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

Comment by Didier A. [ 03/Apr/17 5:14 AM ]

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.

Comment by Alex Miller [ 03/Apr/17 12:19 PM ]

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

Comment by Nikita Prokopov [ 04/Apr/17 2:38 AM ]

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.



Comment by Ertuğrul Çetin [ 04/Apr/17 6:25 AM ]

Totally agreed!

Comment by Murat Berk [ 05/Apr/17 9:38 AM ]

It should NOT violate a common expectation/convention.

Comment by Bozhidar Batsov [ 06/Apr/17 1: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.

Comment by Alex Miller [ 07/Apr/17 3: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).

Comment by Bozhidar Batsov [ 11/Apr/17 2: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.

Comment by Chris Oakman [ 26/May/17 1:12 PM ]

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

Comment by Alex Miller [ 26/May/17 3:28 PM ]

This change is included in 1.9.0-alpha17.

[CLJ-1925] Add uuid and random-uuid functions Created: 10/May/16  Updated: 15/May/17

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Feature Priority: Minor
Reporter: Daniel Compton Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: function


ClojureScript has uuid and random-uuid functions. These are handy to have in ClojureScript, and I think would be useful also in Clojure to improve code portability. Is there interest in a patch for this?

Comment by Alex Miller [ 10/May/16 8:00 AM ]

I think the main reason to do this would be portability. It would make most sense to generate java.util.UUIDs - is that harmonious with what is being done in ClojureScript? That is, could the same code for creating and using uuids work on both platforms? If not, then there might not be a good reason to do so.

Comment by Daniel Compton [ 10/May/16 3:45 PM ]

> It would make most sense to generate java.util.UUIDs - is that harmonious with what is being done in ClojureScript?

ClojureScript defines it's own UUID type, as one doesn't exist in JavaScript. https://github.com/clojure/clojurescript/blob/dd589037f242b4eaace113ffa28ab7b3791caf47/src/main/cljs/cljs/core.cljs#L10088-L10128. I'm not quite sure what you mean by harmonious.

> That is, could the same code for creating and using uuids work on both platforms?

The CLJS UUID doesn't support all of the methods of the Java UUID, but the important things are there (equivalence, constructing from a string, printing to a string) and they would be enough to significantly improve portability when working with UUID's.

Comment by Nicola Mometto [ 10/May/16 4:27 PM ]

both clojure and clojurescript have uuid tagged literals, that should be good enough for interop

Comment by Alex Miller [ 11/May/16 2:48 PM ]

I'm aware of that, just wondering if there are any functions you might invoke on a uuid that would need some portable equivalent, like the stuff in http://docs.oracle.com/javase/8/docs/api/java/util/UUID.html.

Comment by Daniel Compton [ 11/May/16 3:27 PM ]

Most of the extra methods here are useful for distinguishing between multiple types of UUID's, or getting information out of time based UUIDs.

clockSequence() - time based
compareTo(UUID val) - not sure if equivalent required?
boolean	equals(Object obj) - no action required
static UUID	fromString(String name) - constructor
long	getLeastSignificantBits() - not sure how important these two are
long	getMostSignificantBits()
int	hashCode() - no action required
static UUID	nameUUIDFromBytes(byte[] name) - is this useful/important?
long	node() - only useful for time UUID
static UUID	randomUUID() - would implement this
long	timestamp() - time based UUID
String	toString() - no action required
int	variant() - for distinguishing between different types of UUID's
int	version() - for distinguishing between different versions of UUID's

I could potentially see an argument for time based UUID's being included in a patch here too, but I'm not sure if they are used enough to be worth it, and they'd need to go into CLJS, e.t.c.

There is use of some of these methods in Clojure code:

But less than the literal constructor by a factor of ~100:
https://github.com/search?utf8=✓&q=java+util+UUID+language%3Aclojure&type=Code&ref=searchresults (this is a flawed search query, but the best I could do).

Comment by Alex Miller [ 11/May/16 3:56 PM ]

I guess my greater point is: rather than consider just the functions uuid/random-uuid, let's consider the problem to be: how can we add portable uuid support in Clojure/ClojureScript? That's a lot more work, but a lot more valuable in my opinion.

So would also want to consider (some of these exist already, but may not have been tested for portability):

  • construction
  • printing - print, pr, pretty print
  • reading
  • hash code
  • conversion to/from bits
  • conversion to/from string
  • extraction of components

And then I think it's worth considering how much of this should be in core vs in a data.uuid or something.

I think it's probably better to work it off a design page than here (this ticket is but one unit of the greater problem). Perhaps http://dev.clojure.org/pages/viewpage.action?pageId=950382 could suggest some pointers.

[CLJ-1693] into: merge metadata Created: 03/Apr/15  Updated: 03/Apr/15

Status: Open
Project: Clojure
Component/s: None
Affects Version/s: Release 1.7
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Gregg Reynolds Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: function, interop


Approval: Triaged


Currently (into to from) preserves to's metadata but discards from's metadata. The enhancement would be to have 'into' do something like (merge (meta to) (meta from)). Justification: as with data, so with metadata. Use case: Using deftype, I have a class EntityMap that clojurizes a native Java class (App Engine's Entity class), making it behave just like a clojure map. This includes using into to convert an EntityMap to an ordinary PersistentMap; the problem is that key information for the EntityMap is really metadata, so I need (into {} em) to put that metadata into the new PersistentMap.

See also CLJ-916

[CLJ-1594] Colons followed by spaces are not ignored within (comment ...) Created: 19/Nov/14  Updated: 22/Nov/14  Resolved: 22/Nov/14

Status: Closed
Project: Clojure
Component/s: None
Affects Version/s: Release 1.6
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Ed Ward Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: bug, comment, function

Ubuntu Linux 14.10
Clojure 1.6.0
OpenJDK 64-Bit Server VM 1.7.0_65-b32



(comment abc:def)
works, while running
(comment abc: def)
(comment abc : def)
fail with the exception

RuntimeException Invalid token: :  clojure.lang.Util.runtimeException (Util.java:221)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: def in this context, compiling:(/tmp/form-init1585368677683647130.clj:1:1010) 
RuntimeException Unmatched delimiter: )  clojure.lang.Util.runtimeException (Util.java:221)

Comment by Nicola Mometto [ 19/Nov/14 11:19 AM ]

`comment` is a macro, it doesn't bypess the reader.
":" is not a regular clojure token so it's expected that this throws.
The only way to include non clojure tokens in a source code is after a ";" comment

Comment by Andy Fingerhut [ 19/Nov/14 3:37 PM ]

I've just added some notes about this to ClojureDocs.org here: http://clojuredocs.org/clojure.core/comment

Comment by Alex Miller [ 22/Nov/14 9:12 AM ]

agreed with Nicola's comment above

Generated at Fri Jul 21 06:06:04 CDT 2017 using JIRA 4.4#649-r158309.