こんにちは、構文解析研究部員の S.H. です。
社内でやっているエンジニアお茶会で (@koic) さんから以下のIssueの件で「Rubyのパーサー周りの問題ではないか」と相談を受けました。
そこで調査したところ比較的簡単に修正できそうなことが分かり、以下のPull Requestを作成して無事マージされました。
これは、その時の調査した内容などをまとめた記事になります。
もともとの挙動
RuboCop と RuboCop Performance を使っている際に以下のようなコードを自動修正すると、コンパイルエラーになるコードへと変換されてしまうようです。
自動修正前のコード:
v = "117" case "117" in /#{v}/ p "Passed" end # => Passed
自動修正後のコード:
v = "117" case "117" in /#{v}/o # オプションとして `o` が末尾についている p "Passed" end # => NODE_IN: unknown node (NODE_ONCE)
自動修正前のコードとの差分はオプションとして o が末尾についているかどうかだけのようです。またエラーメッセージにある NODE_IN や NODE_ONCE はRuby内部で利用しているASTノードのことなのでRuby本体側の問題らしいことが分かりますね。
調査
まずはエラーメッセージを元に CRuby のソースコードを grep してみました。
NODE関連のエラーメッセージなので該当しそうな個所としては parse.y、node.c、node_dump.c、compile.c、 ruby_parser.c あたりが怪しそうでした。ただ parse.y、node.c、node_dump.c、ruby_parser.c に関してはおそらく無関係かもしれないな思っていました。
これは以下のように考えたためです。
- parse.y、node.c、node_dump.c については、Universal Parser の C API依存削減対応や新しい NODE 追加などを直近で調べていたなので、該当しそうな箇所を見た覚えがないので無関係かもしれない
- またIssueのコードからパターンマッチ関係で発生しているエラーらしく、以前からあるソースコードが怪しいため比較的最近追加された ruby_parser.c ではなさそう
そこで compile.c のソースコードを grep すると該当しそうなところがありました。
iseq_compile_pattern_each という関数の中でエラーメッセージを出力して、パターンマッチで受け取ったパターンのASTノードに合わせて ISeq に変換しているようです。またエラーメッセージと合わせて考えると、 NODE_ONCE というASTノードにマッチした時の処理がないためエラーになっていそうですね。
次に再現コードを作成し、最新のRuby(ruby 3.4.0dev (2024-02-01T12:17:37Z master 8531ac3115) [x86_64-linux])で実行してみました。
v = "117" case "117" in /#{v}/o p "Passed" end
無事、コンパイルエラーになり再現できました。
test.rb:4: NODE_IN: unknown node (NODE_ONCE) test.rb: compile error (SyntaxError)
その後、再現コードがどういったASTノードを生成しているのか dump してみました。
$ ruby --dump parsetree test.rb
以下が dump した結果になります。
########################################################### ## Do NOT use this node dump for any purpose other than ## ## debug and research. Compatibility is not guaranteed. ## ########################################################### # @ NODE_SCOPE (id: 15, line: 1, location: (1,0)-(6,3)) # +- nd_tbl: :v # +- nd_args: # | (null node) # +- nd_body: # @ NODE_BLOCK (id: 13, line: 1, location: (1,0)-(6,3)) # +- nd_head (1): # | @ NODE_LASGN (id: 0, line: 1, location: (1,0)-(1,9))* # | +- nd_vid: :v # | +- nd_value: # | @ NODE_STR (id: 1, line: 1, location: (1,4)-(1,9)) # | +- nd_lit: "117" # +- nd_head (2): # @ NODE_CASE3 (id: 12, line: 3, location: (3,0)-(6,3))* # +- nd_head: # | @ NODE_STR (id: 2, line: 3, location: (3,5)-(3,10)) # | +- nd_lit: "117" # +- nd_body: # @ NODE_IN (id: 11, line: 4, location: (4,0)-(5,12)) # +- nd_head: # | @ NODE_ONCE (id: 7, line: 4, location: (4,3)-(4,10)) # | +- nd_body: # | @ NODE_DREGX (id: 6, line: 4, location: (4,3)-(4,10)) # | +- nd_lit: "" # | +- nd_next->nd_head: # | | @ NODE_EVSTR (id: 4, line: 4, location: (4,4)-(4,8)) # | | +- nd_body: # | | @ NODE_LVAR (id: 3, line: 4, location: (4,6)-(4,7)) # | | +- nd_vid: :v # | +- nd_next->nd_next: # | (null node) # +- nd_body: # | @ NODE_FCALL (id: 8, line: 5, location: (5,2)-(5,12))* # | +- nd_mid: :p # | +- nd_args: # | @ NODE_LIST (id: 10, line: 5, location: (5,4)-(5,12)) # | +- as.nd_alen: 1 # | +- nd_head: # | | @ NODE_STR (id: 9, line: 5, location: (5,4)-(5,12)) # | | +- nd_lit: "Passed" # | +- nd_next: # | (null node) # +- nd_next: # (null node)
これを見ると NODE_ONCE は動的に生成された正規表現用の NODE_DREGEX をラップしていることがわかります。
NODE_ONCE は6年ほど前に導入されたもので、 /regexp/o の時にのみ扱うASTノード ということも以下の記事から分かりました。
ruby-trunk-changes.hatenablog.com
また NODE_DREGX は以下の部分でパターンマッチ用のISeqを生成しているようでした。NODE_LIT(リテラル用のNODE)やNODE_STR なども同じ部分で判定しているようです。
ここまでのことから以下のことが分かりました。
- パターンマッチ用のISeq生成処理で
NODE_ONCEのケースがないため、すり抜けてエラーになっている NODE_ONCEはNODE_DREGXをラップしているだけの NODE なので他への影響範囲は小さそうNODE_DREGXと同じ部分にはNODE_LITやNODE_STRなどもあることから、そこへNODE_ONCEを渡してやればよさそう
これらの調査結果をもとに対応に進めます。
対応
iseq_compile_pattern_each に NODE_ONCE のケースでもISeqを生成するように修正してみました。すると以下の再現コードを実行できるようになりました。
v = "117" case "117" in /#{v}/o p "Passed" end # => Passed
あとはテストケースも追加してPull Requestを作成しました。
先日無事マージされ、現在のRubyのmasterでは修正されています。
反省点
無事修正できましたが反省点もありました。
https://bugs.ruby-lang.org にチケットを作成していなかった点です。
チケットを作成しておくことで後からチケットを見た時にどういった経緯だったかが確認しやすくなりますし、bugs.ruby-lang.org にはバックポートが必要かどうかをチケットに記載できたりもします。 そのため後からバックポートが必要な修正だった場合に、コミッターの方々が対応しやすくなる可能性があります。
こういった点からチケットを起票していなかった点は、良くないですね。
今後はチケットを起票するべきかなども考慮しつつ、対応を進められればと思いますね。
永和システムマネジメントでは、Rubyへのコントリビューションを通じてコミュニティと成長したいエンジニアを絶賛募集しています。
