ClojureScript

Empty regexp causes Closure Compiler error

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Trivial Trivial
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None
  • Patch:
    Code and Test

Description

An empty regexp of the form #"" compiles to //, which is not an empty regexp but a comment, causing the code following the supposed regexp on the affected line to be commented out. This tends to cause compiler errors when Closure Compiler tries to parse it.

#"" should instead produce either /(?:)/ or new RegExp("").

Activity

Hide
Chas Emerick added a comment - - edited

Added updated patch (471-2.diff) to account for the differences in how empty regexes print in different JS engines. I don't think there's any way to cleanly get away from this, unless we want to s/"(?"/""/g when printing regexes...but that'd be a different ticket, and perhaps not worth the trouble?

Michał, git simply refused to apply your prior patch (didn't even produce conflict changes?), so I just recreated it with the necessary tweak. This eliminated any attribution for you. Sorry about that; if anyone knows of a way to fix that, do let me know.

Show
Chas Emerick added a comment - - edited Added updated patch (471-2.diff) to account for the differences in how empty regexes print in different JS engines. I don't think there's any way to cleanly get away from this, unless we want to s/"(?"/""/g when printing regexes...but that'd be a different ticket, and perhaps not worth the trouble? Michał, git simply refused to apply your prior patch (didn't even produce conflict changes?), so I just recreated it with the necessary tweak. This eliminated any attribution for you. Sorry about that; if anyone knows of a way to fix that, do let me know.
Hide
Michał Marczyk added a comment -

Thanks for letting me know. I've checked the value of new RegExp("").source in a Node.js REPL (0.10.3) and it turns out to be '(?'. So, how important is it that we return "#\"\"" rather than "#\"(?\"" anyway? I'm thinking maybe not that much, seeing how regexp support in JS is different to that on the JVM in more fundamental ways. I'll just assume this is correct and attach a new patch accepting both representations soon. Of course if there's a better way to do it, I'll be happy to implement it.

Show
Michał Marczyk added a comment - Thanks for letting me know. I've checked the value of new RegExp("").source in a Node.js REPL (0.10.3) and it turns out to be '(?'. So, how important is it that we return "#\"\"" rather than "#\"(?\"" anyway? I'm thinking maybe not that much, seeing how regexp support in JS is different to that on the JVM in more fundamental ways. I'll just assume this is correct and attach a new patch accepting both representations soon. Of course if there's a better way to do it, I'll be happy to implement it.
Hide
David Nolen added a comment -

I tried applying this in master, the included test fails.

Show
David Nolen added a comment - I tried applying this in master, the included test fails.
Hide
Michał Marczyk added a comment -

Commit message:

CLJS-471: prevent empty regexps from causing compiler errors

This patch chooses to emit

  (new RegExp(""))

rather than

  /(?:)/

so that (pr-str #"") returns

  "#\"\""

rather than

  "#\"(?:)\""

A test for the above is included.
Show
Michał Marczyk added a comment - Commit message:
CLJS-471: prevent empty regexps from causing compiler errors

This patch chooses to emit

  (new RegExp(""))

rather than

  /(?:)/

so that (pr-str #"") returns

  "#\"\""

rather than

  "#\"(?:)\""

A test for the above is included.

People

Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: