Feature #6693
closedDon't warn for unused variables starting with _
Description
Currently, variables which are set but not used will generate a warning (ruby -w), except if they have the special name "_".
So best practice is to use "_" for all unused variables. This does not encourage readable code.
# Currently must read:
_, _, _, suffix = parse_name
# could read:
_first, _middle, _last, suffix = parse_name
We should not warn for unused variables starting with a "_".
This would create an option (but no obligation) to use more descriptive names than "_" without generating warnings.
Updated by regularfry (Alex Young) about 12 years ago
On 04/07/12 05:01, marcandre (Marc-Andre Lafortune) wrote:
Issue #6693 has been reported by marcandre (Marc-Andre Lafortune).
I don't know about anyone else, but I'm already using _foo
as a pattern
for private-like markers. I think I picked it up from Python.
What I mean when I say "private-like" is "you can see this but shouldn't
have to touch it", or "I need a visible identifier here which won't
collide with your code". It often crops up with delegates or proxies
where I need to obviously separate out the methods on the intermediary
from the methods on the target. For example:
class VCR
attr_reader :_calls
def initialize( delegate )
@delegate = delegate
@_calls = []
end
def method_missing(sym, *args, &blk )
@_calls << [sym, args, blk]
@delegate.__send__( sym, *args, &blk )
end
end
Updated by nobu (Nobuyoshi Nakada) about 12 years ago
- Status changed from Open to Closed
- % Done changed from 0 to 100
This issue was solved with changeset r36337.
Marc-Andre, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.
[Feature #6693]
- parse.y (shadowing_lvar_gen, warn_unused_var): no warnings for
variables starting with _. [ruby-core:46160][Feature #6693]
Updated by ko1 (Koichi Sasada) about 12 years ago
これって入れるって決まったんでしたっけ.
(2012/07/08 7:42), nobu wrote:
nobu 2012-07-08 07:36:25 +0900 (Sun, 08 Jul 2012)
New Revision: 36337
http://svn.ruby-lang.org/cgi-bin/viewvc.cgi?view=rev&revision=36337
Log:
[Feature #6693]* parse.y (shadowing_lvar_gen, warn_unused_var): no warnings for variables starting with _. [ruby-core:46160][Feature #6693]
Modified files:
trunk/ChangeLog
trunk/parse.y
trunk/test/ruby/test_rubyoptions.rbIndex: ChangeLog¶
--- ChangeLog (revision 36336)
+++ ChangeLog (revision 36337)
@@ -1,3 +1,8 @@
+Sun Jul 8 07:36:19 2012 Nobuyoshi Nakada nobu@ruby-lang.org
+
- parse.y (shadowing_lvar_gen, warn_unused_var): no warnings for
variables starting with _. [ruby-core:46160][Feature #6693]
Sat Jul 7 23:07:30 2012 CHIKANAGA Tomoyuki nagachika@ruby-lang.org
* test/csv/test_features.rb: add require for Tempfile.
Index: parse.y¶
--- parse.y (revision 36336)
+++ parse.y (revision 36337)
@@ -8595,12 +8595,23 @@
#undef parser_yyerror
}+static int
+is_private_local_id(ID name)
+{
- VALUE s;
- if (name == idUScore) return 1;
- if (!is_local_id(name)) return 0;
- s = rb_id2str(name);
- if (!s) return 0;
- return RSTRING_PTR(s)[0] == '_';
+}#define LVAR_USED ((int)1 << (sizeof(int) * CHAR_BIT - 1))
static ID
shadowing_lvar_gen(struct parser_params *parser, ID name)
{
- if (idUScore == name) return name;
- if (is_private_local_id(name)) return name;
if (dyna_in_block()) {
if (dvar_curr(name)) {
yyerror("duplicated argument name");
@@ -9324,7 +9335,7 @@
}
for (i = 0; i < cnt; ++i) {
if (!v[i] || (u[i] & LVAR_USED)) continue;
- if (idUScore == v[i]) continue;
- if (is_private_local_id(v[i])) continue;
rb_compile_warn(ruby_sourcefile, (int)u[i], "assigned but unused variable - %s", rb_id2name(v[i]));
}
}
Index: test/ruby/test_rubyoptions.rb
===================================================================
--- test/ruby/test_rubyoptions.rb (revision 36336)
+++ test/ruby/test_rubyoptions.rb (revision 36337)
@@ -497,7 +497,8 @@
assert_in_out_err(["-we", "1.times do\n a=1\nend"], "", [], [], feature3446)
assert_in_out_err(["-we", "def foo\n 1.times do\n a=1\n end\nend"], "", [], ["-e:3: warning: assigned but unused variable - a"], feature3446)
assert_in_out_err(["-we", "def foo\n"" 1.times do |a| end\n""end"], "", [], [])
- assert_in_out_err(["-we", "def foo\n _a=1\nend"], "", [], ["-e:2: warning: assigned but unused variable - _a"], feature3446)
- feature6693 = '[ruby-core:46160]'
- assert_in_out_err(["-we", "def foo\n _a=1\nend"], "", [], [], feature6693)
enddef test_shadowing_variable
@@ -509,11 +510,9 @@
["-e:3: warning: shadowing outer local variable - a",
"-e:2: warning: assigned but unused variable - a",
], bug4130)
- feature6693 = '[ruby-core:46160]'
assert_in_out_err(["-we", "def foo\n"" _a=1\n"" 1.times do |_a| end\n""end"],
"", [],
["-e:3: warning: shadowing outer local variable - _a",
"-e:2: warning: assigned but unused variable - _a",
], bug4130)
"", [], [], feature6693)
end
def test_script_from_stdin
--
ML: ruby-changes@quickml.atdot.net
Info: http://www.atdot.net/~ko1/quickml/
--
// SASADA Koichi at atdot dot net
Updated by nobu (Nobuyoshi Nakada) about 12 years ago
なかだです。
At Mon, 9 Jul 2012 06:32:58 +0900,
SASADA Koichi wrote in [ruby-dev:45925]:
これって入れるって決まったんでしたっけ.
いや、どうでもよさそうな話だったので適当に突っ込んでみました。
--
--- 僕の前にBugはない。
--- 僕の後ろにBugはできる。
中田 伸悦
Updated by kosaki (Motohiro KOSAKI) about 12 years ago
なかだです。
At Mon, 9 Jul 2012 06:32:58 +0900,
SASADA Koichi wrote in [ruby-dev:45925]:これって入れるって決まったんでしたっけ.
いや、どうでもよさそうな話だったので適当に突っ込んでみました。
この話、反対意見があったと記憶してます。 [ruby-core:46170]
privateっぽいやつに _ つかって見分けやすくすると言うコーディング規約は
納得できるものですし、議論が収束する前にいきなりコミットしてしまうのは
ちょっと感じ悪いのではないでしょうか。せめて ruby-coreでなぜ元提案に
賛成なのかを説明してからコミットして欲しいです。
個人的には
first, middle, last, suffix = parse_name
_ = first = middle = last
ではだめな理由がよく分かりませんでしたが、どっちの結論になっても反対はしません。
Updated by nobu (Nobuyoshi Nakada) about 12 years ago
- Status changed from Closed to Feedback
- % Done changed from 100 to 50
Sorry, missed [ruby-core:46170].
Now I incline to revert the commit. Any idea?
Updated by marcandre (Marc-Andre Lafortune) about 12 years ago
Hi,
regularfry (Alex Young) wrote:
I don't know about anyone else, but I'm already using _foo as a pattern
for private-like markers.
I don't think that it would be confusing. We don't confuse variables with methods already, right?
Also, I feel the convention for this already exists with two underscore before and after the method, in particular SimpleDelegate#getobj and #setobj
Everyone is of course free to use their own. In any case, is it frequent that we delegate to an object for which we don't know the methods in advance?
Updated by regularfry (Alex Young) about 12 years ago
On 09/07/12 14:54, marcandre (Marc-Andre Lafortune) wrote:
Issue #6693 has been updated by marcandre (Marc-Andre Lafortune).
Hi,
regularfry (Alex Young) wrote:
I don't know about anyone else, but I'm already using _foo as a
pattern for private-like markers.I don't think that it would be confusing. We don't confuse variables
with methods already, right?
You might not, but it's a common gotcha with local variables and private
methods.
Also, I feel the convention for this already exists with two
underscore before and after the method, in particular
SimpleDelegate#getobj and #setobj
That's true, it does exist, but in my head that specific convention is
reserved for core/stdlib. Maybe I'm being overly paranoid about that.
Everyone is of course free to use their own. In any case, is it
frequent that we delegate to an object for which we don't know the
methods in advance?
Well, yes - part of the point of the VCR class I showed is that it's
library code that's usable pretty much anywhere. I use it (and things
like it) in testing a fair amount, where in general I want to be able to
take advantage of method_missing and not have to care about the
specifics of the delegate. Also, while I might know all the methods on
the delegate now, I don't have foreknowledge of all the methods which
will ever be implemented on it as the project develops.
There is precedent for what you're suggesting, in that Erlang does the
same, so it's not unreasonable. It's just close enough to home that it
makes me uncomfortable.
--
Alex
---------------------------------------- Feature #6693: Don't warn
for unused variables starting with _
https://bugs.ruby-lang.org/issues/6693#change-27894Author: marcandre (Marc-Andre Lafortune) Status: Feedback Priority:
Low Assignee: Category: core Target version: 2.0.0Currently, variables which are set but not used will generate a
warning (ruby -w), except if they have the special name "_".So best practice is to use "_" for all unused variables. This does
not encourage readable code.Currently must read: _, _, _, suffix = parse_name¶
could read: _first, _middle, _last, suffix = parse_name¶
We should not warn for unused variables starting with a "_".
This would create an option (but no obligation) to use more
descriptive names than "_" without generating warnings.
Updated by alexeymuranov (Alexey Muranov) about 12 years ago
It this really necessary to treat the variable name _
exceptionally at the interpreter's level? Why not to use the same rule for all variables, and for unused variables introduce a special placeholder, for example -
not followed by anything other than a delimiter (comma or bracket):
each_with_index { |-, value| puts value }
-, -, suffix = parse_name
The underscore is treated exceptionally because it can be repeated in block arguments:
{1=>2}.each_with_index { |,| puts "Oho!" } # Ok
{1=>2}.each_with_index { |x,x| puts "Oho!" } # SyntaxError: (eval):2: duplicated argument name
Is this exception really necessary? Maybe for repeated arguments, the variable (_
or x
) can be made to hold, for example, the last non-nil argument of those assigned to it? Or, as it works currently for _
, the value can be the array of all the arguments assigned to it, but this seems redundant: if one needs all the values, why not to use distinct variables?
Updated by drbrain (Eric Hodel) about 12 years ago
Why should we break compatibility with ruby 1.9?
RDoc uses double assignment to avoid the unused variable warning for use by ERb via a binding. The syntax error would break RDoc, so I dislike it.
Updated by alexeymuranov (Alexey Muranov) about 12 years ago
drbrain (Eric Hodel) wrote:
Why should we break compatibility with ruby 1.9?
RDoc uses double assignment to avoid the unused variable warning for use by ERb via a binding. The syntax error would break RDoc, so I dislike it.
I am sorry, was this about my proposal? I was not suggesting to raise an error, i just suggested that _
be treated as any other variable, for example by making it possible to repeat any variable, not just _
. For completely unused variables, a special syntax can be used.
Updated by drbrain (Eric Hodel) about 12 years ago
alexeymuranov (Alexey Muranov) wrote:
It this really necessary to treat the variable name
_
exceptionally at the interpreter's level? Why not to use the same rule for all variables, and for unused variables introduce a special placeholder, for example-
not followed by anything other than a delimiter (comma or bracket):
Removing warning suppression via "_" reintroduces warnings I've fixed and breaks backward compatibility.
{1=>2}.each_with_index { |x,x| puts "Oho!" } # SyntaxError: (eval):2: duplicated argument name
Adding this SyntaxError will break RDoc.
PS: you should open a separate feature request for these two changes. They are beyond of this change.
Updated by alexeymuranov (Alexey Muranov) about 12 years ago
I am sorry, i think there is misunderstanding.
{1=>2}.each_with_index { |x,x| puts "Oho!" } # SyntaxError: (eval):2: duplicated argument name
is not a feature request, it is the current behavior. I proposed to remove this syntax error and to allow to repeat any variable, not only _
.
Yes, i agree that this is a bit off topic, and maybe contradicts to this feature request.
I have created a new one: #6869
I even think that i am against this proposal, because it makes variables behave differently based on their names, and the proposed use case is to use unused variables as comments.
Update 2012-08-15: I would propose, instead of adding special rules for variables with special names, to come up with some "best practice" of annotating such situations with comments. For example:
_, _, _, suffix = parse_name # <= \first/, \middle/, \last/, suffix = ...
Think also about the possibility to use unicode characters in comment annotations!
Also, think about those who will read the code: they will never be sure if variables like _
, _first
, etc are just placeholders, or if the programmer actually used them somewhere later.
Updated by ko1 (Koichi Sasada) almost 12 years ago
- Assignee set to mame (Yusuke Endoh)
Updated by mame (Yusuke Endoh) almost 12 years ago
- Assignee changed from mame (Yusuke Endoh) to matz (Yukihiro Matsumoto)
Updated by mame (Yusuke Endoh) almost 12 years ago
- Target version changed from 2.0.0 to 2.6
Updated by marcandre (Marc-Andre Lafortune) over 11 years ago
Since 2.0.0p0 has been released with this feature (which I still believe to be good), should we close this?
Updated by marcandre (Marc-Andre Lafortune) over 11 years ago
- Status changed from Feedback to Closed