[PATCH] D35186: [analyzer] Add annotation for functions taking user-facing strings

[Erik Verbruggen via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL308012: [analyzer] Add annotation for functions taking 
user-facing strings (authored by erikjv).

Changed prior to commit:
  https://reviews.llvm.org/D35186?vs=105792=106605#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D35186

Files:
  cfe/trunk/lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp
  cfe/trunk/test/Analysis/localization-aggressive.m

Index: cfe/trunk/lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp
@@ -57,7 +57,7 @@
 };
 
 class NonLocalizedStringChecker
-: public Checker {
 
@@ -79,9 +79,10 @@
   void setNonLocalizedState(SVal S, CheckerContext ) const;
   void setLocalizedState(SVal S, CheckerContext ) const;
 
-  bool isAnnotatedAsLocalized(const Decl *D) const;
-  void reportLocalizationError(SVal S, const ObjCMethodCall ,
-   CheckerContext , int argumentNumber = 0) const;
+  bool isAnnotatedAsReturningLocalized(const Decl *D) const;
+  bool isAnnotatedAsTakingLocalized(const Decl *D) const;
+  void reportLocalizationError(SVal S, const CallEvent , CheckerContext ,
+   int argumentNumber = 0) const;
 
   int getLocalizedArgumentForSelector(const IdentifierInfo *Receiver,
   Selector S) const;
@@ -97,6 +98,7 @@
   void checkPreObjCMessage(const ObjCMethodCall , CheckerContext ) const;
   void checkPostObjCMessage(const ObjCMethodCall , CheckerContext ) const;
   void checkPostStmt(const ObjCStringLiteral *SL, CheckerContext ) const;
+  void checkPreCall(const CallEvent , CheckerContext ) const;
   void checkPostCall(const CallEvent , CheckerContext ) const;
 };
 
@@ -644,7 +646,8 @@
 
 /// Checks to see if the method / function declaration includes
 /// __attribute__((annotate("returns_localized_nsstring")))
-bool NonLocalizedStringChecker::isAnnotatedAsLocalized(const Decl *D) const {
+bool NonLocalizedStringChecker::isAnnotatedAsReturningLocalized(
+const Decl *D) const {
   if (!D)
 return false;
   return std::any_of(
@@ -654,6 +657,19 @@
   });
 }
 
+/// Checks to see if the method / function declaration includes
+/// __attribute__((annotate("takes_localized_nsstring")))
+bool NonLocalizedStringChecker::isAnnotatedAsTakingLocalized(
+const Decl *D) const {
+  if (!D)
+return false;
+  return std::any_of(
+  D->specific_attr_begin(),
+  D->specific_attr_end(), [](const AnnotateAttr *Ann) {
+return Ann->getAnnotation() == "takes_localized_nsstring";
+  });
+}
+
 /// Returns true if the given SVal is marked as Localized in the program state
 bool NonLocalizedStringChecker::hasLocalizedState(SVal S,
   CheckerContext ) const {
@@ -733,8 +749,7 @@
 
 /// Reports a localization error for the passed in method call and SVal
 void NonLocalizedStringChecker::reportLocalizationError(
-SVal S, const ObjCMethodCall , CheckerContext ,
-int argumentNumber) const {
+SVal S, const CallEvent , CheckerContext , int argumentNumber) const {
 
   // Don't warn about localization errors in classes and methods that
   // may be debug code.
@@ -832,7 +847,21 @@
 }
   }
 
-  if (argumentNumber < 0) // There was no match in UIMethods
+  if (argumentNumber < 0) { // There was no match in UIMethods
+if (const Decl *D = msg.getDecl()) {
+  if (const ObjCMethodDecl *OMD = dyn_cast_or_null(D)) {
+auto formals = OMD->parameters();
+for (unsigned i = 0, ei = formals.size(); i != ei; ++i) {
+  if (isAnnotatedAsTakingLocalized(formals[i])) {
+argumentNumber = i;
+break;
+  }
+}
+  }
+}
+  }
+
+  if (argumentNumber < 0) // Still no match
 return;
 
   SVal svTitle = msg.getArgSVal(argumentNumber);
