RFC redundant assignment in switch

Hello!

I am working on a new checker for the StaticAnalyser.

The purpose is to detect unintentional switch case fall through. The checker will warn when there is redundant assignment. Example code:

    switch (x) {
    case 1:
       y = 1; // <- redundant assignment
    case 2:
       y = 2;
       break;
    }

For your info, Cppcheck has this check. It has detected real bugs in real code. There are such cppcheck warnings in emacs,fvwm,etc. here is for example the code cppcheck warns about in fvwm:

  case TEXTURE_BUILTIN:
    Builtin:
    TextureType=TEXTURE_BUILTIN;
   default:
    Solid:
    TextureType=TEXTURE_SOLID;

I would like to discuss the design.

I attach a proof-of-concept checker.

In the proof-of-concept code, variables that are assigned are tracked using a DenseMap. If there is no break and a variable is reassigned in the next case there will be a warning.

I would like to know if I use the proper visitors and good approach for tracking assigned variables, etc.

Best regards,
Daniel Marjamäki

..................................................................................................................
Daniel Marjamäki Senior Engineer
Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden

Mobile: +46 (0)709 12 42 62
E-mail: Daniel.Marjamaki@evidente.se

www.evidente.se

redundantassignment1.patch (13.4 KB)

Hi, Daniel. We already have this in Clang itself as -Wimplicit-fallthrough, but it's only on in C++11 mode, where it can be silenced by adding "[[clang::fallthrough]];". There's a recent thread on cfe-dev about how to extend this to other language modes that don't have statement attributes; I'd suggest chiming in there.

Jordan

Hello!

I do know about that checker.

Personally I don't like that approach.

I don't like cluttering my code with comments that are not there to make the code easier to read/understand.

I don't think suppression comments should be required to avoid false positives.

But comments to avoid false negatives is ok/good. For instance annotations of utility functions.

What do you think about using this design goal in the static analyser?

Best regards,
Daniel Marjamäki

..................................................................................................................
Daniel Marjamäki Senior Engineer
Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden

Mobile: +46 (0)709 12 42 62
E-mail: Daniel.Marjamaki@evidente.se

www.evidente.se

That warning triggers for every fallthrough. Daniel’s checker only triggers for assignments that are redundant with assignments made in cases that are fallen into.

These are philosophically different approaches, and they are mutually compatible. Even if -Wimplicit-fallthrough becomes useful to C/ObjC programmers via the __fallthrough macro, Daniel’s checker is still useful because it will catch logic bugs in code that redundantly assigns to the same variable in two different cases.

--Kyle Sluder

Ah, interesting. Isn’t this already caught with the dead stores checker, though?

:5:5: warning: Value stored to ‘y’ is never read
y = 1;
^ ~

We could probably stand to improve that checker to say why something’s a dead store, but I don’t think we need a separate pass.

Jordan

Agreed. I’d rather we have the general dead stores checker (and make it better) then have something more specialized like this. The dead stores checker has also been hardened over several years to handle many common idioms of defensive code that are “dead” but not interesting.

Speaking as someone who has tried to use the static analyser on a largish code base in the recent past, I would much rather see a separate checker. We have literally hundreds of false positives from dead stores and actively ignore them. A “dead store due to fall-through case” on the other hand is likely a bug and is thus very interesting. To be clear, I’m most commenting on reporting interface not implementation. Philip

I know there has been discussion about making it possible to ignore
specific analyzer warnings, which seems like a better way to address
your concern than to have overlapping checkers.

--Kyle Sluder

At the moment, the class of errors reported by dead store has such a high noise rate, we're not interested in manually tagging them. We completely ignore the entire category.

Philip