Ruby: refactor regex libraries #8489
Conversation
72936fa to
ae91423
Compare
21c8e6a to
659c009
Compare
438e6c3 to
a5e9a7b
Compare
yoff
left a comment
There was a problem hiding this comment.
LGTM, only typos.
Why does ReDoS.expected change?
@yoff |
ruby/ql/lib/codeql/ruby/Regexp.qll
Outdated
|
|
||
| import regexp.RegExpTreeView // re-export | ||
| private import regexp.ParseRegExp | ||
| import regexp.internal.RegExpTreeView // re-export |
There was a problem hiding this comment.
This appears dangerous: I would expect I could make all sorts of breaking changes to a file in an internal folder (except perhaps if it contains a Public module that documents it explicitly).
There was a problem hiding this comment.
You're right. I think everything in RegExpTreeView should actually be public. So perhaps I should not put it in the internal folder.
nickrolfe
left a comment
There was a problem hiding this comment.
I have a couple of trivial/pedantic comments. Other than that, having two different RegExpTreeView.qll files seems undesirable. Can they be merged?
| @@ -0,0 +1,143 @@ | |||
| /** | |||
There was a problem hiding this comment.
Regarding the naming of this file: although the Ruby class is spelled Regexp, we've been using a capital E (RegExp) for the QL module/class names, for consistency between languages.
There was a problem hiding this comment.
Yes, I considered that, however, for JavaScript the file is named javascript/ql/lib/semmle/javascript/Regexp.qll too.
The one in the |
b15a05f to
1d0d166
Compare
Co-authored-by: yoff <lerchedahl@gmail.com>
Co-authored-by: Nick Rolfe <nickrolfe@github.com>
|
Should we have done a DCA run before merging this PR? I am not intimate with the changes, but the diff is quite large. |
I do not expect any of the changes to have an impact, but yes.. |
This pull request refactors the regular expression libraries for Ruby to match the Python and JavaScript libraries.
I aligned the contents of the regex parser module (
ruby/ql/lib/codeql/ruby/regexp/internal/ParseRegExp.qll) and the regex AST module (ruby/ql/lib/codeql/ruby/regexp/internal/RegExpTreeView.qll) as much as possible with the respective Python implementationspython/ql/lib/semmle/python/regex.qllandpython/ql/lib/semmle/python/RegexTreeView.qllThe additional classes and predicates are moved into
ruby/ql/lib/codeql/ruby/Regexp.qll.This module should be used by most users and imports the parser module and re-exports the AST module.