pawn-lang / compiler

Pawn compiler for SA-MP with bug fixes and new features - runs on Windows, Linux, macOS
Other
306 stars 72 forks source link

Unreachable code after `switch` is not being detected #560

Closed Daniel-Cortez closed 3 years ago

Daniel-Cortez commented 4 years ago

Issue description:

Currently the compiler isn't able to detect unreachable code after switch statements when all cases (including default) end with return.

Func(x) return x;
main()
{
    new x = 0;
    switch (x)
    {
        case 0: return Func(0);
        default: return Func(1);
    }
    return Func(2); // no warning(s), even though
                    // the code on this line is unreachable
}

Since the compiler already can detect unreachable code when both if branches end with return

main()
{
    new x = 0;
    if (x)
        return 0;
    else
        return 1;
    return 2; // warning 225: unreachable code
}

, I think it would make sense to adopt the same for switch.

Minimal complete verifiable example (MCVE):

See above.

Workspace Information:

Daniel-Cortez commented 4 years ago

I already had this feature implemented locally for a while and just got the time to test it on a few gamemodes. Turns out there might be a lot of cases like the following one:

stock IsWeaponHandgun(weaponid) {
    switch(weaponid) {
        case 2..8: return true;
        case 10..24: return true;
        default: return false;
    }
    return false; // warning 225: unreachable code
}

(source: https://github.com/NextGenerationGamingLLC/SA-MP-Development/blob/a059abaebda0f2dff3c2e0f95ee4c9c5696406c7/GMs/includes/core/weapons.pwn#L124-L131) I think originally the author of this code might have intended to write this code in the following way:

stock IsWeaponHandgun(weaponid) {
    switch(weaponid) {
        case 2..8: return true;
        case 10..24: return true;
        default: return false;
    }
}

, without the return false; line at the end, but then the compiler warned them about the function not returning a value on all paths (warning 209: function "%s" should return a value), in response to which the author copy-pasted the code from default: to the end of function, although they could simply move it there, without duplicating it (thus eliminating the default case):

stock IsWeaponHandgun(weaponid) {
    switch(weaponid) {
        case 2..8: return true;
        case 10..24: return true;
    }
    return false;
}

On one hand the compiler is correct to warn the user in such situations, as it's their fault for duplicating their code, on the other hand those situations are nothing serious (the only drawback I can see is that this results in dead code, which is just a couple instructions: const.pri and retn), and at the same time they are not uncommon - for some gamemode scripts they might make about 60-90% of new warning 225 messages (for example, I counted 14 of them in NGRP, including the example above), even though they're very easy to fix.

Should I disable warning 225 for situations when return <constant value> immediately follows a switch statement, or rather leave the diagnostic as is? @Y-Less?

Daniel-Cortez commented 3 years ago

Fixed in #580.