[analyser] different.LabelInsideSwitch checker implementation

[alexey.knyshev]

Summary

Implementation of checker "different.LabelInsideSwitch" from potential checkers list (https://clang-analyzer.llvm.org/potential_checkers.html#different)

[a.sidorin]

Hello Alexey,

Thank you for the patch. I have made a preliminary review and will add some other reviewers. You can find my comments inline.

lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp

1

License header and checker descriptions are missed here. You can take a look at any other checker to find how it should be filled.

10

Spaces in template arguments should be removed.

12

LLVM Coding Standard requires variables to start with capital letter: 'Mgr'. Please change such names here and below.

15

We usually pass StringRef by value, not by reference. Same below.

16

LLVM Coding Standard requires method names to start with small letter.

33

Obvious comment. I think it is better to remove it.

47

1. The longest string we can get here is "label inside switch (did you mean 'default'?)". It is 46 char long so we can reduce the allocation size to 48 or 64.
2. Should we rename the variable to "Message"?

59

raw_svector_ostream is always synchronized with the string it prints to so we can just pass the message string instead of calling .str().

65

Seems like you are trying to implement some kind of "similarity" metric. Consider using StringRef::edit_distance() instead. I guess, in your case it will give you much more freedom to vary different match parameters. For example, it can consider replacements.
Also, I think it's better to rename this function into something like "areStringsSimilar()".

84

1. The indent looks broken here. You can use clang-format tool to automatically make your code formatted as needed.
2. How about just returning StringRef? We can return empty StringRef (return StringRef()) if no matches found and return a non-empty value if match was found.

110

This line exceeds 80-char limit. Please split it.

115

You can write just `void ento::registerLabelInsideSwitchChecker(CheckerManager &Mgr) {

test/Analysis/label-inside-switch.c

1

Not sure if we need to enable 'core' while testing path-insensitive checkers. Artem?

5

Space after switch. Same below

[kromanenkov]

A few comments.

lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp

19

Maybe I miss something, but do not we return StringRef to temporary string going out of scope here? Same below.

24

Do you consider using ASTMatchers like in NumberObjectConversionChecker instead of manually traversing the AST?

59

You could use S->getSourceRange() instead of Sr, as llvm::ArrayRef in EmitBasicReport() could be constructed even from the 0 consecutively elements.

80

Maybe so?
size_t MinSize = std::min(StrSize, ExpSize);
size_t SizeDelta = std::labs(StrSize, ExpSize);

[alexey.knyshev]

1. Now implemented via MatchFinder
2. Added missing License header
3. Pass all StringRefs by value
4. Method names now start from small letter
5. Using StringRef::edit_distance instead of custom "similarity" metric
6. Various other formating fixes

[alexey.knyshev]

lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp

24

Haven't seen it before but surely will consider to use it. Looks like I should use StatementMatcher, shouldn't I?

24

One more thing. After reviewing implementation of NumberObjectConversionChecker I'm not sure if it's more clear to use Matcher + Callback there.

59

Is there way to getSourceRange() which doesn't include following comment (if it exists)?
Currently source range marks comment string in addition to LabelStmt.

65

Looks like it can be replaced by StringRef::edit_distance

80

SizeDelta is abs(StrSize - ExpSize) from logical point of view. But if StrSize < ExpSize subtraction ExpSize from StrSize will underflow. Anyway I'm going to replace whole function with existing implementation StringRef::edit_distance.

88

Looks like I have to use forEachDescendant instead of hasDescendant. Please, comment!

94

Typo: Of course should be "sense" instead

115

Actually I can't, get error:

/llvm/tools/clang/lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp:103:68: error: 'void clang::ento::registerLabelInsideSwitchChecker(clang::ento::CheckerManager&)' should have been declared inside 'clang::ento'
     void ento::registerLabelInsideSwitchChecker(CheckerManager &Mgr) {

[a.sidorin]

Hello Alexey,

Thank you for the update. The code looks much cleaner now.

lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp

2

Check for label inside switch

12

misprinting

20

We don't use CheckerContext in the code below. Looks like this include is redundant or should be replaced by more relevant include files.

88

1. Yes, hasDescendant() will give you only single match. forEachDescendant() will continue matching after match found and that is what we should do here.
2. Maybe we can just use stmt(forEachDescendant(switchStmt(forEachDescendant(labelStmt()))))? We don't distinguish "label" and "label_in_case" anyway. Also, current matcher will ignore deeply-nested switches:

switch (x) }
case 1: {
  {{ label: }} // ignored
}
}

Is that intentional?

115

This looks strange. I can remember that I have met this issue but I cannot remember how I resolved it.

[alexey.knyshev]

lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp

88

1. Thanks, got it.
2. I've used eachOf for matching 2 different cases:
   • Label under case stmt such as:

switch (x) {
case 1:
someCodeHere;
cas: // missprint
... // other cases
}

• And label under switch compound stmt:

switch (x) {
cas: blabla; // this label is not a child of any case stmt
case 3: ...
...
// etc

On the other hand I would like to avoid matching deeply-nested labels inside switch compound stmt for reducing false-positives probability. Actually I'm not sure in my assumption.

[dcoughlin]

Thanks for looking into this!

This checker is in the 'core' package, which means (when moved out of alpha) it will be enabled by default.

• Do you think that this checker should be enabled by default for all users of the analyzer?
• If users do actually want to use labels in their switch statements, how should they suppress the diagnostics from the checker?
• What is the benefit of adding this check in the static analyzer vs. in clang-tidy?

(My own sense is that the check for labels that are close to "default" could be on by default but that warning on *any* label inside a switch is more stylistic. I think users should have to opt in to that check.)

[alexey.knyshev]

In D40715#951665, @dcoughlin wrote:

Thanks for looking into this!

This checker is in the 'core' package, which means (when moved out of alpha) it will be enabled by default.

• Do you think that this checker should be enabled by default for all users of the analyzer?

I think so

• If users do actually want to use labels in their switch statements, how should they suppress the diagnostics from the checker?

Good point, is there recommended way to implement options for checker? Where can I find any reference example?

• What is the benefit of adding this check in the static analyzer vs. in clang-tidy?

(My own sense is that the check for labels that are close to "default" could be on by default but that warning on *any* label inside a switch is more stylistic. I think users should have to opt in to that check.)

It makes sense. So, I can make generic case when we found any label in swichStmt opt-in (default=off) and left cases when it looks like typo in 'case' or 'default' keywords enabled by default.
Thanks!

include/clang/StaticAnalyzer/Checkers/Checkers.td

   HelpText<"Warn about unintended use of identical expressions in operators">,
   DescFile<"IdenticalExprChecker.cpp">;
 
+def LabelInsideSwitchChecker : Checker<"LabelInsideSwitch">,
+  HelpText<"Warn about labeled statements inside switch">,
+  DescFile<"LabelInsideSwitchChecker.cpp">;
+
 def FixedAddressChecker : Checker<"FixedAddr">,
   HelpText<"Check for assignment of a fixed address to a pointer">,
   DescFile<"FixedAddressChecker.cpp">;

lib/StaticAnalyzer/Checkers/CMakeLists.txt

   IteratorChecker.cpp
   IvarInvalidationChecker.cpp
   LLVMConventionsChecker.cpp
+  LabelInsideSwitchChecker.cpp
   LocalizationChecker.cpp
   MacOSKeychainAPIChecker.cpp
   MacOSXAPIChecker.cpp

lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp

+#include "clang/AST/StmtVisitor.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+  class LabelInsideSwitchChecker : public Checker< check::ASTCodeBody > {
+  public:
+    void checkASTCodeBody(const Decl *D, AnalysisManager &mgr, BugReporter &BR) const;
+  };
+
+  static std::pair<StringRef, bool> Suggest(const StringRef &Str, StringRef Exp);
+  static std::pair<StringRef, bool> SuggestTypoFix(const StringRef &Str) {
+    const auto S = Suggest(Str, "default");
+    if (S.second)
+      return S;
+
+    return Suggest(Str, "case");
+  }
+
+  class WalkAST : public ConstStmtVisitor<WalkAST, void, bool> {
+    const CheckerBase *Checker;
+    BugReporter &BR;
+    AnalysisDeclContext *AC;
+
+  public:
+    WalkAST(const CheckerBase *Checker, BugReporter &BR, AnalysisDeclContext *AC)
+        : Checker(Checker), BR(BR), AC(AC) {}
+
+    // Statement visitor methods.
+    void VisitChildren(const Stmt *S, bool InSwitch) {
+      for (const Stmt *Child : S->children())
+        if (Child)
+          Visit(Child, InSwitch);
+    }
+
+    void VisitStmt(const Stmt *S, bool InSwitch) {
+      if (!InSwitch && isa<SwitchStmt>(S)) {
+        InSwitch = true;
+      } else if (InSwitch && isa<LabelStmt>(S)) {
+        const LabelStmt *L = cast<LabelStmt>(S);
+        const auto Sug = SuggestTypoFix(L->getName());
+
+        SmallString<256> SBuf;
+        llvm::raw_svector_ostream OS(SBuf);
+        OS << "label inside switch";
+        if (Sug.second) {
+          OS << " (did you mean '" << Sug.first << "'?)";
+        }
+
+        PathDiagnosticLocation Loc =
+          PathDiagnosticLocation::createBegin(S, BR.getSourceManager(), AC);
+        SourceRange Sr[1];
+        Sr[0] = S->getSourceRange();
+        BR.EmitBasicReport(AC->getDecl(), Checker, "Labeled statement inside switch",
+                           categories::LogicError, OS.str(), Loc, Sr);
+      }
+      VisitChildren(S, InSwitch);
+    }
+  };
+
+  std::pair<StringRef, bool> Suggest(const StringRef &Str, StringRef Exp) {
+    const size_t StrSize = Str.size();
+    const size_t ExpSize = Exp.size();
+
+    assert(StrSize);
+    assert(ExpSize);
+
+    size_t SizeDelta = 0;
+    size_t MinSize = StrSize;
+
+    if (StrSize < ExpSize) {
+      SizeDelta = ExpSize - StrSize;
+    } else if (StrSize > ExpSize) {
+      SizeDelta = StrSize - ExpSize;
+      MinSize = ExpSize;
+    }
+
+    // We only accept size delta <= 1
+    if (SizeDelta > 1)
+        return std::make_pair("", false);
+
+    size_t SameSymCount = 0;
+    for (size_t i = 0; i < MinSize; i++)
+      if (Str[i] == Exp[i])
+        SameSymCount++;
+
+    if (SameSymCount > 0 && SameSymCount - SizeDelta >= ExpSize / 2)
+      return std::make_pair(Exp, true);
+
+    // Str & Exp have the same size. No sence to check from back to front
+    if (SizeDelta == 0)
+      return std::make_pair("", false);
+
+    SameSymCount = 0;
+    for (size_t i = StrSize, j = ExpSize; i > 0 && j > 0; i--, j--)
+    if (Str[i - 1] == Exp[j - 1])
+      SameSymCount++;
+
+    if (SameSymCount > 0 && SameSymCount - SizeDelta >= ExpSize / 2)
+      return std::make_pair(Exp, true);
+
+    return std::make_pair("", false);
+  }
+} // end anonymous namespace
+
+void LabelInsideSwitchChecker::checkASTCodeBody(const Decl *D, AnalysisManager &mgr, BugReporter &BR) const {
+  WalkAST walker(this, BR, mgr.getAnalysisDeclContext(D));
+  walker.Visit(D->getBody(), /*InSwitch=*/ false);
+}
+
+namespace clang {
+  namespace ento {
+    void registerLabelInsideSwitchChecker(CheckerManager &mgr) {
+      mgr.registerChecker<LabelInsideSwitchChecker>();
+    }
+  } // end ento namespace
+} // end clang namespace

test/Analysis/label-inside-switch.c

+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core.LabelInsideSwitch -w -verify %s
+
+int caseAnddefaultMissprint(unsigned count) {
+  int res = 0;
+  switch(count) {
+  case 1:
+    res = 1;
+    break;
+  cas: // expected-warning {{label inside switch (did you mean 'case'?)}}
+  case 2:
+    res = 5;
+    break;
+  case 3:
+    exit(1);
+  defaul: // expected-warning {{label inside switch (did you mean 'default'?)}}
+    return -1;
+  }
+
+  return res;
+}
+
+int nestedSwitch(unsigned x, unsigned y) {
+  int res = 0;
+  switch (x) {
+  case 1:
+    res = 1;
+    break;
+  case 2:
+    res = 5;
+    break;
+  case 3:
+    switch(y) {
+    case 1:
+    case 2:
+    case 4:
+      res = x * y;
+      break;
+    defaul:; // expected-warning {{label inside switch (did you mean 'default'?)}}
+    }
+    break;
+  default:;
+  }
+
+  return res;
+}
+
+int arbitaryLabelInSwitch(int arg) {
+  switch (arg)  {
+  case 1:
+  case 2:
+  label: // expected-warning {{label inside switch}}
+  case 3:
+    break;
+  }
+
+  return 0;
+}