Feature #1269: warning when Kernel#p is used

• Status: Rejected
• Assignee: matz (Yukihiro Matsumoto)
• Target version: 2.0.0

[mame (Yusuke Endoh)]

=begin
遠藤です。

Kernel#p は非常に便利ですが、デバッグ後に p の呼び出しをすべて消せたか
不安になることがあります。
p という名前は使うときは非常に便利ですが、残念ながら検索するには非常に
不便です。単語区切りで検索できないエディタだと検索は限りなく困難ですし、
単語区切りで検索できるエディタであっても、

• p という変数や attr を使っている場合
• rdoc 中のサンプルコードとして p を使っている場合
• html 断片の

  や sprint の %p などが文字列リテラル中にある場合

など、false positive がとても多いです。
p を再定義して出力を止めれば p の呼び出しが残っても実害はなくせますが、
余計なコードが残るのは気持ち悪いですし、後でほかのデバッグをするときに
邪魔になる恐れもあります。

そこで、$DEBUG = true の場合に p を関数呼び出ししている箇所があったら
警告することを提案します。

通常時は何も言わない

$ ruby19 -e 'p :debug'
:debug

-d をつけると警告する

$ ruby19 -d -e 'p :debug'
-e:1: warning: Kernel#p is used
:debug

パーサでチェックするので実行されない場合でも検出できる

$ ruby19 -d -e 'p :debug if rand > 0.999'
-e:1: warning: Kernel#p is used

関数呼び出しでなければ警告しない

$ ruby19 -d -e 's = Struct.new(:p); p = s.new; p.p = "

"; puts p.p'

もちろん、eval される文字列中の p の呼び出しは実行しないと検出できない
ので完璧ではないですが、それでも多くの場合はカバーできると思います。

問題点は

• p という名前のメソッドだけ特別扱いで警告されるのが気持ち悪い
• 互換性

くらいかと思いますが、$DEBUG = true なら本番環境でないので、個人的には
許容範囲かなと思いました。どうでしょうか。

Index: id.c

--- id.c (revision 22892)
+++ id.c (working copy)
@@ -47,4 +47,5 @@
REGISTER_SYMID(idSend, "send");
REGISTER_SYMID(id__send__, "send");
REGISTER_SYMID(idInitialize, "initialize");

