New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Crash when using an invalid regular expression #157

Closed
jonathanhefner opened this Issue Feb 3, 2015 · 4 comments

Comments

Projects
None yet
5 participants
@jonathanhefner
Contributor

jonathanhefner commented Feb 3, 2015

Issue for discussion thread https://groups.google.com/d/topic/elm-discuss/7Uwl5-usqjs/discussion

Basically, JavaScript's RegExp (which is what Elm's Regex sits on top of) can throw an exception when given an invalid string. Examples:

RegExp("a"); // OK
RegExp("\\"); // Exception!
RegExp("("); // Exception!
RegExp("(?"); // Exception!
RegExp("(?:"); // Exception!
RegExp("?"); // Exception!
RegExp("+"); // Exception!
RegExp("*"); // Exception!

The obvious fix is to make Elm's Regex.regex return a Result (or a Maybe), but that seems like a pain to work with. It's also an API-breaking change, which I have no problems with whatsoever, but others might.

The only other solution I can think of is to escape characters at the end of an input string until no exception is thrown. For example:

RegExp("(?:"); // Exception!
RegExp("(?\\:"); // Exception!
RegExp("(\\?\\:"); // Exception!
RegExp("\\(\\?\\:"); // OK

It's a bit hacky though, and may not follow the principle of least surprise. Thoughts?

@Dobiasd

This comment has been minimized.

Show comment
Hide comment
@Dobiasd

Dobiasd Feb 3, 2015

In my opinion the API-breaking change is the better solution. I prefer Result over Maybe, since the exceptions message can be seen then.

Dobiasd commented Feb 3, 2015

In my opinion the API-breaking change is the better solution. I prefer Result over Maybe, since the exceptions message can be seen then.

@salutis

This comment has been minimized.

Show comment
Hide comment
@salutis

salutis Feb 3, 2015

Contributor

+1 for API-breaking changes. The best thing about Elm is that it's opinionated.

Contributor

salutis commented Feb 3, 2015

+1 for API-breaking changes. The best thing about Elm is that it's opinionated.

@rtfeldman

This comment has been minimized.

Show comment
Hide comment
@rtfeldman

rtfeldman Aug 30, 2015

Member

Duplicate of #378, which has further discussion.

Member

rtfeldman commented Aug 30, 2015

Duplicate of #378, which has further discussion.

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Aug 30, 2015

Contributor

Okay, closing this here as a duplicate.

Contributor

jvoigtlaender commented Aug 30, 2015

Okay, closing this here as a duplicate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment