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

Add builder method for nest_limit. #454

Closed
wants to merge 1 commit into from
Closed

Conversation

meh
Copy link

@meh meh commented Mar 9, 2018

I didn't know where to put the test case but I think crazy is fairly accurate (it's one of thousands of regular expressions coming from libphonenumber).

Also I know this may sound silly, but could this get a version bump too please? The phonenumber crate cannot parse phone numbers anymore and it was through a minor bump, so I'm kind of stuck.

Mirrors the addition to regex-syntax, or there would be no way to set
it.
)
.nest_limit(1024)
.build()
.unwrap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm just from glancing it kind of looks like this regex isn't that deeply nested. I wonder if these is a bug in the logic that computes the nest level. What nest level is reported for this regex?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do I get the nest level? The error only mentions the maximum nest level (or I can't see it).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see, then it's not exposed. Requires debugging. I'd like to explore that before merging this. With that said, even if your bug wasn't a thing, exposing the nest level seems right to me.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, there are some really huge regular expressions in there, so I wouldn't be suprised if some of them went beyond the nest limit in case it is indeed a bug.

@BurntSushi
Copy link
Member

@meh It turns out this was indeed a (quite embarrassing!) bug and your regex definitely should not have tripped the limit. It's fixed on master. I should be able to get a new release out before the weekend is through. If not, please ping me. :-)

@BurntSushi
Copy link
Member

@meh Errmm, I broke up your commit into two pieces but forgot to fix the author information. Sorry about that! :-(

@meh
Copy link
Author

meh commented Mar 10, 2018

@BurntSushi no worries, as long as it's fixed 🐼

@meh
Copy link
Author

meh commented Mar 12, 2018

@BurntSushi release ping 🔔

@BurntSushi
Copy link
Member

@meh Thanks for the ping! I just put regex 0.2.8 and regex-syntax 0.5.1 on crates.io. :-)

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

Successfully merging this pull request may close these issues.

None yet

2 participants