This is an archive of the discontinued LLVM Phabricator instance.

[Static Analyzer] Checker for OS X / iOS localizability issues
ClosedPublic

Authored by kulpreet on Jul 28 2015, 1:28 PM.

Details

Summary

This is the basis for a set of checks to detect code-level localizability issues for OS X / iOS including:

  1. A checker that warns about uses of non-localized NSStrings passed to UI methods expecting localized strings
  2. A syntax checker that warns against not including a comment in NSLocalizedString macros.

Diff Detail

Event Timeline

kulpreet updated this revision to Diff 30849.Jul 28 2015, 1:28 PM
kulpreet retitled this revision from to [Static Analyzer] Checker for OS X / iOS localizability issues.
kulpreet updated this object.
kulpreet added a subscriber: cfe-commits.
zaks.anna edited edge metadata.Jul 28 2015, 5:51 PM

Thanks for working on this!

Comments in line,
Anna.

lib/StaticAnalyzer/Checkers/Checkers.td
461

How about shortening the message to "Warn about.."

lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp
14

Is the comment argument optional in some cases or is it always encouraged in some contexts?

40

Unknown is never used?

67

Please, use more descriptive names here (or add a comment on what these are).
Why StringSet does not work?

202

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.

209

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.

236

Are these temporaries needed?

290

Or the heuristic is triggered, right?
This applies to C++ as well.

323

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?

334

Why are we not using the same heuristic as in the case with other calls?

366

Is it possible to see that we are dealing with an empty string from an SVal? That way you could keep the state lean.

427

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)]
451

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.

506

"include comment" -> "include a non-empty comment"?

kulpreet marked 14 inline comments as done.Jul 29 2015, 5:15 PM

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

lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp
14

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.

40

Removing Unknown from the enum.

67

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"

202

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?

209

Good idea. Adding this for arguments so it highlights the string.

236

No, these will be removed.

323

Adding a comment to the method signature, clarifying this.

334

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.

366

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

451

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.

kulpreet updated this revision to Diff 30971.Jul 29 2015, 5:16 PM
kulpreet edited edge metadata.
kulpreet marked 10 inline comments as done.

Including changes from Anna's comments.

zaks.anna added inline comments.Jul 29 2015, 5:50 PM
lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp
15

My quick Google search did not show any mention of this :(

203

Adding a new transition is more debugging friendly.

335

Ah, I see!

367

I am not sure if we are on the same page.. I would not store/mark this information in the state, but just use the code as part of the check.

452

The point is that this macro can be used inside another, user defined macro where you'd no longer know which argument corresponds to "comment". The only way around that that I see is to only warn when these macros are called directly.

kulpreet updated this revision to Diff 31181.Jul 31 2015, 5:32 PM
  • Added in a test for the case for macro expansion scenario that Anna pointed out.
  • Skipping warning on string literals that only contain whitespace in addition to empty string literals.
  • Ran on a large projects to verify low false-positive rate (will send details in a separate email to reviewers)
zaks.anna added inline comments.Aug 5 2015, 4:58 PM
test/Analysis/localization-aggressive.m
203

How about something like this, where the last argument to the user defined macro is nil, but it has noting to do with the comment.

#define POSSIBLE_FALSE_POSITIVE(s, myarg) YOLOC(s)

..
POSSIBLE_FALSE_POSITIVE(@"Hello", nil)

kulpreet marked an inline comment as done.Aug 6 2015, 2:21 PM
kulpreet added inline comments.
test/Analysis/localization-aggressive.m
203

I added a regression test for this and it seems to not produce a false positive. This makes sense because the Lexer will only lex through NSLocalizedString macros.

kulpreet updated this revision to Diff 31481.Aug 6 2015, 3:44 PM
kulpreet marked an inline comment as done.
  • By default (non-aggressive mode) the UnlocalizedStringChecker will ignore strings that are less than two characters long to avoid false positives such as @"-"
  • Added UIAlertView APIs to add coverage to methods that require localized strings.
  • Added in test case for EmptyLocalizationContext checker to show that it doesn't produce a false positive in the case that Anna pointed out.

Updated diff to include Anna's suggestions.

zaks.anna accepted this revision.Aug 6 2015, 11:22 PM
zaks.anna edited edge metadata.

Thank you! LGTM.

I'll commit this tomorrow.

This revision is now accepted and ready to land.Aug 6 2015, 11:22 PM

Committed in r244389.
Thank you!

kulpreet updated this revision to Diff 31894.Aug 11 2015, 6:17 PM
kulpreet edited edge metadata.

Moved UIMethods over to StringMap so it should compile on Windows now.

kulpreet updated this revision to Diff 32102.Aug 13 2015, 3:51 PM

Fixed memory corruption bug with temporary std::string - found with address sanitizer.

Now, all regression tests pass with MallocScribble on.

dcoughlin added inline comments.Aug 14 2015, 11:18 AM
lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp
68

FWIW,

#include "llvm/ADT/StringSet.h"

and using

mutable llvm::StringMap<llvm::StringSet<>> UIMethods;

works for me.

72

Is there a reason you use StringMap<char> here and StringMap<uint8_t> for UIMethods?

279

You can just write

StringRef SelectorName = SelectorString;

here.

509

You can just write

ME->getSelector().getAsString() == "localizedStringForKey:value:table:"

for the string comparison.

kulpreet updated this revision to Diff 32174.Aug 14 2015, 12:13 PM
  • Incorporated feedback from Devin's review
  • Moved LSM over to StringSet<>

Committed earlier today in r245093.

Thank you for contributing the checker!

zaks.anna closed this revision.Aug 14 2015, 5:31 PM