• REGISTER_SYMID(idP, "p");
  }
  Index: parse.y
  ===================================================================
  --- parse.y (revision 22892)
  +++ parse.y (working copy)
  @@ -1257,6 +1257,9 @@
  command : operation command_args %prec tLOWEST
  {
  /%%%/

•  	if ($1 == idP && RTEST(ruby_debug)) {
  

•  	    rb_warning0("Kernel#p is used");
  

•  	}
   	$$ = NEW_FCALL($1, $2);
   	fixpos($$, $2);
       /*%
  

@@ -3525,6 +3528,9 @@
method_call : operation paren_args
{
/%%%/

•  	if ($1 == idP && RTEST(ruby_debug)) {
  

•  	    rb_warning0("Kernel#p is used");
  

•  	}
   	$$ = NEW_FCALL($1, $2);
   	fixpos($$, $2);
       /*%
  

Index: id.h

--- id.h (revision 22892)
+++ id.h (working copy)
@@ -99,6 +99,7 @@
tSend,
t__send__,
tInitialize,

• tP,
  #if SUPPORT_JOKE
  tBitblt,
  tAnswer,
  @@ -118,7 +119,8 @@
  TOKEN2ID(Lambda),
  TOKEN2ID(Send),
  TOKEN2ID(send),

• TOKEN2ID(Initialize)

• TOKEN2ID(Initialize),
• TOKEN2ID(P)
  };

#ifdef tLAST_TOKEN
Index: template/id.h.tmpl

--- template/id.h.tmpl (revision 22892)
+++ template/id.h.tmpl (working copy)
@@ -92,6 +92,7 @@
tSend,
t__send__,
tInitialize,

• tP,
  #if SUPPORT_JOKE
  tBitblt,
  tAnswer,
  @@ -111,7 +112,8 @@
  TOKEN2ID(Lambda),
  TOKEN2ID(Send),
  TOKEN2ID(send),

• TOKEN2ID(Initialize)

• TOKEN2ID(Initialize),
• TOKEN2ID(P)
  };

#ifdef tLAST_TOKEN

--
Yusuke ENDOH [e-mail]
=end

[usa (Usaku NAKAMURA)]

=begin
こんにちは、なかむら(う)です。

In message "[ruby-dev:38153] [feature:trunk] warning when Kernel#p is used"
on Mar.11,2009 21:29:22, [e-mail] wrote:

問題点は

• p という名前のメソッドだけ特別扱いで警告されるのが気持ち悪い
• 互換性

くらいかと思いますが、$DEBUG = true なら本番環境でないので、個人的には
許容範囲かなと思いました。どうでしょうか。

私は

p foo if $DEBUG

というコードを頻繁に書くんですが、ダメなんでしょうか。
pを使うのはデバッグ時なわけですから、当然$DEBUGがtrueの時にこ
そpに本来の仕事を余計な妨害なしでこなしてもらうべきだと思いま
す。

アイデア自体は悪くないと思いますが、パーサレベルでチェックす
るのであれば、syntax check (-c)などでひっかけるのがよいのでは
ないかと思います。

それでは。

U.Nakamura [e-mail]

=end

[mame (Yusuke Endoh)]

=begin
遠藤です。

2009/03/11 21:50 U.Nakamura [e-mail]:

私は

p foo if $DEBUG

というコードを頻繁に書くんですが、ダメなんでしょうか。

p というすばらしい名前の利点が掻き消えるのでダメです。

さらに、ある箇所のデバッグ中に p foo if $DEBUG と書いて残して
しまうと、別の箇所をデバッグするために p bar if $DEBUG としたら
出力が混ざってわけわからなくなります。

pを使うのはデバッグ時なわけですから、当然$DEBUGがtrueの時にこ
そpに本来の仕事を余計な妨害なしでこなしてもらうべきだと思いま
す。

確かに $DEBUG を使うのはいまいちですね。
p の検出機能の有効・無効を切り替えられればなんでもいいので、

• 新しいオプションを追加する
• 環境変数で指定する
• RubyVM.warn_p = true という warn-p.rb を require する

など、特にこだわりはないです。(最後のは名前以外わりといいかも？)

アイデア自体は悪くないと思いますが、パーサレベルでチェックす
るのであれば、syntax check (-c)などでひっかけるのがよいのでは
ないかと思います。

それでもいいと思います。

--
Yusuke ENDOH [e-mail]

=end

[ko1 (Koichi Sasada)]

=begin
　ささだです．

Yusuke ENDOH wrote::

• 新しいオプションを追加する
• 環境変数で指定する
• RubyVM.warn_p = true という warn-p.rb を require する

など、特にこだわりはないです。(最後のは名前以外わりといいかも？)

　warn-p.rb を使いたい人が require する，だと足りないですかね．で，p を
再定義してしまえば．

アイデア自体は悪くないと思いますが、パーサレベルでチェックす
るのであれば、syntax check (-c)などでひっかけるのがよいのでは
ないかと思います。

それでもいいと思います。

　パーサレベルでは難しいんじゃないですかね．

--
// SASADA Koichi at atdot dot net

=end

[mame (Yusuke Endoh)]

=begin
遠藤です。

2009/03/11 23:48 SASADA Koichi [e-mail]:

　ささだです．

Yusuke ENDOH wrote::

• 新しいオプションを追加する
• 環境変数で指定する
• RubyVM.warn_p = true という warn-p.rb を require する

など、特にこだわりはないです。(最後のは名前以外わりといいかも？)

　warn-p.rb を使いたい人が require する，だと足りないですかね．で，p を
再定義してしまえば．

ええと、提案のポイントは「p という名前のメソッドを関数呼び出しする
箇所を (静的に) 検出して警告する機能を入れよう」です。

alias my_p p として my_p を使うなどすると検出できなくなりますし、
自分で p というメソッドを定義して関数呼び出しすれば誤検出します。
が、(検出回避など以外の目的で) そんなことすることはなさそうですし、
この機能を使わなければ現状と何も変わりません。

で、この機能を有効にするトリガの案として、最初は「$DEBUG = true」を
挙げていましたが、これだと、検出は要らないけれど $DEBUG は使いたい
人が困るのでいまいちでした。
そこで改良案として、上では「warn-p を require する」をトリガとして
挙げています。

アイデア自体は悪くないと思いますが、パーサレベルでチェックす
るのであれば、syntax check (-c)などでひっかけるのがよいのでは
ないかと思います。

それでもいいと思います。

　パーサレベルでは難しいんじゃないですかね．

「p という名前のメソッドを関数呼び出しするかどうか」を検査するだけ
なのでできます。最初のメールにパッチがあります。

--
Yusuke ENDOH [e-mail]

=end

[marcandre (Marc-Andre Lafortune)]

• Category set to core
• Assignee set to matz (Yukihiro Matsumoto)

[naruse (Yui NARUSE)]

• Status changed from Open to Assigned

[mame (Yusuke Endoh)]

• Target version set to 2.0.0

=begin
遠藤です。

2009年3月11日21:31 Yusuke ENDOH [e-mail]:

Kernel#p は非常に便利ですが、デバッグ後に p の呼び出しをすべて消せたか
不安になることがあります。
p という名前は使うときは非常に便利ですが、残念ながら検索するには非常に
不便です。単語区切りで検索できないエディタだと検索は限りなく困難ですし、
単語区切りで検索できるエディタであっても、

• p という変数や attr を使っている場合
• rdoc 中のサンプルコードとして p を使っている場合
• html 断片の

  や sprint の %p などが文字列リテラル中にある場合

など、false positive がとても多いです。
p を再定義して出力を止めれば p の呼び出しが残っても実害はなくせますが、
余計なコードが残るのは気持ち悪いですし、後でほかのデバッグをするときに
邪魔になる恐れもあります。

そこで、$DEBUG = true の場合に p を関数呼び出ししている箇所があったら
警告することを提案します。

という提案を 1 年前にしましたが、$DEBUG というインターフェイス
にケチがついていました。

2009年3月11日21:50 U.Nakamura [e-mail]:

アイデア自体は悪くないと思いますが、パーサレベルでチェックす
るのであれば、syntax check (-c)などでひっかけるのがよいのでは
ないかと思います。

[ruby-dev:40768] を見て思いついたのですが、ruby -c -v の時だけ
警告してくれるといいんじゃないかと思いました。

蛇足ですが、他にも簡単な lint を実装するなど面白そう。
ローカル変数は全部小文字で _ 区切りとか、クラス名は Camel Case
とか。一行 80 桁とか、1 メソッドの行数は 24 行までとか。異論は
ありそう。

--
Yusuke ENDOH [e-mail]
=end

[yugui (Yuki Sonoda)]

=begin
2010/3/26 Yusuke Endoh [e-mail]:

蛇足ですが、他にも簡単な lint を実装するなど面白そう。
ローカル変数は全部小文字で _ 区切りとか、クラス名は Camel Case
とか。一行 80 桁とか、1 メソッドの行数は 24 行までとか。異論は
ありそう。

ripperを使ったlint実装の例を公開するほうが、汎用性があって弊害が少なく、その後に続くcheckerやらindentやらの礎になりそうですね。

--
Yuki Sonoda (Yugui)
[e-mail]
http://yugui.jp

=end

[ko1 (Koichi Sasada)]

これ，どうしましょう．

[matz (Yukihiro Matsumoto)]

まつもと ゆきひろです

In message "Re: [ruby-dev:43705] [Ruby 1.9 - Feature #1269] warning when Kernel#p is used"
on Sat, 11 Jun 2011 14:21:18 +0900, Koichi Sasada [e-mail] writes:

|これ，どうしましょう．

pをコンパイル時に特別扱いというのがなんとなく納得できないので、
1.9.3には入れない方向で。

[matz (Yukihiro Matsumoto)]

まつもと ゆきひろです

In message "Re: [ruby-dev:43705] [Ruby 1.9 - Feature #1269] warning when Kernel#p is used"
on Sat, 11 Jun 2011 14:21:18 +0900, Koichi Sasada [e-mail] writes:

|これ，どうしましょう．

pをコンパイル時に特別扱いというのがなんとなく納得できないので、
1.9.3には入れない方向で。

[kosaki (Motohiro KOSAKI)]

• Status changed from Assigned to Rejected

これも問答無用で閉じてしまいます。蒸し返したい人がreopenしてください。個人的には １）ｐで警告を出したい と ２）パーサーでｐを検出したい の間に距離があるので２つの事をパラで議論するチケットになってしまっていて収束する傾向に見えないです