Bug 1485588 - Part 1: Clang-plugin checker for locale-specific functi…

…ons. r=hsivonen.

They are warnings, not errors, because of the many existing uses of these
functions throughout the codebase.

Differential Revision: https://phabricator.services.mozilla.com/D81785

build/clang-plugin/Checks.inc

@@ -25,6 +25,7 @@ CHECK(NoAddRefReleaseOnReturnChecker, "no-addref-release-on-return")
 CHECK(NoAutoTypeChecker, "no-auto-type")
 CHECK(NoDuplicateRefCntMemberChecker, "no-duplicate-refcnt-member")
 CHECK(NoExplicitMoveConstructorChecker, "no-explicit-move-constructor")
+CHECK(NoLocaleSpecificChecker, "no-locale-specific-checker")
 CHECK(NoNewThreadsChecker, "no-new-threads")
 CHECK(NonMemMovableMemberChecker, "non-memmovable-member")
 CHECK(NonMemMovableTemplateArgChecker, "non-memmovable-template-arg")

build/clang-plugin/ChecksIncludes.inc

@@ -26,6 +26,7 @@
 #include "NoAutoTypeChecker.h"
 #include "NoDuplicateRefCntMemberChecker.h"
 #include "NoExplicitMoveConstructorChecker.h"
+#include "NoLocaleSpecificChecker.h"
 #include "NoNewThreadsChecker.h"
 #include "NonMemMovableMemberChecker.h"
 #include "NonMemMovableTemplateArgChecker.h"

build/clang-plugin/NoLocaleSpecificChecker.cpp

@@ -0,0 +1,107 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#include "NoLocaleSpecificChecker.h"
+#include <algorithm>
+#include "CustomMatchers.h"
+
+void NoLocaleSpecificChecker::registerMatchers(MatchFinder *AstMatcher) {
+  auto hasCharPtrParam = [](const unsigned int Position) {
+    return hasParameter(
+        Position, hasType(hasCanonicalType(pointsTo(asString("char")))));
+  };
+  auto hasConstCharPtrParam = [](const unsigned int Position) {
+    return hasParameter(
+        Position, hasType(hasCanonicalType(pointsTo(asString("const char")))));
+  };
+  auto hasIntegerParam = [](const unsigned int Position) {
+    return hasParameter(Position, hasType(isInteger()));
+  };
+  auto hasNameAndIntegerParam = [&](const char* name) {
+    return allOf(hasName(name), hasIntegerParam(0));
+  };
+
+  AstMatcher->addMatcher(
+      declRefExpr(
+          allOf(
+              isFirstParty(),
+              to(functionDecl(allOf(
+                  isInSystemHeader(),
+                  anyOf(
+                      hasNameAndIntegerParam("isalnum"),
+                      hasNameAndIntegerParam("isalpha"),
+                      hasNameAndIntegerParam("isblank"),
+                      hasNameAndIntegerParam("iscntrl"),
+                      hasNameAndIntegerParam("isdigit"),
+                      hasNameAndIntegerParam("isgraph"),
+                      hasNameAndIntegerParam("islower"),
+                      hasNameAndIntegerParam("isprint"),
+                      hasNameAndIntegerParam("ispunct"),
+                      hasNameAndIntegerParam("isspace"),
+                      hasNameAndIntegerParam("isupper"),
+                      hasNameAndIntegerParam("isxdigit"),
+                      allOf(hasName("strcasecmp"),
+                            hasConstCharPtrParam(0),
+                            hasConstCharPtrParam(1)),
+                      allOf(hasName("strncasecmp"),
+                            hasConstCharPtrParam(0),
+                            hasConstCharPtrParam(1),
+                            hasIntegerParam(2)),
+                      allOf(hasName("strcoll"),
+                            hasConstCharPtrParam(0),
+                            hasConstCharPtrParam(1)),
+                      hasNameAndIntegerParam("strerror"),
+                      allOf(hasName("strxfrm"),
+                            hasCharPtrParam(0),
+                            hasConstCharPtrParam(1),
+                            hasIntegerParam(2)),
+                      hasNameAndIntegerParam("tolower"),
+                      hasNameAndIntegerParam("toupper")
+                  )
+              )))
+          )
+      ).bind("id"),
+      this);
+}
+
+struct LocaleSpecificCheckerHint {
+    const char* name;
+    const char* message;
+};
+
+static const LocaleSpecificCheckerHint Hints[] = {
+    { "isalnum", "Consider mozilla::IsAsciiAlphanumeric instead." },
+    { "isalpha", "Consider mozilla::IsAsciiAlpha instead." },
+    { "isdigit", "Consider mozilla::IsAsciiDigit instead." },
+    { "islower", "Consider mozilla::IsAsciiLowercaseAlpha instead." },
+    { "isspace", "Consider mozilla::IsAsciiWhitespace instead." },
+    { "isupper", "Consider mozilla::IsAsciiUppercaseAlpha instead." },
+    { "isxdigit", "Consider mozilla::IsAsciiHexDigit instead." },
+    { "strcasecmp",
+      "Consider mozilla::CompareCaseInsensitiveASCII "
+      "or mozilla::EqualsCaseInsensitiveASCII instead." },
+    { "strncasecmp",
+      "Consider mozilla::CompareCaseInsensitiveASCII "
+      "or mozilla::EqualsCaseInsensitiveASCII instead." },
+};
+
+void NoLocaleSpecificChecker::check(const MatchFinder::MatchResult &Result) {
+  const auto *MatchedDecl = Result.Nodes.getNodeAs<DeclRefExpr>("id");
+  diag(MatchedDecl->getExprLoc(),
+       "Do not use locale-specific standard library functions.",
+       DiagnosticIDs::Warning);
+
+  // Also emit a relevant hint, if we have one.
+  std::string Name = MatchedDecl->getNameInfo().getName().getAsString();
+  const LocaleSpecificCheckerHint* HintsEnd = Hints + sizeof(Hints) / sizeof(Hints[0]);
+  auto Hit = std::find_if(
+    Hints,
+    HintsEnd,
+    [&](const LocaleSpecificCheckerHint& hint) {
+      return strcmp(hint.name, Name.c_str()) == 0;
+    });
+  if (Hit != HintsEnd) {
+    diag(MatchedDecl->getExprLoc(), Hit->message, DiagnosticIDs::Note);
+  }
+}

