ClojureScript

clojure.string/replace ignores regex flags

Details

  • Type: Defect Defect
  • Status: Open Open
  • Priority: Minor Minor
  • Resolution: Unresolved
  • Affects Version/s: 1.7.145
  • Fix Version/s: Next
  • Component/s: None
  • Labels:
  • Patch:
    Code and Test

Description

The replace function in namespace clojure.string ignores regex flag provided in the match pattern. For example:

CLJS
clojure.string/replace "I am NOT matched" #"(?i)not " "")
=> "I am NOT matched"
CLJ
clojure.string/replace "I am NOT matched" #"(?i)not " "")
=> "I am matched"

The attached patch fixes this by parsing the m and i flags, if set, from the match object, instead of explicitly setting only "g".

Activity

Hide
Chas Emerick added a comment -

I can confirm the bug. The attached patch applies cleanly, and works as expected.

Esa, sorry for the long delay (this one must have slipped through the cracks)! Could you please submit a contributor's agreement, so that your patch can be merged? More info is here:

http://clojure.org/contributing

Show
Chas Emerick added a comment - I can confirm the bug. The attached patch applies cleanly, and works as expected. Esa, sorry for the long delay (this one must have slipped through the cracks)! Could you please submit a contributor's agreement, so that your patch can be merged? More info is here: http://clojure.org/contributing
Hide
lvh added a comment -

I got bit by this bug. Working on figuring out if I can sign that agreement.

Show
lvh added a comment - I got bit by this bug. Working on figuring out if I can sign that agreement.
Hide
lvh added a comment -

This is a duplicate of CLJS-794.

Show
lvh added a comment - This is a duplicate of CLJS-794.
Hide
Jake McCrary added a comment -

This patch changes string/replace-all to respect flags that were set on regexp passed as an argument.

I originally attached this to CLJS-794 and then noticed there was this older issue. I was unable to figure out how to edit at ticket to mark the patch as having "Code and Test" so I'm adding it to this issue instead.

I've signed a contributors agreement.

Show
Jake McCrary added a comment - This patch changes string/replace-all to respect flags that were set on regexp passed as an argument. I originally attached this to CLJS-794 and then noticed there was this older issue. I was unable to figure out how to edit at ticket to mark the patch as having "Code and Test" so I'm adding it to this issue instead. I've signed a contributors agreement.
Hide
Mike Fikes added a comment -
Show
Mike Fikes added a comment - There is a "sticky" flag y that could be conveyed. http://www.ecma-international.org/ecma-262/6.0/index.html#sec-get-regexp.prototype.sticky
Hide
Jake McCrary added a comment -

Reading a bit more about it and looks like both 'u' and 'y' are newly supported in ECMA6. Is there a way to write tests that exercise this functionality? I've got changes locally to get support of 'y' but felt the need to write a test for it (as its a bit more complicated than simply looking for a flag) and am hung up on having EMCA6 support while running the tests.

I'm actually wondering if having 'u' and 'y' in there is a bit premature. Any guidance on whether adding code for 'u' and 'y' should be done (or removing 'u' from my patch) or testing ClojureScript with ECMAScript 6 support?

Show
Jake McCrary added a comment - Reading a bit more about it and looks like both 'u' and 'y' are newly supported in ECMA6. Is there a way to write tests that exercise this functionality? I've got changes locally to get support of 'y' but felt the need to write a test for it (as its a bit more complicated than simply looking for a flag) and am hung up on having EMCA6 support while running the tests. I'm actually wondering if having 'u' and 'y' in there is a bit premature. Any guidance on whether adding code for 'u' and 'y' should be done (or removing 'u' from my patch) or testing ClojureScript with ECMAScript 6 support?
Hide
Mike Fikes added a comment -

One thought: Whatever this is exercising passes on recent versions of V8, JavaScriptCore, SpiderMonkey, Nashorn, and ChakraCore: https://github.com/clojure/clojurescript/blob/628d957f3ecabf8d26d57665abdef3dea765151e/src/test/cljs/cljs/core_test.cljs#L1472

It does seem tricky to write a robust RegExp clone implementation, and if you do some googling you see people dealing with u and y as special cases. The patch seemed OK to me with respect to this, but I'm not a JavaScript expert.

Show
Mike Fikes added a comment - One thought: Whatever this is exercising passes on recent versions of V8, JavaScriptCore, SpiderMonkey, Nashorn, and ChakraCore: https://github.com/clojure/clojurescript/blob/628d957f3ecabf8d26d57665abdef3dea765151e/src/test/cljs/cljs/core_test.cljs#L1472 It does seem tricky to write a robust RegExp clone implementation, and if you do some googling you see people dealing with u and y as special cases. The patch seemed OK to me with respect to this, but I'm not a JavaScript expert.

People

Vote (3)
Watch (5)

Dates

  • Created:
    Updated: