[PATCH] D11572: [Static Analyzer] Checker for OS X / iOS localizability issues

[Anna Zaks]

zaks.anna added a comment.

Thanks for working on this!

Comments in line,
Anna.


================
Comment at: lib/StaticAnalyzer/Checkers/Checkers.td:461
@@ +460,3 @@
+  HelpText<"Check that warns about uses of non-localized NSStrings passed to UI methods expecting localized strings">,
+  DescFile<"LocalizationChecker.cpp">;
+
----------------
How about shortening the message to "Warn about.."

================
Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:13
@@ +12,3 @@
+//     UI methods expecting localized strings
+//  2) A syntactic checker that warns against not including a comment in
+//     NSLocalizedString macros.
----------------
Is the comment argument optional in some cases or is it always encouraged in some contexts?

================
Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:39
@@ +38,3 @@
+private:
+  enum Kind { Unknown, NonLocalized, Localized } K;
+  LocalizedState(Kind InK) : K(InK) {}
----------------
Unknown is never used?

================
Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:66
@@ +65,3 @@
+  mutable std::map<StringRef, std::map<StringRef, uint8_t>> UIMethods;
+  mutable llvm::SmallSet<std::pair<StringRef, StringRef>, 10> LSM;
+  mutable llvm::StringMap<char> LSF; // StringSet doesn't work
----------------
Please, use more descriptive names here (or add a comment on what these are).
Why StringSet does not work?

================
Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:201
@@ +200,3 @@
+
+  ExplodedNode *ErrNode = C.addTransition();
+  if (!ErrNode)
----------------
I believe this would just return a predecessor and not add a new transition.
If you want to create a new transition, you should tag the node with your checker info. For example, see CheckerProgramPointTag in the MallocChecker.

================
Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:208
@@ +207,3 @@
+      new BugReport(*BT, "String should be localized", ErrNode));
+  R->addRange(M.getSourceRange());
+  R->markInteresting(S);
----------------
You can try reporting a more specific range here, for example the range of the argument expression if available. This is what gets highlighted in Xcode.

================
Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:235
@@ +234,3 @@
+    // These special NSString methods draw to the screen
+    StringRef drawAtPoint("drawAtPoint");
+    StringRef drawInRect("drawInRect");
----------------
Are these temporaries needed?

================
Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:289
@@ +288,3 @@
+/// if it is in LocStringFunctions (LSF) or the function is annotated
+void NonLocalizedStringChecker::checkPostCall(const CallEvent &Call,
+                                              CheckerContext &C) const {
----------------
Or the heuristic is triggered, right?
This applies to C++ as well.

================
Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:322
@@ +321,3 @@
+    } else {
+      // Perhaps I can use a different heuristic for non-aggressive reporting?
+      const SymbolicRegion *SymReg =
----------------
Could you add a comment documenting what it is?
Is it that the literals other than the empty string are assumed to be non-localized?


================
Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:333
@@ +332,3 @@
+/// if it is in LocStringMethods or the method is annotated
+void NonLocalizedStringChecker::checkPostObjCMessage(const ObjCMethodCall &msg,
+                                                     CheckerContext &C) const {
----------------
Why are we not using the same heuristic as in the case with other calls?

================
Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:365
@@ +364,3 @@
+  StringRef stringValue = SL->getString()->getString();
+  SVal sv = C.getSVal(SL);
+  if (stringValue.empty()) {
----------------
Is it possible to see that we are dealing with an empty string from an SVal? That way you could keep the state lean.

================
Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:426
@@ +425,3 @@
+
+void EmptyLocalizationContextChecker::MethodCrawler::VisitObjCMessageExpr(
+    const ObjCMessageExpr *ME) {
----------------
Could you add a comment explaining what this does. For example, we try to match these macros and we assume they are defined in this way.. 

Also, the point here is that we cannot use the path sensitive check because the macro argument we are checking for is not used, so it only appears in the macro call, but not in the expansion.

#define NSLocalizedString(key, comment) \
        [[NSBundle mainBundle] localizedStringForKey:(key) value:@"" table:nil]
#define NSLocalizedStringFromTable(key, tbl, comment) \
        [[NSBundle mainBundle] localizedStringForKey:(key) value:@"" table:(tbl)]
#define NSLocalizedStringFromTableInBundle(key, tbl, bundle, comment) \
        [bundle localizedStringForKey:(key) value:@"" table:(tbl)]
#define NSLocalizedStringWithDefaultValue(key, tbl, bundle, val, comment) \
        [bundle localizedStringForKey:(key) value:(val) table:(tbl)]

================
Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:450
@@ +449,3 @@
+  // an NSLocalizedString macro
+  SourceLocation SL =
+      Mgr.getSourceManager().getImmediateMacroCallerLoc(R.getBegin());
----------------
Would this be susceptible to the macro definition changes?

What the while loop below is doing?

I don't think this will work if the expansion is inside of another macro expansion, so we should check for that case and not warn if that is the case.

================
Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:505
@@ +504,3 @@
+      MD, Checker, "Context Missing", "Localization Error",
+      "Localized string macro should include comment for translator",
+      PathDiagnosticLocation(ME, BR.getSourceManager(), DCtx));
----------------
"include comment" -> "include a non-empty comment"?


http://reviews.llvm.org/D11572