fix: ecma ranges with set terminator

[stevenh]

Fix ECMAScript un-escaped literal '-' when followed by predefined character sets.

Also:

• Fixed missing error check on parseProperty() call.
• Use addChar(ch) helper instead of addRange(ch, ch).

Fixes #54

[dlclark]

Hey, thanks for this. I have a few notes:

• I'd like to see tests for all the impacted shortcuts code paths, \d \s \w \p
• The inRange flag is not reset when we figure out we're not actually in a range, what are the implications of this? It seems like our parsing state would be messed up and could lead to bad parsing. [a-\s\b] should parse fine, but currently fails, I suspect related to this parsing state issue.
• For the behavior I did some research into javascript regex engines and it appears:
  • [\w-z] should fail to parse (currently doesn't fail)
  • The current regexp2 behavior matches the unicode option in a javascript engine. If we're changing this behavior we need to maintain the original compatibility with unicode option. This research opened my eyes to a whole class of bugs that likely exist in regexp2.
  • If you don't use the unicode option in a javascript then \p shouldn't be a special case at all! (it just means p)

[stevenh]

Nice catches, working on fixing these.

Can you confirmed the desired compatibility is it PCRE or PCRE2?

I ask as testoutput1 contains some patterns which are valid for PCRE but not PCRE2, namely:

• /[\d-z]+/
• /^[\d-a]/

I'm guessing PCRE2 as that's what .NET seems to match as per https://regex101.com/

[dlclark]

Desired base compatibility is the .NET engine.

[stevenh]

I think I've covered most the bases with this update, but definitely needs a second pair of eyes.

[stevenh]

@dlclark in case you missed the notification.

[dlclark]

Thanks for the reminder @stevenh!

In reviewing the commit I see several regex's that used to work but now return errors. I know the new behavior matches C#, but I don't want to break regex's that may be out in the wild already working. It's fine if new regex patterns suddenly stop throwing errors during compile, but the corpus of existing unit tests should still pass as-is.

I'm more willing to make breaking changes behind the ECMAScript flag since people use that option for compatibility with a standard.

Thoughts?

[stevenh]

That's what I thought, but testing some more it seems that regexp101 C# implementation actually has a bug and reports this as failure incorrectly as confirmed by this fiddler share

So I need to update so these patterns are once again valid anyway.