ClojureScript

re-matches returns [] if string is nil

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None
  • Environment:
    clojurescript 0.0-2227

Description

(re-matches #"no-match" "") => nil
(re-matches #"no-match" nil) => []

In clojure calling re-matches on nil throws an exception, but in clojurescript it returns [].

  1. cljs-810.patch
    01/Jul/14 11:27 PM
    3 kB
    Francis Avila
  2. cljs-810-nil.patch
    12/Jun/14 2:51 AM
    3 kB
    Francis Avila
  3. cljs-810-throw.patch
    24/Jun/14 10:16 AM
    3 kB
    Francis Avila

Activity

Hide
David Nolen added a comment -

this is a platform distinction, otherwise we need to put checks for nil and throw exceptions in several places.

Show
David Nolen added a comment - this is a platform distinction, otherwise we need to put checks for nil and throw exceptions in several places.
Hide
Stephen Nelson added a comment -

I understand the need for efficiency and recognise that it is reasonable to have differences between platforms, but I don't think it's reasonable to return truthy when given null – the regex did not match, the function should return falsey.

I think the performance implication can be avoided using a case switch on the result of the existing length test:

Replace:

(if (== (count matches) 1)
(first matches)
(vec matches))

With:

(case (count matches)
0 nil
1 (first matches)
(vec matches))

Show
Stephen Nelson added a comment - I understand the need for efficiency and recognise that it is reasonable to have differences between platforms, but I don't think it's reasonable to return truthy when given null – the regex did not match, the function should return falsey. I think the performance implication can be avoided using a case switch on the result of the existing length test: Replace: (if (== (count matches) 1) (first matches) (vec matches)) With: (case (count matches) 0 nil 1 (first matches) (vec matches))
Hide
Stephen Nelson added a comment -

Just to note, this is not a hypothetical problem, we encountered a bug in production where a 'local url' regex test passed when it was unexpectedly given nil. This bug was hard to track down because it's not obvious even from reading the implementation that the function could return truthy when the regex didn't match.

Show
Stephen Nelson added a comment - Just to note, this is not a hypothetical problem, we encountered a bug in production where a 'local url' regex test passed when it was unexpectedly given nil. This bug was hard to track down because it's not obvious even from reading the implementation that the function could return truthy when the regex didn't match.
Hide
David Nolen added a comment -

Oh, I didn't realize that re-matches differs from re-find in this regard. Is there any reason that re-matches can't follow re-find? If it can, yes, patch welcome.

Show
David Nolen added a comment - Oh, I didn't realize that re-matches differs from re-find in this regard. Is there any reason that re-matches can't follow re-find? If it can, yes, patch welcome.
Hide
Francis Avila added a comment -

`re-find` and `re-matches` are both completely broken for nil because of js coersion of nil to the string "null". Your suggested fix (using a case statement) is not radical enough to solve this.

(re-find #"." nil)
; => "n"
(re-matches #"." nil)
; => nil

Note that `re-matches` itself doesn't work as you might expect: CLJS-776. You should use re-find with an explicit `^` and `$` instead.

I think both re-find and re-matches should have an explicit nil check on their string argument.

Show
Francis Avila added a comment - `re-find` and `re-matches` are both completely broken for nil because of js coersion of nil to the string "null". Your suggested fix (using a case statement) is not radical enough to solve this.
(re-find #"." nil)
; => "n"
(re-matches #"." nil)
; => nil
Note that `re-matches` itself doesn't work as you might expect: CLJS-776. You should use re-find with an explicit `^` and `$` instead. I think both re-find and re-matches should have an explicit nil check on their string argument.
Hide
Francis Avila added a comment - - edited

The fundamental issue here is that RegExp.exec(x) in javascript will x.toString() its argument before applying the regex. This leads to surprising behavior for anything which is not a string.

Options are:

  1. Call it a platform difference and leave current behavior as-is.
  2. Return nil if argument is not a string (unlike clojure, which throws).
  3. Throw if argument is not a string (like clojure and re-seq in cljs).

Attached patch takes option 2 and does nothing about re-seq (which currently throws). This is inconsistent with clojure but nil-punning is convenient. Option 3 seems equally valid, though.

Show
Francis Avila added a comment - - edited The fundamental issue here is that RegExp.exec(x) in javascript will x.toString() its argument before applying the regex. This leads to surprising behavior for anything which is not a string. Options are:
  1. Call it a platform difference and leave current behavior as-is.
  2. Return nil if argument is not a string (unlike clojure, which throws).
  3. Throw if argument is not a string (like clojure and re-seq in cljs).
Attached patch takes option 2 and does nothing about re-seq (which currently throws). This is inconsistent with clojure but nil-punning is convenient. Option 3 seems equally valid, though.
Hide
David Nolen added a comment -

I think I prefer option 3 if it aligns us with Clojure, thanks!

Show
David Nolen added a comment - I think I prefer option 3 if it aligns us with Clojure, thanks!
Hide
Francis Avila added a comment -

Added patch which throws TypeError instead of nil for non-string.

Show
Francis Avila added a comment - Added patch which throws TypeError instead of nil for non-string.
Hide
David Nolen added a comment -

Can we rebase this patch to master? Thanks!

Show
David Nolen added a comment - Can we rebase this patch to master? Thanks!
Hide
Francis Avila added a comment -

Rebased to master.

Show
Francis Avila added a comment - Rebased to master.

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: