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

[Kulpreet Chilana]

kulpreet marked 14 inline comments as done.
kulpreet added a comment.

Thanks for the feedback! Uploading a new diff with the changes.


================
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.
----------------
zaks.anna wrote:
> Is the comment argument optional in some cases or is it always encouraged in some contexts?
Engineers sometimes assume that a particular string alone will have enough context for the translators to figure out the meaning, but this usually a bad assumption. Similar to the way that it is "bad practice" to not have descriptive variable and function names (i.e. foo()), it's "bad practice" to not include a comment in a localized string. I modified my comment to clarify.

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

================
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
----------------
zaks.anna wrote:
> Please, use more descriptive names here (or add a comment on what these are).
> Why StringSet does not work?
Adding in comments. 

When I try to use StringSet I get:

`error: no type named 'StringSet' in namespace 'llvm';`

even when I explicitly #include "llvm/ADT/StringSet.h" 

================
Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:201
@@ +200,3 @@
+
+  ExplodedNode *ErrNode = C.addTransition();
+  if (!ErrNode)
----------------
zaks.anna wrote:
> 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.
I'm not 100% clear on why I would want to create a new transition, but I adjusted to code so it does something similar to MallocChecker. The alternative is to just call getPredecessor() which seems to work the same?

================
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);
----------------
zaks.anna wrote:
> 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.
Good idea. Adding this for arguments so it highlights the string.

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

================
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 =
----------------
zaks.anna wrote:
> 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?
> 
Adding a comment to the method signature, clarifying this.

================
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 {
----------------
zaks.anna wrote:
> Why are we not using the same heuristic as in the case with other calls?
I think we are? checkPostCall should also cover ObjC methods for the heuristic. This function is to specifically cover the case where we need to mark a value returned by a method in LocStringMethods (LSM) as localized.

================
Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:365
@@ +364,3 @@
+  StringRef stringValue = SL->getString()->getString();
+  SVal sv = C.getSVal(SL);
+  if (stringValue.empty()) {
----------------
zaks.anna wrote:
> Is it possible to see that we are dealing with an empty string from an SVal? That way you could keep the state lean.
Good idea. I was able to do this with the following:
```
if (const ObjCStringRegion *SR =
            dyn_cast_or_null<ObjCStringRegion>(svTitle.getAsRegion())) {
      StringRef stringValue =
          SR->getObjCStringLiteral()->getString()->getString();
      if (stringValue.empty())
        return;
    }
```
And then marking all string literals here as NonLocalized

================
Comment at: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp:450
@@ +449,3 @@
+  // an NSLocalizedString macro
+  SourceLocation SL =
+      Mgr.getSourceManager().getImmediateMacroCallerLoc(R.getBegin());
----------------
zaks.anna wrote:
> 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.
This should work for any macro for [NSBundle localizedStringForKey:Value:Table:] where the last argument holds the comment.

I have a test in localization-aggressive.m that checks for the macro expansion within a macro expansion and it seems to be working as expected. (i.e. giving a warning where the user wrote the code).

Adding in some comments to clarify your other questions.


http://reviews.llvm.org/D11572