www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.bugs - [Issue 6949] New: no warning or error if unsigned variable is compared to 0

reply d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=6949

           Summary: no warning or error if unsigned variable is compared
                    to 0
           Product: D
           Version: D1 & D2
          Platform: All
        OS/Version: All
            Status: NEW
          Keywords: diagnostic
          Severity: normal
          Priority: P2
         Component: DMD
        AssignedTo: nobody puremagic.com
        ReportedBy: mrmocool gmx.de



void main()
{
    uint i = 0;
    if (i < 0)
        assert(0, "never");
}

I think such a case means that something went subtly wrong and the variable's
type was intended to be signed, so there should be an error or warning.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 14 2011
next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=6949


Walter Bright <bugzilla digitalmars.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bugzilla digitalmars.com
           Severity|normal                      |enhancement


-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 16 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=6949


Andrej Mitrovic <andrej.mitrovich gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |pull
                 CC|                            |andrej.mitrovich gmail.com
         AssignedTo|nobody puremagic.com        |andrej.mitrovich gmail.com



16:14:56 PST ---
https://github.com/D-Programming-Language/dmd/pull/1337

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 29 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=6949


bearophile_hugs eml.cc changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bearophile_hugs eml.cc



I don't know if Walter will accept this change, but surely I welcome it a lot.
I have desired this in D since years.

For me one important use case of this warning is during code refactoring. Let's
say I have written code that uses some ints. Later for some reasons I turn one
or more of those ints into uints or size_ts. In such situation a warning like
this becomes very useful for me (I know it's useful because this warning has
avoided me troubles several times in C-GCC) because the warnings put in
evidence where the program contains code like "if (x < 0)". This means the code
requires that variable to be signed, or the program logic needs some changes.
Either way, the warning tells me the change I was doing is wrong and needs to
be undone or needs further changes.

Regarding how much common this kind of bug is in already debugged famous open
source programs, there is a kind of static analysis tool that has shown to spot
tens of similar bugs in that code. So this bug is common enough.

The disadvantages of this warning are probably some spurious warnings in
templated code. Is Phobos giving some warnings with this patch? More study is
probably needed.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 29 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=6949




16:40:59 PST ---


 Is Phobos giving some warnings with this patch? More study is
 probably needed.


The autotester is running. It failed once because of this:

enum En : uint
{
    a,   // 0
    b    // triggers comparison
}

I've added a workaround so the warning isn't triggered in this case anymore.
We'll see what the tester says..

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 29 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=6949







 I've added a workaround so the warning isn't triggered in this case anymore.
 We'll see what the tester says..


OK. It seems Andrei has appreciated this.

In your code this comment is obsolete:
// Warn when unsigned type is ...

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 29 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=6949




17:21:48 PST ---


 In your code this comment is obsolete:
 // Warn when unsigned type is ...


Thanks.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 29 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=6949




How about code like this?

void main() {
    uint i = 0;
    if (i == -2)
        assert(0, "never");
}


Note that this is currently valid D code:

void main() {
    uint i = -2;
    if (i == -2) {}
}

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 29 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=6949




18:14:27 PST ---


 How about code like this?
 
 void main() {
     uint i = 0;
     if (i == -2)
         assert(0, "never");
 }
 
 
 Note that this is currently valid D code:
 
 void main() {
     uint i = -2;
     if (i == -2) {}
 }


I want to see how Walter reacts to the pull before these are handled.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 29 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=6949







 I want to see how Walter reacts to the pull before these are handled.


Right, OK.

This new error message of yours hits code like this in std.bitmanip:

            enum result = " property  safe "~T.stringof~" "~name~"() pure
nothrow const { auto result = "
                "("~store~" & "
                ~ myToString(maskAllElse) ~ ") >>"
                ~ myToString(offset) ~ ";"
                ~ (T.min < 0
                   ? "if (result >= " ~ myToString(signBitCheck)
                   ~ ") result |= " ~ myToString(extendSign) ~ ";"
                   : "")
                ~ " return cast("~T.stringof~") result;}\n"


Unless D grows a "static ternary operator" (that is usable in this case) it's
not easy to solve such situations.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 29 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=6949




Maybe you want to temporarily turn your new error message into a warning (that
later will become an error again, if you want), so if Phobos gets entirely
compiled with "-wi" it will not stop the compilation and the log will show the
possible bugs spotted by this warning.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 29 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=6949




19:50:14 PST ---


 How about code like this?
 
 void main() {
     uint i = 0;
     if (i == -2)
         assert(0, "never");
 }


Equality has nothing to do with sign, i.e. there is no "signed equality" and
"unsigned equality". There's just equality.

I don't think there's any getting away from the fact that in languages like D
and C, signed and unsigned are simply different ways of viewing the same data.
For example, you can offset a pointer with both signed and unsigned values.
There is no clean separation between the two.

Some languages, such as Java, deal with this duality by defining the unsigned
type out of existence.

I've made more comments in the pull request.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 29 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=6949







 Some languages, such as Java, deal with this duality by defining the unsigned
 type out of existence.


There are languages like Ada, that have both signed and unsigned native
integrals, but behave differently from C/C++/D.

(Even if this patch is eventually refused, I suggest to turn it into a warning
and compile all phobos one time again with "-wi" to look for possible bugs
spotted by this.)

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 29 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=6949




Regarding a comment by Don, is it possible to generate a warning/error only in
foo() and not in bar()?


void foo(uint x) {
    if (x < 0) {} // error or warning here
}
void bar(T)(T x) {
    if (x < 0) {} // OK
}
void main() {
    foo(5U);
    bar(5U);
}

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 30 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=6949


Andrej Mitrovic <andrej.mitrovich gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|pull                        |
         AssignedTo|andrej.mitrovich gmail.com  |nobody puremagic.com



04:43:25 PST ---


 Regarding a comment by Don, is it possible to generate a warning/error only in
 foo() and not in bar()?
 
 
 void foo(uint x) {
     if (x < 0) {} // error or warning here
 }
 void bar(T)(T x) {
     if (x < 0) {} // OK
 }
 void main() {
     foo(5U);
     bar(5U);
 }


I doubt it. Anyway I've closed the pull for now since it's controversial when
(or if) we should have warnings/errors for this. 

If we had a switch like GCC's `-Wtype-limits` then we could implement this
check for all cases when the switch is enabled and not worry about it. But
Walter is against switches so...

Maybe LDC/GDC can or already do support this though.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 30 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=6949








 Regarding a comment by Don, is it possible to generate a warning/error only in
 foo() and not in bar()?
 
 
 void foo(uint x) {
     if (x < 0) {} // error or warning here
 }
 void bar(T)(T x) {
     if (x < 0) {} // OK
 }
 void main() {
     foo(5U);
     bar(5U);
 }


 
 I doubt it.


I'd like a better answer about this, maybe from Don or Hara or someone that
knows this answer better.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 30 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=6949










 Regarding a comment by Don, is it possible to generate a warning/error only in
 foo() and not in bar()?
 
 
 void foo(uint x) {
     if (x < 0) {} // error or warning here
 }
 void bar(T)(T x) {
     if (x < 0) {} // OK
 }
 void main() {
     foo(5U);
     bar(5U);
 }


 
 I doubt it.


 
 I'd like a better answer about this, maybe from Don or Hara or someone that
 knows this answer better.


It's easy.
DSymbol.inTemplateInstance()  returns non-NULL if you are inside a template.
You just need to call it on the function you're in.

BTW std.bigint has a lot of examples of valid comparisons of  unsigned < 0 .

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 30 2012
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=6949







 BTW std.bigint has a lot of examples of valid comparisons of  unsigned < 0 .


The comparisons in std.bigint are like this, all of them are inside templated
functions:

    BigInt opAssign(T: long)(T x)
    {
        data = cast(ulong)((x < 0) ? -x : x);
        sign = (x < 0);
        return this;
    }

    BigInt opOpAssign(string op, T)(T y)
        if ((op=="+" || op=="-" || op=="*" || op=="/" || op=="%"
          || op==">>" || op=="<<" || op=="^^") && is (T: long))
    {
        ulong u = cast(ulong)(y < 0 ? -y : y);
...





 It's easy.
 DSymbol.inTemplateInstance()  returns non-NULL if you are inside a template.
 You just need to call it on the function you're in.


Thank you Don.
If it's not hard to do then I suggest to add that in this line to exclude
templates from this test, and take a look at what the Phobos autotester says:

// Error when unsigned type is compared less-than to zero, but not in enum
declarations or in static if
if ((!(sc->flags & SCOPEstaticif)) && sc->parent &&
!sc->parent->isEnumDeclaration())

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Nov 30 2012
prev sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=6949




09:19:32 PST ---
*** Issue 5539 has been marked as a duplicate of this issue. ***

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