search

seattlerb / ruby_parser

ruby_parser is a ruby parser written in pure ruby. It outputs s-expressions which can be manipulated and converted back to ruby via the ruby2ruby gem.
http://www.zenspider.com/projects/ruby_parser.html
476 stars 100 forks source link

%r!^on_(?\!or_build$)! gets translated incorrectly? #255

Closed envygeeks closed 6 years ago

envygeeks commented 6 years ago

Upon moving to CodeClimate v2 we started noticing our builds were failing, and in that error I also noticed that %r!^on_(?\!or_build$)! is causing a warning when parsed by RubyParser. Upon further investigation the warning seems to be the result of %r!^on_(?\!or_build$)! -> /^on_(?\!or_build$)/

Full-Disclosure: I don't know if it's directly caused by Ruby Parser or if CodeClimate is doing something that's causing it, but we just noticed it's spit out when it's ran by CodeClimate.

zenspider commented 6 years ago

Seems legit:

9995 % ruby -cwe '%r!^on_(?\!or_build$)!'
-e:1: warning: possibly useless use of a literal in void context
Syntax OK
9995 % rake debug R='%r!^on_(?\!or_build$)!'
WARNING: undefined group option: /^on_(?\!or_build$)/ for "^on_(?\\!or_build$)" ""
WARNING: trying to recover with ENC_UTF8
WARNING: trying to recover with ENC_NONE
rake aborted!
RegexpError: undefined group option: /^on_(?\!or_build$)/
/Users/ryan/Work/p4/zss/src/ruby_parser/dev/lib/ruby_parser_extras.rb:771:in `initialize'
zenspider commented 6 years ago

anything?

envygeeks commented 6 years ago

?

zenspider commented 6 years ago

Nevermind. I think I got myself confused triaging today and now I'm back again.

While legit... I must point out that your original regexp is looks horrible. The whole point of %rX...X is to choose an X that doesn't need to be escaped in the body of your regexp. You could write it as: %r%^on_(?!or_build$)% or even /^on_(?!or_build$)/.

The lexing of strings/heredocs/regexps is such a rats nest as it is... I don't think this particular edge case will ever be addressed unless someone wants to submit a PR for it.

zenspider commented 6 years ago

Nevermind. I was able to track it down to a narrow enough surface area... I'm sure there are other edge cases this fix doesn't cover...

envygeeks commented 6 years ago

I agree the regexp was garbage, and I'm glad you were able to narrow it down a bit to fix some part of the case, there were circumstances that led to that regexp being what it is, that have since been fixed.

Thanks!