Bug 6949 – no warning or error if unsigned variable is compared to 0

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.

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.

(In reply to comment #2) > 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..

(In reply to comment #3) > 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 ...

(In reply to comment #4) > In your code this comment is obsolete: > // Warn when unsigned type is ... Thanks.

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) {} }

(In reply to comment #6) > 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.

(In reply to comment #7) > 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.

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.

(In reply to comment #6) > 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.

(In reply to comment #10) > 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.)

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); }

(In reply to comment #12) > 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.

(In reply to comment #13) > (In reply to comment #12) > > 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.

(In reply to comment #14) > (In reply to comment #13) > > (In reply to comment #12) > > > 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 .

(In reply to comment #15) > 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())

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

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

(In reply to Lionello Lunesu from comment #18) > *** Issue 8874 has been marked as a duplicate of this issue. *** Issue 8874 shows an extensive example of why this warning is quite important.

Implementing what is asked in this bug report leads to code breakage (assuming we issue an error and not a warning) of otherwise valid code. Walter opposed this in the PR so I am going to close this as WONTFIX.