Skip to content
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

chrono-tz doesn't build anymore with latest regex-syntax #455

Closed
meh opened this issue Mar 12, 2018 · 8 comments
Closed

chrono-tz doesn't build anymore with latest regex-syntax #455

meh opened this issue Mar 12, 2018 · 8 comments

Comments

@meh
Copy link

meh commented Mar 12, 2018

Sorry for finding these issues in small steps instead of all at once, one of the regular expressions in chrono-tz is not parsing anymore, which makes the build script fail.

The regular expression comes from here, an issue about it was opened here.

Would this be considered a breaking change since it was working before?

@BurntSushi
Copy link
Member

Yes, it's a bug, and not surprising sadly :-(. I was somewhat expecting bugs in the case insensitive mode. I wrote as many tests as I could think of, but it looks like some cases have eluded me! I will try to get a fix out today.

@BurntSushi
Copy link
Member

OK, I've reproduced this down to a minimal example:

(?x)[ / - ]

which produces

regex parse error:
    (?x)[ / - ]
        ^^
error: unclosed character class

Another variant:

(?x)[ a - ]

which produces

regex parse error:
    (?x)[ a - ]
          ^^^^^
error: invalid character class range, the start must be <= the end

In other words, the parser is treating the trailing - as if it signified a range, but it should actually treat the regexes as equivalent to [/-] (or [a-]), which is a special case of character class syntax where the trailing - is treated as a literal - instead of a marker for a range. We get [/-] right, but not (?x)[ / - ] right, presumably because the check for the trailing - is not accounting for whitespace insensitive mode.

This should be straight-forward to fix.

@meh
Copy link
Author

meh commented Mar 12, 2018

Nice, I'm going to add [patch.crates-io] for regex-syntax when it's fixed so I can see if there's anything else that affects me before I make you go through the release churn again 🐼

@BurntSushi
Copy link
Member

Thanks! <3 :)

@BurntSushi
Copy link
Member

OK, I've pushed a fix. I did try running your chrono-tz tests and everything seemed to work. If you have other crates with lots of regexes, it would be much appreciated to test those too if possible! :-)

@meh
Copy link
Author

meh commented Mar 12, 2018

Everything is building and all tests are green here.

Thanks!

@BurntSushi
Copy link
Member

@meh regex-syntax 0.5.2 is now on crates.io with this fix. I don't believe a new regex release is required. (Come to think of it, the previous release of regex wasn't required either, although I did that to bring in the nest_limit API.)

@meh
Copy link
Author

meh commented Mar 12, 2018

Everything seems to be still fine, so thanks a bunch 🐼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants