[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

2

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

11

Spaces in template arguments should be removed.

13

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

16

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

17

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

34

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

48

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"?

60

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

66

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()".

85

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.

111

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

116

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

test/Analysis/label-inside-switch.c

2

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

6

Space after switch. Same below

[kromanenkov]

A few comments.

lib/StaticAnalyzer/Checkers/LabelInsideSwitchChecker.cpp

20

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

25

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

60

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

81

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

25

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

25

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

60

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

66

Looks like it can be replaced by StringRef::edit_distance

81

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.

87

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

95

Typo: Of course should be "sense" instead

116

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

1

Check for label inside switch

11

misprinting

19

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

87

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?

116

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

87

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

+//== LabelInsideSwitchChecker.cpp - Lable inside switch checker -*- C++ -*--==//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This defines LabelInsideSwitchChecker, which looks for typos in switch
+// cases like missprinting 'defualt', 'cas' or other accidental insertion
+// of labeled statement.
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+
+using namespace clang;
+using namespace ento;
+using namespace ast_matchers;
+
+namespace {
+class LabelInsideSwitchChecker : public Checker<check::ASTCodeBody> {
+public:
+  void checkASTCodeBody(const Decl *D, AnalysisManager &Mgr,
+                        BugReporter &BR) const;
+};
+
+static StringRef suggestTypoFix(StringRef Str) {
+  StringRef DefaultStr("default");
+  const unsigned DefaultStrED =
+      Str.edit_distance(DefaultStr, true, DefaultStr.size() / 2);
+
+  if (DefaultStrED <= DefaultStr.size() / 2)
+    return DefaultStr;
+
+  StringRef CaseStr("case");
+  const unsigned CaseStrED =
+      Str.edit_distance(CaseStr, true, CaseStr.size() / 2);
+
+  if (CaseStrED <= CaseStr.size() / 2)
+    return CaseStr;
+
+  return StringRef();
+}
+
+class Callback : public MatchFinder::MatchCallback {
+  const CheckerBase *C;
+  BugReporter &BR;
+  AnalysisDeclContext *AC;
+
+  void report(const LabelStmt *L) {
+    StringRef Suggest = suggestTypoFix(L->getName());
+
+    SmallString<64> Message;
+    llvm::raw_svector_ostream OS(Message);
+    OS << "label inside switch";
+    if (!Suggest.empty())
+      OS << " (did you mean '" << Suggest << "'?)";
+
+    PathDiagnosticLocation Loc =
+        PathDiagnosticLocation::createBegin(L, BR.getSourceManager(), AC);
+    BR.EmitBasicReport(AC->getDecl(), C, "Labeled statement inside switch",
+                       categories::LogicError, Message, Loc,
+                       L->getSourceRange());
+  }
+
+public:
+  Callback(const CheckerBase *C, BugReporter &BR, AnalysisDeclContext *AC)
+      : C(C), BR(BR), AC(AC) {}
+
+  virtual void run(const MatchFinder::MatchResult &Result) override {
+    if (const LabelStmt *L = Result.Nodes.getNodeAs<LabelStmt>("label"))
+      report(L);
+    if (const LabelStmt *L = Result.Nodes.getNodeAs<LabelStmt>("label_in_case"))
+      report(L);
+  }
+};
+} // end anonymous namespace
+
+void LabelInsideSwitchChecker::checkASTCodeBody(const Decl *D,
+                                                AnalysisManager &Mgr,
+                                                BugReporter &BR) const {
+  auto LabelStmt = stmt(hasDescendant(switchStmt(
+      eachOf(has(compoundStmt(forEach(labelStmt().bind("label")))),
+             forEachSwitchCase(forEach(labelStmt().bind("label_in_case")))))));
+
+  MatchFinder F;
+  Callback CB(this, BR, Mgr.getAnalysisDeclContext(D));
+
+  F.addMatcher(LabelStmt, &CB);
+  F.match(*D->getBody(), Mgr.getASTContext());
+}
+
+namespace clang {
+namespace ento {
+void registerLabelInsideSwitchChecker(CheckerManager &Mgr) {
+  Mgr.registerChecker<LabelInsideSwitchChecker>();
+}
+} // namespace ento
+} // namespace clang

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;
+}
+
+int unreachableLabelInSwitch(int arg) {
+  switch (arg) {
+  unreachable: // expected-warning {{label inside switch}}
+  case 1:
+    return 0;
+  default:
+    return arg;
+  }
+}