build/clang-plugin/NoLocaleSpecificChecker.h

@@ -0,0 +1,18 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#ifndef NoLocaleSpecificChecker_h__
+#define NoLocaleSpecificChecker_h__
+
+#include "plugin.h"
+
+class NoLocaleSpecificChecker : public BaseCheck {
+public:
+  NoLocaleSpecificChecker(StringRef CheckName, ContextType *Context = nullptr)
+      : BaseCheck(CheckName, Context) {}
+  void registerMatchers(MatchFinder *AstMatcher) override;
+  void check(const MatchFinder::MatchResult &Result) override;
+};
+
+#endif

build/clang-plugin/moz.build

@@ -29,6 +29,7 @@ HOST_SOURCES += [
     'NoAutoTypeChecker.cpp',
     'NoDuplicateRefCntMemberChecker.cpp',
     'NoExplicitMoveConstructorChecker.cpp',
+    'NoLocaleSpecificChecker.cpp',
     'NoNewThreadsChecker.cpp',
     'NonMemMovableMemberChecker.cpp',
     'NonMemMovableTemplateArgChecker.cpp',

build/clang-plugin/tests/TestNoLocaleSpecificChecker.cpp

@@ -0,0 +1,29 @@
+// Locale-specific standard library functions are banned.
+
+#include <ctype.h>
+#include <errno.h>
+#include <stdio.h>
+#include <string.h>
+#include <algorithm>
+#include <string>
+
+int useLocaleSpecificFunctions() {
+  if (isalpha('r')) {  // expected-warning{{locale-specific}} expected-note {{IsAsciiAlpha instead}}
+    return 1;
+  }
+  if (isalnum('8')) {  // expected-warning{{locale-specific}} expected-note {{IsAsciiAlphanumeric instead}}
+    return 1;
+  }
+  std::string s("8167");
+  if (std::find_if(s.begin(), s.end(), isdigit) != s.end()) {  // expected-warning{{locale-specific}} expected-note {{IsAsciiDigit instead}}
+    return 1;
+  }
+
+  if (strcasecmp(s.c_str(), "81d7") == 0) { // expected-warning{{locale-specific}} expected-note {{CompareCaseInsensitiveASCII}}
+      return 1;
+  }
+
+  printf("problems: %s\n", strerror(errno)); // expected-warning{{locale-specific}}
+
+  return 0;
+}

build/clang-plugin/tests/TestNoLocaleSpecificChecker2.cpp

@@ -0,0 +1,54 @@
+// Like TestNoLocaleSpecificChecker.cpp, but using the standard C++ <cctype> and <locale> headers.
+
+#include <algorithm>
+#include <cctype>
+#include <cerrno>
+#include <cstring>
+#include <locale>
+#include <string>
+
+int useLocaleSpecificFunctions() {
+  if (isalpha('r')) {  // expected-warning{{locale-specific}} expected-note {{IsAsciiAlpha instead}}
+    return 1;
+  }
+  using std::isalnum;
+  if (isalnum('8')) {  // expected-warning{{locale-specific}} expected-note {{IsAsciiAlphanumeric instead}}
+    return 1;
+  }
+  std::string s("8167");
+  if (std::find_if(s.begin(), s.end(), isdigit) != s.end()) {  // expected-warning{{locale-specific}} expected-note {{IsAsciiDigit instead}}
+    return 1;
+  }
+  typedef int (*PredicateType)(int);
+  PredicateType predicate = std::isdigit;  // expected-warning{{locale-specific}} expected-note {{IsAsciiDigit instead}}
+
+  if (strcasecmp(s.c_str(), "81d7") == 0) { // expected-warning{{locale-specific}} expected-note {{CompareCaseInsensitiveASCII}}
+      return 1;
+  }
+
+  printf("problems: %s\n", strerror(errno)); // expected-warning{{locale-specific}}
+
+  return 0;
+}
+
+using std::isalnum;
+
+int useStdLocaleFunctions() {
+  std::locale l;
+
+  if (std::isalpha('r', l)) {  // expected-warning{{locale-specific}} expected-note {{IsAsciiAlpha instead}}
+    return 1;
+  }
+
+  if (isalnum('8', l)) {  // expected-warning{{locale-specific}} expected-note {{IsAsciiAlphanumeric instead}}
+    return 1;
+  }
+
+  using std::isdigit;
+  std::string s("8167");
+  if (std::find_if(s.begin(), s.end(), [&](char c) { return isdigit(c, l); }) != s.end()) {  // expected-warning{{locale-specific}} expected-note {{IsAsciiDigit instead}}
+    return 1;
+  }
+
+  return 0;
+}

build/clang-plugin/tests/moz.build

@@ -30,6 +30,8 @@ SOURCES += [
     'TestNoAutoType.cpp',
     'TestNoDuplicateRefCntMember.cpp',
     'TestNoExplicitMoveConstructor.cpp',
+    'TestNoLocaleSpecificChecker.cpp',
+    'TestNoLocaleSpecificChecker2.cpp',
     'TestNoNewThreadsChecker.cpp',
     'TestNonHeapClass.cpp',
     'TestNonMemMovable.cpp',