New warnings/errors?

[Daniel-Cortez]

Issue description:

Pawn is positioned as "a robust language with a compiler that performs a maximum of static checks", but there are still a lot of areas where these checks could be improved.
I think it would be useful to have the following warnings/errors:

• (MERGED) redundant code: switch control expression is constant ([MERGED] warning 24X: redundant code: switch control expression is constant #548)
  We already have similar warnings for if statements:

if (false) {} // warning 205: redundant code: constant expression is zero
if (true) {} // warning 206: redundant test: constant expression is non-zero

so why not adopt the same for switch?

• (MERGED) shift overflow in enum element declaration (symbol "%s") ([MERGED] warning 24X: negative or too big shift count #539)
  This can be achieved if the user has too many bit flags.

enum (<<= 1)
{
	e0 = 1, // 0b00000000000000000000000000000001
	e1,     // 0b00000000000000000000000000000010
	// ...
	e31,    // 0b10000000000000000000000000000000
	e32     // overflow
};

• (MERGED) negative or too big shift count ([MERGED] warning 24X: negative or too big shift count #539)
  Again, this might happen if the user has too many bit flags.

enum ePlayerFlags
{
    pfEmailConfirmed,    // 0
    pfTutorialCompleted, // 1
    // ...
    pfImprisoned,        // 31
    pfHospitalized       // 32
};

// ...

player_info[playerid][pFlags] |= (1 << pfHospitalized); // overflow

• (MERGED) enum element "%s" not handled in switch ([MERGED] warning 24X: enum item "%s" not handled in switch #549)
  I already experimented with implementing this warning locally last year, and had a few false-positives in YSI and amx_assembly (1, 2). I think restricting this warning to cases where only 1 or 2 enum element are missing should help to get rid from this kind of false-positives.

• (MERGED) division by zero (error 094: division by zero #538)
  Interestingly, the compiler already has a check for zero division in function flooreddiv() (sc3.c), but it only works if both operands are constant values:

new x = 1 / 0; // error 029: invalid expression, assumed zero
new y = x / 0; // no errors/warnings if the dividend is non-constant

Also, the message "invalid expression, assumed zero" in the above example is misleading, because

1. this is not the same as "division by zero";
2. usually the compiler prints this message because of invalid syntax, while zero division is rather a semantic error.

• (MERGED) enum increment "%s" has no effect on zero value
  Might be useful if the user defined bit flags, but forgot to specify the value for the first enum element (or specified 0 instead of 1 by mistake):

enum ePlayerFlags (<<= 1)
{
    pfEmailConfirmed,    // 0
    pfTutorialCompleted, // enum increment "<<= 1" has no effect on zero value
};

• (MERGED) use of operator "~" on a "bool:" value always results in "true"; did you mean operator "!"?
  Works on bool: values when the user has accidentally used bitwise negation instead of the logical one.

new bool:success = Func();
if (~success) // use of operator "~" on a "bool:" value always results in "true"; did you mean operator "!"?

• (MERGED) possible misuse of comma operator
  Works when the user has used , inside an if/switch/while/do/for control expression, and the sub-expression before the comma doesn't have a side effect, which means that the use of comma is most probably unintended.

// The user could've intended to use "+" or any other operator instead of ","
if (x , y) { /* ... */} // warning 24X: possible misuse of comma operator

// The "Func()" part of the expression may have a side effect; shouldn't warn in this case
if (Func(), x)  { /* ... */}

• variable "%s" used in loop condition not modified in loop body
  Detects situations when the user forgot to add an increment for the loop counter variable, thus making the loop endless:

new index = 0;
while (index < 10) // warning 2XX: variable "index" used in loop condition not modified in loop body
    printf("%d", index);

• none of the variables used in loop condition are modified in loop body
  Mostly the same as the previous diagnostic, except that it works when multiple variables are used in the loop condition.

new i = 0, m = 10;
while (i < m) // warning 2XX: none of the variables used in loop condition are modified in loop body
    printf("%d", i);

The problem is that usually in such situations only one of the variables needs to be modified, but the compiler can't know which one.
This new warning solves the issue by only telling that none of the variables were modified and allowing the user to decide which variable should be modified and which shouldn't.

Workspace Information:

• Compiler version:
• Command line arguments provided (or sampctl version):
• Operating System:

[Daniel-Cortez]

Should I implement those warnings exactly as described? Would be nice to have any feedback before I get to implementing them.

[Daniel-Cortez]

Update: Added an idea for a new warning:

• enum increment "%s" has no effect on zero value
  Might be useful if the user defined bit flags, but forgot to specify the value for the first enum item (or specified 0 instead of 1 by mistake):

enum ePlayerFlags (<<= 1)
{
    pfEmailConfirmed,    // 0
    pfTutorialCompleted, // enum increment "<<= 1" has no effect on zero value
};

Any feedback is welcome.

[Daniel-Cortez]

Added ideas for two new diagnostics:

• variable "%s" used in loop condition not modified in loop body
  Detects situations when the user forgot to add an increment for the loop counter variable, thus making the loop endless:

new index = 0;
while (index < 10) // warning 2XX: variable "index" used in loop condition not modified in loop body
    printf("%d", index);

• none of the variables used in loop condition are modified in loop body
  Mostly the same as the previous diagnostic, except that it works when multiple variables are used in the loop condition.

new i = 0, m = 10;
while (i < m) // warning 2XX: none of the variables used in loop condition are modified in loop body
    printf("%d", i);

The problem is that usually in such situations only one of the variables needs to be modified, but the compiler can't know which one.
This new warning solves the issue by only telling that none of the variables were modified and allowing the user to decide which variable should be modified and which shouldn't.