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

Extend the tests in regexp/inspect_spec with over escaping combined with character classes #987

Open
herwinw opened this issue Jan 2, 2023 · 2 comments

Comments

@herwinw
Copy link
Member

herwinw commented Jan 2, 2023

The specs have one test for escaped backslashes in Regexp#inspect:

it "does not over escape" do
  Regexp.new('\\\/').inspect.should == "/\\\\\\//"
end

The obvious implementation to make this spec pass is to simply escape every slash with another slash. This works in this case, but that causes an error in language/case_spec for something that is not related to the case statement:

it "tests with a regexp interpolated within another regexp" do
  digits_regexp = /\d+/
  case "foo43"
  when /oo(#{digits_regexp})/

This results in a regexp /oo(?-mix:\\d+)/, which now expects a slash followed by one or more d characters. This does not match the input, but does not indicate the escaping as the issue.

I tested my implementation with the these specs added, but this was more or less brute forcing every possibility. Also, because Regexp.new takes a String which has an additional layer of escaping, I peferred the // notation.

Regexp.new('\d+').inspect.should == "/\\d+/"
/\d+/.inspect.should == "/\\d+/"

Regexp.new('\\d+').inspect.should == "/\\d+/"
/\\d+/.inspect.should == "/\\\\d+/"

Regexp.new('\\\d+').inspect.should == "/\\\\d+/"
/\\\d+/.inspect.should == "/\\\\\\d+/"

Regexp.new('\\\\d+').inspect.should == "/\\\\d+/"
/\\\\d+/.inspect.should == "/\\\\\\\\d+/"

I think it would be better to at least include the following cases:

/\d+/ # Output should have 1 not escaped slash, since this should not be escaped
/\\d+/ # Output should have 1 escaped slash (so 2 slashes), this should be escaped
/\\\d+/ # Output should have 1 escaped and 1 not escaped slash (so 3 slashes)

Furthermore, I think it would be better to add an explicit test for the regexp embedded in regexp scenario like there is in the case spec now, just to make the test a bit more explicit and decouple it from the case statement.

I'm not that familiar with this project and how things are structured, can I just add a few tests to the inspect spec of regexp, and make a new spec file for the embedding tests?

@herwinw
Copy link
Member Author

herwinw commented Jan 2, 2023

For what it's worth: natalie-lang/natalie#779 has a bit of a write up with the issues I encountered

@eregon
Copy link
Member

eregon commented Jan 3, 2023

I'm not that familiar with this project and how things are structured, can I just add a few tests to the inspect spec of regexp, and make a new spec file for the embedding tests?

Yes, just make a PR and based on the changes we can find out what's the right place.
Specs testing interpolating a regexp in another regexp should probably move to language/regexp_spec.rb or core/regexp/to_s_spec.rb.

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

No branches or pull requests

2 participants