http://d.puremagic.com/issues/show_bug.cgi?id=4694

           Summary: Unused last assignment warning
           Product: D
           Version: D2
          Platform: All
        OS/Version: All
            Status: NEW
          Keywords: diagnostic
          Severity: enhancement
          Priority: P2
         Component: DMD
        AssignedTo: nobody@puremagic.com
        ReportedBy: bearophile_hugs@eml.cc


--- Comment #0 from bearophile_hugs@eml.cc 2010-08-20 14:44:40 PDT ---
This enhancement request is about a warning that helps avoid some kinds of bugs.

(This is related but distinct from the enhancement request of bug 3960
This has nothing to do with uninitialized variables, it's kind of the opposite
problem).

The "Unused last assignment warning" means that you assign something to a variable, and then you do nothing with it.

The compiler is often able to remove the dead code, but the presence of this situation may denote low quality code, or code where the programmer has forgotten to use the variable. In both cases it's useful to find such situations.


void main() {
    int x;
    x = 10; // warning here
}


Here 'x' is not an unused variable, because the code does something with it, so there is no warning for unused variable (bug 3960 ), but this code deserves a warning anyway because the last value assigned to x is not unused.

It is better to not show this warning if the variable address is assigned to a pointer or similar things.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=4694



--- Comment #1 from bearophile_hugs@eml.cc 2010-08-20 14:54:07 PDT ---
Answers to a comment by Jonathan M Davis:

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.

---------------

> However, giving a warning for setting it somewhere that is not where it is declared doesn't make sense.

This is not the purpose of this warning.

The purpose of this warning is to spot situations where you assign something to a variable and then you forget to use such contents. It's kind of the opposite of the unused variable.


> 1. The compiler should be able to optimize out such a simple case.

The compiler is often able to remove dead code, but the purpose of this warning is to detect a point where code is written badly (no need for an operation) or it's a possible sign of a spot where the programmer has forgotten to use a variable.


> 2. More complex cases much harder to detect,

This is true, but this is just a warning, it's not a change in the language, so even if the compiler is not able to spot the most complex situations, it's OK still. A partial help against bugs is better than no help.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=4694


Jonathan M Davis <jmdavisProg@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jmdavisProg@gmail.com


--- Comment #2 from Jonathan M Davis <jmdavisProg@gmail.com> 2010-08-20 15:08:49 PDT ---
I'm okay with this as long as it gives no false positives. Unlike with the variable initialization issue, where you might have to initialize something when you don't need to, a false positive in this case would result in having to remove an assignment that you needed. Presumably, if this were implemented, it would not be done in a manner that that would ever happen. So, given that, it seems like a fine request to me. I don't think that it's something that would help me much, but it wouldn't hurt. The main question, however, is whether Walter would ever even considering it, since one, he hates warnings, and two, this involves code flow analysis which he generally avoids.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=4694


Stewart Gordon <smjg@iname.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |smjg@iname.com


--- Comment #3 from Stewart Gordon <smjg@iname.com> 2011-06-10 12:21:59 PDT ---
(In reply to comment #2)
> The main question, however, is whether
> Walter would ever even considering it, since one, he hates warnings, and two,
> this involves code flow analysis which he generally avoids.

Does picking out the most trivial cases constitute control flow analysis?

Cases where the variable isn't in a loop contained within the variable's scope and is not referenced at all between the assignment and the end of the scope certainly ought to be spotted.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=4694



--- Comment #4 from bearophile_hugs@eml.cc 2012-04-24 18:16:35 PDT ---
See also  Issue 2197

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

http://d.puremagic.com/issues/show_bug.cgi?id=4694



--- Comment #5 from bearophile_hugs@eml.cc 2012-07-28 10:43:01 PDT ---
I have found another important use case for this diagnostic enhancement request.

In my code three times or more I have put a not easy to find bug. Maybe the original code was using nested dynamic arrays, or class instances:


void main() {
    auto data = [[10], [20], [30]];
    foreach (f; data)
        f[0]++;
}


And for some reason I replace the nested arrays with a struct or tuple:

import std.typecons: tuple;
void main() {
    alias tuple T;
    auto data = [T(10), T(20), T(30)];
    foreach (f; data)
        f[0]++;
}


The bug is that changes in f are not altering the array data. The correct code is:


import std.typecons: tuple;
void main() {
    alias tuple T;
    auto data = [T(10), T(20), T(30)];
    foreach (ref f; data)
        f[0]++;
}



The C# language is designed to avoid this bug, because foreach on an array of structs produces immutable structs. This is C# code:


struct F {
    public int x;
}
public class Test {
    public static void Main() {
        F[] data = {new F { x = 10 }, new F { x = 20 }, new F { x = 30 }};
        foreach (F f in data)
            f.x++;
    }
}


The C# compiler gives:

test_s.cs(8,13): error CS1654: Cannot assign to members of `f' because it is a
`foreach iteration variable'
Compilation failed: 1 error(s), 0 warnings



A way to help avoid this bug in D is to add the unused last assignment warning:

import std.typecons: tuple;
void main() {
    alias tuple T;
    auto data = [T(10), T(20), T(30)];
    foreach (f; data)
        f[0]++;
}


A warning similar to:

temp.d(6): Warning: last assignment to f[0] is unused

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------