Bug 3960 – Unused local variables not reported

This is C program: #include "stdio.h" #include "stdlib.h" int main(int argc, char** argv) { int x, y; x = (argc > 1) ? atoi(argv[1]) : 10; printf("%d\n", x); return 0; } Compiled with gcc v.4.4.1 with: gcc -Wall foo.c -o foo The compiler prints: foo.c: In function 'main': foo.c:4: warning: unused variable 'y' The Intel C/C++ compiler gives a similar error (this is a mockup, but it's equal or very close to the real one): foo.c(4): warning #177: variable "y" was declared but never referenced int x, y; ^ ----------------------- This is C# code: using System; public class Foo { public static void Main() { int x, y; x = int.Parse(Console.ReadLine()); Console.WriteLine(x); } } With default settings the C# compiler prints: prog.cs(4,16): warning CS0168: The variable `y' is declared but never used Compilation succeeded - 1 warning(s) This explains the warning CS0168: http://msdn.microsoft.com/en-us/library/tf85t6e4.aspx ----------------------- In Java all the most important editors/IDEs detect unused local variables: JDeveloper, Eclipse, JEdit, JBuilder, BlueJ, CodeGuide, NetBeans/Sun Java Studio Enterprise/Creator, etc. If the most important compilers/IDEs of the most important and used languages (C/C++, C#, Java) give warnings for unused variables, then it can be an useful warning. By itself an unused variable is not an error, but its presence is often caused due to oversight (either you declared a variable you didn't need or you refactored and a variable that was once needed now is no long needed). While coding in C the GCC compiler has avoided me 2 possible bugs thanks to that unused variable warning, because I was forgetting some part of the code. So I believe this warning can be useful in D too.

A related warning can be see with this C# program: using System; namespace Test { class Foo { int x; private void Add(int y) { x += y; } static void Main() { System.Console.WriteLine("test"); } } } Compiled with mono: ...gmcs test.cs test.cs(5,22): warning CS0169: The private method `Test.Foo.Add(int)' is never used Compilation succeeded - 1 warning(s) In D private methods can be used by other functions in the same module, so it's less easy to produce a warning like this.

There is more than just unused variables, there are also unused last assignments: void main() { int x; x = 10; } Here 'x' is not an unused variable, because the code does something with it, but the code deserves a warning anyway (and one C compiler-like shows this warning) because the last value assigned to it gets unused; and this is wasted coding/running effort at best, or sign of a possible latent bug (just like unused variables). This warning isn't necessary if the variable address is assigned to a pointer or similar things.

Okay. Giving a warning for an unused variable makes sense. However, giving a warning for setting it somewhere that is not where it is declared doesn't make sense. Sure, in this case, it's dumb of the programmer to have done that, but 1. The compiler should be able to optimize out such a simple case. 2. More complex cases much harder to detect, and it's not all that hard for you to _want_ to disconnect the declaration of the variable and initializing it - likely because of scoping issues or conditional blocks. The compiler is only going to be able to detect the simplest cases, and as I said in #1, such simple cases will likely get optimized away. Cases too complex to optimize away are going to be much harder for the compiler to detect, and if you start going that far, you're getting to the point where you just about should have done what Java did and force initialization of variables rather than default initialize them. Not initializing a variable yourself, and the setting it later with a value that you could have set it to at its declaration is not a coding error. It may not be best practice, but it's not going to result in an error. So, I don't think that it makes any sense to make it a warning.

Sorry, my mistake, I have lumped two different warnings into this enhancement request, so please ignore comment 2 I have moved the second warning to bug 4694 Look at bug 4694 for an answer to comment 3 regarding the second warning.

See also "Using Redundancies to Find Errors", by Yichen Xie and Dawson Engler, 2002: www.stanford.edu/~engler/p401-xie.pdf

Two new good related flags in GCC 4.6: >New -Wunused-but-set-variable and -Wunused-but-set-parameter warnings were added for C, C++, Objective-C and Objective-C++. These warnings diagnose variables respective parameters which are only set in the code and never otherwise used. Usually such variables are useless and often even the value assigned to them is computed needlessly, sometimes expensively. The -Wunused-but-set-variable warning is enabled by default by -Wall flag and -Wunused-but-set-parameter by -Wall -Wextra flags.< Some discussions in this thread: http://www.digitalmars.com/webnews/newsgroups.php?art_group=digitalmars.D&article_id=132909

*** Issue 3163 has been marked as a duplicate of this issue. ***

