- Adjusted copy to be consistent with diagnostic text in other Apple API checkers
- Added in ~150 UIKit / AppKit methods that require localized strings in UnlocalizedStringsChecker
- UnlocalizedStringChecker now checks for UI methods up the class hierarchy and UI methods that conform for a certain Objective-C protocol.
- Added in alpha version of PluralMisuseChecker and some regression tests. False positives are still not ideal.
Details
- Reviewers
dcoughlin zaks.anna xazax.hun - Commits
- rG9f21f68bfeed: [analyzer] Improve localizability checks for iOS / OS X.
rGab58314357c0: [analyzer] Improve localizability checks for iOS / OS X.
rC248432: [analyzer] Improve localizability checks for iOS / OS X.
rC248350: [analyzer] Improve localizability checks for iOS / OS X.
rL248432: [analyzer] Improve localizability checks for iOS / OS X.
rL248350: [analyzer] Improve localizability checks for iOS / OS X.
Diff Detail
Event Timeline
I think in general, it might be better to have all the UI methods in a separate file, in a specific format, and you could include that file and use a macro, to generate the code.
Thanks for working on this!
Added in ~150 UIKit / AppKit methods that require localized strings in UnlocalizedStringsChecker
It might be more efficient to store identifiers. Have you considered that approach?
For the Plural Misuse Checker Tests, please, add some test cases that show the common false positives that your checker already avoids as well as common false positives it does not deal with right now. (And mark them clearly to tell the difference.) These are important to ensure that we do not regress in false positive rates once someone less familiar with the checker modifies it!
Other comments are inline.
lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp | ||
---|---|---|
1114 | For better readability, please, rename into something like isCheckingPlurality. | |
1138 | Is there a "working" test case for this? | |
1145 | Should this I/O line be removed? | |
1174 | FD->getNameInfo().getAsString() is a temporary variable, so it might get deallocated by the time you use NormalizeName. Please, build and test with ASan on. You could also check the return type of the Decl. | |
1213 | You could simplify this code with an early return statement. Why do we need to override traverse? If you are trying to avoid traversing everything that's not an IfStmt, maybe you could call Visitor.TraverseStmt on the top level if statements.. | |
1270 | "Use a .stringsdict file" -> "Use a .stringsdict file instead" |
It might be more efficient to store identifiers. Have you considered that approach?
I did consider this approach a while back, but I ran into a problem when I realized that Selector comparison doesn’t work the way I expected it. You can see it in this debug output:
765 766 Selector S = msg.getSelector(); 767 -> 768 std::string SelectorString = S.getAsString(); 769 StringRef SelectorName = SelectorString; 770 assert(!SelectorName.empty()); 771 (lldb) p S (clang::Selector) $3 = (InfoPtr = 4513118723) (lldb) p S.getAsString() (std::__1::string) $4 = "localizedStringForKey:value:table:" (lldb) p getKeywordSelector(C.getASTContext(),"localizedStringForKey:","value:","table:",nullptr) (clang::Selector) $5 = (InfoPtr = 4513118835) (lldb) p S == getKeywordSelector(C.getASTContext(),"localizedStringForKey:","value:","table:",nullptr) (bool) $7 = false
Looking at the implementation of getKeywordSelector(), it looks like a new Selector object is being constructed from the identifiers that make up the components of the selector. In its constructor, Selector maintains InfoPtr to an underlying MultiKeywordSelector object. When comparing two Selectors, this underlying InfoPtr is compared. Unfortunately, MultiKeywordSelector does not override operator==(), so it just compares the pointers directly. My guess is that it probably should compare the IdentifierInfo pointer in each slot to see if they are the same.
For the Plural Misuse Checker Tests, please, add some test cases that show the common false positives that your checker already avoids as well as common false positives it does not deal with right now.
I added a couple test cases for the one heuristic I added to reduce false positives and one for the common false positive I saw in some projects. I've been looking around for good localized projects to test this on, but there aren't many. I tested on Wikipedia iOS and there were no diagnostics raised for PluralMisuseChecker. (but there were 11 real diagnostics raised with the NonLocalizedStringChecker - hooray!)
I think in general, it might be better to have all the UI methods in a separate file, in a specific format, and you could include that file and use a macro, to generate the code.
This is something I will look into in the future, since I've already generated this code.
lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp | ||
---|---|---|
1138 | This is tested in test5: in localization.m | |
1174 | I tested with ASan on and there were no memory issues reported. I think here NormalizeName doesn't get deallocated because it's the result of StringRef.lower(). | |
1213 | I simplified the code as per your recommendation. We override TraverseIfStmt to keep track of whether we are inside (visiting the children) of an IfStmt. By overriding TraverseIfStmt, we know when we have finished visiting all of the children of a matching IfStmt and can set InMatchingStatment to false. I discussed this problem with Gabor a while back and that was the solution we came up with. |
lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp | ||
---|---|---|
1209 | In the visitor pattern BeginVisit and EndVisit are commonly used terms. Maybe it would be cleaner to refactor the code bellow into a separate method that has a name like EndVisitIfStmt and add a comment here that EndVisit callbacks are not provided by the recursiveASTVisitor, and this is the code to add it. |
I think, if that is the only reason why storing identifiers/selectors does not work, it could be a good enough motivation to implement proper == operators in clang!
In general, == should be either disabled or work intuitively.
NonLocalizedStringChecker:
- Got the Selector/Identifier object comparison working! So no more string comparison for UIMethods
- Moved over to using macros for list of UIMethods as per Gabor's recommendation
PluralMisuseChecker:
- Separated out TraverseIfStmt function into EndVisitIfStmt in to make it clear that this is after an IfStmt is visited
(Sorry for the delay!)
lib/StaticAnalyzer/Checkers/Checkers.td | ||
---|---|---|
493 | It would be good to shorten the description. |
Shortened description of PluralMisuseChecker in Checkers.td as per Anna's recommendation.
This is causing tests to fail on the bots: http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/31373
/home/llvmbb/llvm-build-dir/clang-x86_64-debian-fast/llvm.obj/Release+Asserts/bin/clang -cc1 -internal-isystem /home/llvmbb/llvm-build-dir/clang-x86_64-debian-fast/llvm.obj/Release+Asserts/bin/../lib/clang/3.8.0/include -nostdsysteminc -analyze -fblocks -analyzer-store=region -analyzer-checker=alpha.osx.cocoa.NonLocalizedStringChecker -analyzer-checker=alpha.osx.cocoa.PluralMisuseChecker -verify /home/llvmbb/llvm-build-dir/clang-x86_64-debian-fast/llvm.src/tools/clang/test/Analysis/localization.m -- Exit Code: 1 Command Output (stderr): -- error: 'warning' diagnostics expected but not seen: File /home/llvmbb/llvm-build-dir/clang-x86_64-debian-fast/llvm.src/tools/clang/test/Analysis/localization.m Line 115: Plural cases are not supported accross all languages. Use a .stringsdict file File /home/llvmbb/llvm-build-dir/clang-x86_64-debian-fast/llvm.src/tools/clang/test/Analysis/localization.m Line 117: Plural cases are not supported accross all languages. Use a .stringsdict file File /home/llvmbb/llvm-build-dir/clang-x86_64-debian-fast/llvm.src/tools/clang/test/Analysis/localization.m Line 124: Plural cases are not supported accross all languages. Use a .stringsdict file File /home/llvmbb/llvm-build-dir/clang-x86_64-debian-fast/llvm.src/tools/clang/test/Analysis/localization.m Line 126: Plural cases are not supported accross all languages. Use a .stringsdict file File /home/llvmbb/llvm-build-dir/clang-x86_64-debian-fast/llvm.src/tools/clang/test/Analysis/localization.m Line 134: Plural cases are not supported accross all languages. Use a .stringsdict file File /home/llvmbb/llvm-build-dir/clang-x86_64-debian-fast/llvm.src/tools/clang/test/Analysis/localization.m Line 136: Plural cases are not supported accross all languages. Use a .stringsdict file File /home/llvmbb/llvm-build-dir/clang-x86_64-debian-fast/llvm.src/tools/clang/test/Analysis/localization.m Line 150: Plural cases are not supported accross all languages. Use a .stringsdict file 7 errors generated.
It would be good to shorten the description.