@@ -855,6 +884,25 @@
   }
 }
 
+void NonLocalizedStringChecker::checkPreCall(const CallEvent ,
+ CheckerContext ) const {
+  const Decl *D = Call.getDecl();
+  if (D && isa(D)) {
+const FunctionDecl *FD = dyn_cast(D);
+auto formals = FD->parameters();
+for (unsigned i = 0,
+  ei = std::min(unsigned(formals.size()), Call.getNumArgs());
+ i != ei; ++i) {
+  if (isAnnotatedAsTakingLocalized(formals[i])) {
+auto actual = Call.getArgSVal(i);
+if (hasNonLocalizedState(actual, C)) {
+  reportLocalizationError(actual, Call, C, i + 1);
+}
+  }
+}
+  }
+}
+
 static inline bool isNSStringType(QualType T, ASTContext ) {
 
   const 

[PATCH] D35186: [analyzer] Add annotation for functions taking user-facing strings

[Devin Coughlin via Phabricator via cfe-commits]

dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.

Thanks for the patch! This looks very good to me.

The one thing I would suggest is renaming 'isAnnotatedAsLocalized()' and 
'isAnnotatedTakingLocalized()' to make it more clear what each does, now that 
there are two of them. How about: 'isAnnotatedAsReturningLocalized()' and 
'isAnnotatedAsTakingLocalized()'?


https://reviews.llvm.org/D35186



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35186: [analyzer] Add annotation for functions taking user-facing strings

[Erik Verbruggen via Phabricator via cfe-commits]

erikjv updated this revision to Diff 105792.
erikjv added a comment.

Sorry, now with more context in the diff.


https://reviews.llvm.org/D35186

Files:
  lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp
  test/Analysis/localization-aggressive.m

Index: test/Analysis/localization-aggressive.m
===
--- test/Analysis/localization-aggressive.m
+++ test/Analysis/localization-aggressive.m
@@ -61,8 +61,16 @@
 NSString *CFNumberFormatterCreateStringWithNumber(float x);
 + (NSString *)forceLocalized:(NSString *)str
 __attribute__((annotate("returns_localized_nsstring")));
++ (NSString *)takesLocalizedString:
+(NSString *)__attribute__((annotate("takes_localized_nsstring")))str;
 @end
 
+NSString *
+takesLocalizedString(NSString *str
+ __attribute__((annotate("takes_localized_nsstring" {
+  return str;
+}
+
 // Test cases begin here
 @implementation LocalizationTestSuite
 
@@ -75,6 +83,8 @@
   return str;
 }
 
++ (NSString *) takesLocalizedString:(NSString *)str { return str; }
+
 // An ObjC method that returns a localized string
 + (NSString *)unLocalizedStringMethod {
   return @"UnlocalizedString";
@@ -269,4 +279,13 @@
   NSString *string2 = POSSIBLE_FALSE_POSITIVE(@"Hello", @"Hello"); // no-warning
 }
 
+- (void)testTakesLocalizedString {
+  NSString *localized = NSLocalizedString(@"Hello", @"World");
+  NSString *alsoLocalized = [LocalizationTestSuite takesLocalizedString:localized]; // no-warning
+  NSString *stillLocalized = [LocalizationTestSuite takesLocalizedString:alsoLocalized]; // no-warning
+  takesLocalizedString(stillLocalized); // no-warning
+
+  [LocalizationTestSuite takesLocalizedString:@"not localized"]; // expected-warning {{User-facing text should use localized string macro}}
+  takesLocalizedString(@"not localized"); // expected-warning {{User-facing text should use localized string macro}}
+}
 @end
Index: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp
+++ lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp
@@ -57,7 +57,7 @@
 };
 
 class NonLocalizedStringChecker
-: public Checker {
 
@@ -80,8 +80,9 @@
   void setLocalizedState(SVal S, CheckerContext ) const;
 
   bool isAnnotatedAsLocalized(const Decl *D) const;
-  void reportLocalizationError(SVal S, const ObjCMethodCall ,
-   CheckerContext , int argumentNumber = 0) const;
+  bool isAnnotatedTakingLocalized(const Decl *D) const;
+  void reportLocalizationError(SVal S, const CallEvent , CheckerContext ,
+   int argumentNumber = 0) const;
 
   int getLocalizedArgumentForSelector(const IdentifierInfo *Receiver,
   Selector S) const;
@@ -97,6 +98,7 @@
   void checkPreObjCMessage(const ObjCMethodCall , CheckerContext ) const;
   void checkPostObjCMessage(const ObjCMethodCall , CheckerContext ) const;
   void checkPostStmt(const ObjCStringLiteral *SL, CheckerContext ) const;
+  void checkPreCall(const CallEvent , CheckerContext ) const;
   void checkPostCall(const CallEvent , CheckerContext ) const;
 };
 
@@ -654,6 +656,18 @@
   });
 }
 
+/// Checks to see if the method / function declaration includes
+/// __attribute__((annotate("takes_localized_nsstring")))
+bool NonLocalizedStringChecker::isAnnotatedTakingLocalized(const Decl *D) const {
+  if (!D)
+return false;
+  return std::any_of(
+  D->specific_attr_begin(),
+  D->specific_attr_end(), [](const AnnotateAttr *Ann) {
+return Ann->getAnnotation() == "takes_localized_nsstring";
+  });
+}
+
 /// Returns true if the given SVal is marked as Localized in the program state
 bool NonLocalizedStringChecker::hasLocalizedState(SVal S,
   CheckerContext ) const {
@@ -733,8 +747,7 @@
 
 /// Reports a localization error for the passed in method call and SVal
 void NonLocalizedStringChecker::reportLocalizationError(
-SVal S, const ObjCMethodCall , CheckerContext ,
-int argumentNumber) const {
+SVal S, const CallEvent , CheckerContext , int argumentNumber) const {
 
   // Don't warn about localization errors in classes and methods that
   // may be debug code.
@@ -832,7 +845,21 @@
 }
   }
 
-  if (argumentNumber < 0) // There was no match in UIMethods
+  if (argumentNumber < 0) { // There was no match in UIMethods
+if (const Decl *D = msg.getDecl()) {
+  if (const ObjCMethodDecl *OMD = dyn_cast_or_null(D)) {
+auto formals = OMD->parameters();
+for (unsigned i = 0, ei = formals.size(); i != ei; ++i) {
+  if (isAnnotatedTakingLocalized(formals[i])) {
+

[PATCH] D35186: [analyzer] Add annotation for functions taking user-facing strings

[Erik Verbruggen via Phabricator via cfe-commits]

erikjv created this revision.
Herald added a subscriber: xazax.hun.

There was already a returns_localized_nsstring annotation to indicate
that the return value could be passed to UIKit methods that would
display them. However, those UIKit methods were hard-coded, and it was
not possible to indicate that other classes/methods in a code-base would
do the same.

The takes_localized_nsstring annotation can be put on function
parameters and selector parameters to indicate that those will also show
the string to the user.


https://reviews.llvm.org/D35186

Files:
  lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp
  test/Analysis/localization-aggressive.m

Index: test/Analysis/localization-aggressive.m
===
--- test/Analysis/localization-aggressive.m
+++ test/Analysis/localization-aggressive.m
@@ -61,8 +61,16 @@
 NSString *CFNumberFormatterCreateStringWithNumber(float x);
 + (NSString *)forceLocalized:(NSString *)str
 __attribute__((annotate("returns_localized_nsstring")));
++ (NSString *)takesLocalizedString:
+(NSString *)__attribute__((annotate("takes_localized_nsstring")))str;
 @end
 
+NSString *
+takesLocalizedString(NSString *str
+ __attribute__((annotate("takes_localized_nsstring" {
+  return str;
+}
+
 // Test cases begin here
 @implementation LocalizationTestSuite
 
@@ -75,6 +83,8 @@
   return str;
 }
 
++ (NSString *) takesLocalizedString:(NSString *)str { return str; }
+
 // An ObjC method that returns a localized string
 + (NSString *)unLocalizedStringMethod {
   return @"UnlocalizedString";
@@ -269,4 +279,13 @@
   NSString *string2 = POSSIBLE_FALSE_POSITIVE(@"Hello", @"Hello"); // no-warning
 }
 
+- (void)testTakesLocalizedString {
+  NSString *localized = NSLocalizedString(@"Hello", @"World");
+  NSString *alsoLocalized = [LocalizationTestSuite takesLocalizedString:localized]; // no-warning
+  NSString *stillLocalized = [LocalizationTestSuite takesLocalizedString:alsoLocalized]; // no-warning
+  takesLocalizedString(stillLocalized); // no-warning
+
+  [LocalizationTestSuite takesLocalizedString:@"not localized"]; // expected-warning {{User-facing text should use localized string macro}}
+  takesLocalizedString(@"not localized"); // expected-warning {{User-facing text should use localized string macro}}
+}
 @end
Index: lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp
+++ lib/StaticAnalyzer/Checkers/LocalizationChecker.cpp
@@ -57,7 +57,7 @@
 };
 
 class NonLocalizedStringChecker
-: public Checker {
 
@@ -80,8 +80,9 @@
   void setLocalizedState(SVal S, CheckerContext ) const;
 
   bool isAnnotatedAsLocalized(const Decl *D) const;
-  void reportLocalizationError(SVal S, const ObjCMethodCall ,
-   CheckerContext , int argumentNumber = 0) const;
+  bool isAnnotatedTakingLocalized(const Decl *D) const;
+  void reportLocalizationError(SVal S, const CallEvent , CheckerContext ,
+   int argumentNumber = 0) const;
 
   int getLocalizedArgumentForSelector(const IdentifierInfo *Receiver,
   Selector S) const;
@@ -97,6 +98,7 @@
   void checkPreObjCMessage(const ObjCMethodCall , CheckerContext ) const;
   void checkPostObjCMessage(const ObjCMethodCall , CheckerContext ) const;
   void checkPostStmt(const ObjCStringLiteral *SL, CheckerContext ) const;
+  void checkPreCall(const CallEvent , CheckerContext ) const;
   void checkPostCall(const CallEvent , CheckerContext ) const;
 };
 
@@ -654,6 +656,18 @@
   });
 }
 
+/// Checks to see if the method / function declaration includes
+/// __attribute__((annotate("takes_localized_nsstring")))
+bool NonLocalizedStringChecker::isAnnotatedTakingLocalized(const Decl *D) const {
+  if (!D)
+return false;
+  return std::any_of(
+  D->specific_attr_begin(),
+  D->specific_attr_end(), [](const AnnotateAttr *Ann) {
+return Ann->getAnnotation() == "takes_localized_nsstring";
+  });
+}
+
 /// Returns true if the given SVal is marked as Localized in the program state
 bool NonLocalizedStringChecker::hasLocalizedState(SVal S,
   CheckerContext ) const {
@@ -733,8 +747,7 @@
 
 /// Reports a localization error for the passed in method call and SVal
 void NonLocalizedStringChecker::reportLocalizationError(
-SVal S, const ObjCMethodCall , CheckerContext ,
-int argumentNumber) const {
+SVal S, const CallEvent , CheckerContext , int argumentNumber) const {
 
   // Don't warn about localization errors in classes and methods that
   // may be debug code.
@@ -832,7 +845,21 @@
 }
   }
 
-