This is accepts-invalid, not an enhancement request. http://www.digitalmars.com/d/1.0/function.html "It is an error to declare a local variable that is never referred to."

I would point out that if it is made an error to have unused local variables, function parameters must _not_ be included in it. That would be a major problem in cases where overridden functions don't use all of their parameters, but they have to have them because the base class' function does.

(In reply to comment #9) > I would point out that if it is made an error to have unused local variables, > function parameters must _not_ be included in it. That would be a major problem > in cases where overridden functions don't use all of their parameters, but they > have to have them because the base class' function does. Don't worry, warnings against "unused local variables" and "unused argument values" are meant to be two distinct features. Each of them needs a separate discussion and has to stand for its own.

(In reply to comment #8) > This is accepts-invalid, not an enhancement request. > > http://www.digitalmars.com/d/1.0/function.html > "It is an error to declare a local variable that is never referred to." See: http://www.digitalmars.com/d/archives/digitalmars/D/bugs/2105.html http://www.digitalmars.com/d/archives/digitalmars/D/Re_Wish_Variable_Not_Used_Warning_73539.html It actually seems more likely that this is a bug in the spec, as I think Walter is against pretty much anything that requires flow analysis.

(In reply to comment #11) > (In reply to comment #8) > > This is accepts-invalid, not an enhancement request. > > > > http://www.digitalmars.com/d/1.0/function.html > > "It is an error to declare a local variable that is never referred to." > > See: > http://www.digitalmars.com/d/archives/digitalmars/D/bugs/2105.html > http://www.digitalmars.com/d/archives/digitalmars/D/Re_Wish_Variable_Not_Used_Warning_73539.html > > It actually seems more likely that this is a bug in the spec, as I think Walter > is against pretty much anything that requires flow analysis. In the beginning this was an enhancement request, now someone has made it a kind of error. But in the end the situation is the same. In most cases in well written code you don't want unused variables, and various experiments have shown that unused experiments are often associated with bugs. On the other hand lint tools in C that are able to spot such unused variables are seldom used, and several of the most used compilers do spot such unused variables today. So I conclude that I'd like the D compiler to spot them automatically for me.

(In reply to comment #11) > It actually seems more likely that this is a bug in the spec, as I think Walter > is against pretty much anything that requires flow analysis. What has this issue to do with flow analysis?

(In reply to comment #13) > (In reply to comment #11) > > It actually seems more likely that this is a bug in the spec, as I think Walter > > is against pretty much anything that requires flow analysis. > > What has this issue to do with flow analysis? You're right! I'm still fairly sure I've seen Walter say no to this request somewhere. If I can find it I'll change the report to spec/enhancement.

(In reply to comment #14) > I'm still fairly sure I've seen Walter say no to this request somewhere. If I > can find it I'll change the report to spec/enhancement. This isn't an enhancement. That the compiler doesn't behave according to the spec is an outright bug. It might be a case of removing the statement from the spec. But - it won't stop people wanting the compiler to warn of an unused local variable, so this would remain valid albeit as an enhancement request - any clue of why Walter may have changed his mind about this?

(In reply to comment #15) > (In reply to comment #14) > > I'm still fairly sure I've seen Walter say no to this request somewhere. If I > > can find it I'll change the report to spec/enhancement. > > This isn't an enhancement. That the compiler doesn't behave according to the > spec is an outright bug. > > It might be a case of removing the statement from the spec. But > - it won't stop people wanting the compiler to warn of an unused local > variable, so this would remain valid albeit as an enhancement request > - any clue of why Walter may have changed his mind about this? All I can find is http://www.digitalmars.com/d/archives/digitalmars/D/Unused_variables_better_as_error_or_warning_115751.html#N115794 and http://www.digitalmars.com/d/archives/c++/command-line/282.html (from 2003!) If the spec is wrong, then this _is_ a valid enhancement _and_ a valid spec bug.

(In reply to comment #9) > I would point out that if it is made an error to have unused local variables, > function parameters must _not_ be included in it. That would be a major problem > in cases where overridden functions don't use all of their parameters, but they > have to have them because the base class' function does. In languages where you cannot omit the name of the parameter it is a problem. In D you just leave the parameter name away and this problem is solved + it becomes evident from looking at the method, that it doesn't use a certain parameter. That would make a nice coding standard as well.

Whether or not a parameter is used is unrelated to whether or not it has a name.

(In reply to comment #18) > Whether or not a parameter is used is unrelated to whether or not it has a > name. You can't use an unnamed parameter, or can you? What I wanted to hint at is this: interface I { void foo(int a, int b); } class C { int sum_a; void foo(int a, int b) { sum_a += a; } } This is a typical situation I face in Java code. The IDE (Eclipse in this case) would warn me, that I don't use parameter 'b'. Now my code is perfectly fine and my implementation of foo() doesn't depend on the value of 'b' to do its job. The better solution is to write: void foo(int a, int) { sum_a += a; } Now the interface is happy, as is the compiler check, because we made it clear, that we intended to ignore 'b'.

(In reply to comment #19) > Now the interface is happy, as is the compiler check, because we > made it clear, that we intended to ignore 'b'. Indeed. Some C++ compilers work on the principle that if you named a parameter, you intended to use it, and so issue a warning if you haven't used it. But this bug report is about unused local variables, not unused function parameters.

You are right. Since local variables and parameters are related, it could be that the person fixing this also thinks about the situation for function parameters. A new bug report or enhancement request would cause twice the work in this case.

I know that Walter is against having warnings or errors for unused parameters (he has _definitely_ said as much on the newsgroup), but he may feel differently about local variables. The spec specifically says that unused _local_ variables are an error. So, Walter probably views those differently, which makes some sense - particularly since they aren't affected by overriding and the like.

(In reply to comment #19) > (In reply to comment #18) > > Whether or not a parameter is used is unrelated to whether or not it has a > > name. > > You can't use an unnamed parameter, or can you? > I can: import std.stdio; double foo(double){return _param_0;} void main(){writeln(foo(2));}

I have found an interesting information. In Go language an unused variable is an _error_: package main func main() { x := 10; } prog.go:3: x declared and not used

See also Issue 2197 and Issue 4694

I think that issue# 7989 is a great argument for why there shouldn't be any warnings or errors for unused variables. Such would needlessly make writing template constraints harder.

This discussion is relevant to both this and issue 7989, so I'm continuing it on digitalmars.D.learn under the existing "Docs: Section on local variables" thread.

(In reply to comment #26) > I think that issue# 7989 is a great argument for why there shouldn't be any > warnings or errors for unused variables. Such would needlessly make writing > template constraints harder. The fact that in some uncommon situations you want to define a variable and not use it, can't justify the lack of a compile reaction to unused variables. There are solutions to that problem, Steven Schveighoffer suggests something like: pragma(used) int x; Or: @used int x;

(In reply to comment #28) > There are solutions to that problem, Steven Schveighoffer suggests > something like: > > pragma(used) int x; > > Or: > @used int x; Does this gain anything over what I suggested in issue 7989 comment 3?

I wrote a short programm which detect and list unused variables. I'm sure it isn't perfect but it passed most of my test cases and IMO something like this should be integrated into the D compiler. Maybe automatically if you use the -w or -wi compiler flag switch. Code: http://dpaste.dzfl.pl/c5f1d2ba Some of my testcases: http://dpaste.dzfl.pl/f74da85e

(In reply to comment #30) > Code: http://dpaste.dzfl.pl/c5f1d2ba > Some of my testcases: http://dpaste.dzfl.pl/f74da85e Paste sites eventually lose their content. That's why I have said to *attach* your stuff in Bugzilla.

Created attachment 1128 Unused variables detector code Forget to upload the code of my unused variables detector.

A related warning is for "unused local alias". A bug in GCC, libstdc++/33084, in <valarrray> function with this body: typedef _BinClos<_Name, _Constant, _ValArray, _Tp, _Tp> _Closure; typedef typename __fun<_Name, _Tp>::result_type _Rt; return _Expr<_Closure, _Tp>(_Closure(__t, __v)); There is a typo: _Rt -> _Tp In GCC 4.7, the -Wunused-local-typedefs warning detects such sort of suspect "unused" typedef.

Commit pushed to master at https://github.com/dlang/dlang.org https://github.com/dlang/dlang.org/commit/eb74b278dfde3982ddeaf6f7a57af76462a8a9b0 Fix Issues 3960 and 7989

Commit pushed to stable at https://github.com/dlang/dlang.org https://github.com/dlang/dlang.org/commit/eb74b278dfde3982ddeaf6f7a57af76462a8a9b0 Fix Issues 3960 and 7989

Automatically set resolution type was "FIXED". "WONTFIX" matches better, as unused locals are now allowed, in case anyone else watching this issue got confused.