Feature #6693

Don't warn for unused variables starting with _

Added by Marc-Andre Lafortune almost 2 years ago. Updated about 1 year ago.

[ruby-core:46160]
Status:Closed
Priority:Low
Assignee:Yukihiro Matsumoto
Category:core
Target version:next minor

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.

Associated revisions

Revision 36337
Added by Nobuyoshi Nakada almost 2 years ago

[Feature #6693]

  • parse.y (shadowinglvargen, warnunusedvar): no warnings for variables starting with _. [Feature #6693]

History

#1 Updated by Alex Young almost 2 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

Here I don't want the #calls method to be likely to collide with
anything on @delegate, so I'm using a _ as a sort of
method
missing-escaping namespace. While this isn't specific to local
variables, it would be extremely confusing to have identical-looking
conventions for local variables and methods meaning completely different
things.

Does anyone else do this, or is it just me?

--
Alex


Feature #6693: Don't warn for unused variables starting with _
https://bugs.ruby-lang.org/issues/6693

Author: marcandre (Marc-Andre Lafortune)
Status: Open
Priority: Low
Assignee:
Category: core
Target version: 2.0.0

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.

#2 Updated by Nobuyoshi Nakada almost 2 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 (shadowinglvargen, warnunusedvar): no warnings for variables starting with _. [Feature #6693]

#3 Updated by Koichi Sasada almost 2 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 _.  [Feature #6693]

Modified files:
trunk/ChangeLog
trunk/parse.y
trunk/test/ruby/test_rubyoptions.rb

Index: 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 (shadowinglvargen, warnunusedvar): no warnings for
+ variables starting with _. [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
+isprivatelocalid(ID name)
+{
+ VALUE s;
+ if (name == idUScore) return 1;
+ if (!is
localid(name)) return 0;
+ s = rb
id2str(name);
+ if (!s) return 0;
+ return RSTRINGPTR(s)[0] == '';
+}
+
#define LVARUSED ((int)1 << (sizeof(int) * CHARBIT - 1))

static ID
shadowinglvargen(struct parserparams *parser, ID name)
{
- if (idUScore == name) return name;
+ if (is
privatelocalid(name)) return name;
if (dynainblock()) {
if (dvarcurr(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 (isprivatelocalid(v[i])) continue;
rb
compilewarn(rubysourcefile, (int)u[i], "assigned but unused variable - %s", rb_id2name(v[i]));
}
}

Index: test/ruby/test_rubyoptions.rb

--- test/ruby/testrubyoptions.rb (revision 36336)
+++ test/ruby/test
rubyoptions.rb (revision 36337)
@@ -497,7 +497,8 @@
assertinouterr(["-we", "1.times do\n a=1\nend"], "", [], [], feature3446)
assert
inouterr(["-we", "def foo\n 1.times do\n a=1\n end\nend"], "", [], ["-e:3: warning: assigned but unused variable - a"], feature3446)
assertinouterr(["-we", "def foo\n"" 1.times do |a| end\n""end"], "", [], [])
- assert
inouterr(["-we", "def foo\n a=1\nend"], "", [], ["-e:2: warning: assigned but unused variable - _a"], feature3446)
+ feature6693 = ''
+ assert
inouterr(["-we", "def foo\n _a=1\nend"], "", [], [], feature6693)
end

def testshadowingvariable
@@ -509,11 +510,9 @@
["-e:3: warning: shadowing outer local variable - a",
"-e:2: warning: assigned but unused variable - a",
], bug4130)
+ feature6693 = ''
assertinouterr(["-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 testscriptfrom_stdin

ML: ruby-changes@quickml.atdot.net
Info: http://www.atdot.net/~ko1/quickml/

--
// SASADA Koichi at atdot dot net

#4 Updated by Nobuyoshi Nakada almost 2 years ago

なかだです。

At Mon, 9 Jul 2012 06:32:58 +0900,
SASADA Koichi wrote in :

これって入れるって決まったんでしたっけ.

いや、どうでもよさそうな話だったので適当に突っ込んでみました。

--
--- 僕の前にBugはない。
--- 僕の後ろにBugはできる。
中田 伸悦

#5 Updated by Motohiro KOSAKI almost 2 years ago

なかだです。

At Mon, 9 Jul 2012 06:32:58 +0900,
SASADA Koichi wrote in :

これって入れるって決まったんでしたっけ.

いや、どうでもよさそうな話だったので適当に突っ込んでみました。

この話、反対意見があったと記憶してます。
privateっぽいやつに _ つかって見分けやすくすると言うコーディング規約は
納得できるものですし、議論が収束する前にいきなりコミットしてしまうのは
ちょっと感じ悪いのではないでしょうか。せめて ruby-coreでなぜ元提案に
賛成なのかを説明してからコミットして欲しいです。

個人的には

first, middle, last, suffix = parse_name
_ = first = middle = last

ではだめな理由がよく分かりませんでしたが、どっちの結論になっても反対はしません。

#6 Updated by Nobuyoshi Nakada almost 2 years ago

  • Status changed from Closed to Feedback
  • % Done changed from 100 to 50

Sorry, missed .

Now I incline to revert the commit. Any idea?

#7 Updated by Marc-Andre Lafortune almost 2 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?

#8 Updated by Alex Young almost 2 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-27894

Author: marcandre (Marc-Andre Lafortune) Status: Feedback Priority:
Low Assignee: Category: core Target version: 2.0.0

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 = parsename

could read: first, _middle, _last, suffix = parsename

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.

#9 Updated by Alexey Muranov over 1 year 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):

eachwithindex { |-, value| puts value }

-, -, suffix = parse_name

The underscore is treated exceptionally because it can be repeated in block arguments:

{1=>2}.eachwithindex { |,| puts "Oho!" } # Ok

{1=>2}.eachwithindex { |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?

#10 Updated by Eric Hodel over 1 year 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.

#11 Updated by Alexey Muranov over 1 year 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.

#12 Updated by Eric Hodel over 1 year 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}.eachwithindex { |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.

#13 Updated by Alexey Muranov over 1 year ago

I am sorry, i think there is misunderstanding.

{1=>2}.eachwithindex { |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 = parsename # <= \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.

#14 Updated by Koichi Sasada over 1 year ago

  • Assignee set to Yusuke Endoh

#15 Updated by Yusuke Endoh over 1 year ago

  • Assignee changed from Yusuke Endoh to Yukihiro Matsumoto

#16 Updated by Yusuke Endoh over 1 year ago

  • Target version changed from 2.0.0 to next minor

#17 Updated by Marc-Andre Lafortune about 1 year ago

Since 2.0.0p0 has been released with this feature (which I still believe to be good), should we close this?

#18 Updated by Marc-Andre Lafortune about 1 year ago

  • Status changed from Feedback to Closed

Also available in: Atom PDF