[clang-tidy] Implement a check for large Objective-C type encodings 🔍

[stephanemoore]

Summary

Objective-C type encodings are normally pretty small but they it is
pretty easy for them to bloat to undesirable levels. Bloated Objective-C
type encodings are particularly common for Objective-C methods with
templated C++ types in their interface in Objective-C++. For example,
in Objective-C type encodings std::string expands to 277 bytes and
std::map<std::string, std::string> expands to 1219 bytes. The bloat
isn't particularly important for larger binaries but SDKs sometimes
optimize their binary size to ease adoption. This check aims to provide
some level of visibility into the size of generated Objective-C type
encodings so that developers can address them if they want.

Related article:
https://medium.com/@dmaclach/objective-c-encoding-and-you-866624cc02de

Test Notes:
Verified clang-tidy tests pass successfully.

[theraven]

I wonder if we want to have an option to elide ObjC type info for all non-POD C++ types. Nothing that you do with the type encoding is likely to be correct (for example, you can see the pointer field in a std::shared_ptr, but you can't see that changes to it need to update reference counts) so it probably does more harm than good.

[Eugene.Zelenko]

docs/clang-tidy/checks/objc-type-encoding-size.rst

7

Please synchronize with Release Notes.

[stephanemoore]

Update check description to match release notes.

[dmaclach]

clang-tidy/objc/TypeEncodingSizeCheck.cpp

53

I found it very useful in my diagnostics to know how big it actually was. Can you add that?

101

Can you add what the actual size was in the output?

[hokein]

clang-tidy/objc/TypeEncodingSizeCheck.cpp

36

hasAncestor is an expensive matcher, does hasDeclContext meet your use cases?

60

nit: add an assertion assert(EncodedDecl).

63

Do you forget to register the matcher for ObjCIvarDecl? In the matcher you register it for ObjCPropertyDecl, and ObjCInterfaceDecl, so this branch will never be executed.

docs/clang-tidy/checks/objc-type-encoding-size.rst

13

nit: mention the default value.

[stephanemoore]

Changes:
• Assert on EncodedDecl.
• Mention default value in objc-type-encoding-size check notes.

Outstanding action items:
• Evaluate using hasDeclContext instead of hasAncestor.
• Include type encoding size information in diagnostic messages.

[stephanemoore]

In D55640#1329390, @theraven wrote:

I wonder if we want to have an option to elide ObjC type info for all non-POD C++ types. Nothing that you do with the type encoding is likely to be correct (for example, you can see the pointer field in a std::shared_ptr, but you can't see that changes to it need to update reference counts) so it probably does more harm than good.

I think it's worth investigating some kind of option for eliding Objective-C type information though I think that investigation is likely beyond the scope of this proposal. Eliding Objective-C type information could change runtime behavior of a variety of systems that rely on this information. This could include systems embedded in Apple's libraries as well as systems implemented in third party or open source libraries. I imagine that an investigation into such a feature would probably need to be quite thorough.

clang-tidy/objc/TypeEncodingSizeCheck.cpp

36

I started looking into using hasDeclContext but encountered some unexpected behavior when adding relevant test cases—which should have been added from the beginning—to verify. I am going to continue investigating whether hasDeclContext is satisfactory here and try to resolve the unexpected behavior that I encountered.

63

On line 32 in this diff is where I register the matcher for ObjCIvarDecl. The matching is functioning as I expect it should. Let me know if you want me to reorganize the matching logic.

docs/clang-tidy/checks/objc-type-encoding-size.rst

7

I changed the check description to match the release notes. Let me know if I misunderstood the requested change.

[stephanemoore]

Reiterating outstanding action items:
• Evaluate using hasDeclContext instead of hasAncestor.
• Include type encoding size information in diagnostic messages.

I've been observing some unexpected behaviors with matching in the proposed test sample that I am still investigating (as I recall the issue pertained to interfaces/protocols without interfaces).

Including the type encoding size information in diagnostic messages shouldn't be too hard (assuming we can get precise type encoding size information by analyzing the AST).

[thakis]

FYI D96816 made clang emit way smaller encodings by default

clang-tidy/objc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyObjCModule
   AvoidNSErrorInitCheck.cpp
   AvoidSpinlockCheck.cpp
   ForbiddenSubclassingCheck.cpp
   ObjCTidyModule.cpp
   PropertyDeclarationCheck.cpp
+  TypeEncodingSizeCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   )

clang-tidy/objc/ObjCTidyModule.cpp

 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "AvoidNSErrorInitCheck.h"
 #include "AvoidSpinlockCheck.h"
 #include "ForbiddenSubclassingCheck.h"
 #include "PropertyDeclarationCheck.h"
+#include "TypeEncodingSizeCheck.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace objc {
 
 class ObjCModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<AvoidNSErrorInitCheck>(
         "objc-avoid-nserror-init");
     CheckFactories.registerCheck<AvoidSpinlockCheck>(
         "objc-avoid-spinlock");
     CheckFactories.registerCheck<ForbiddenSubclassingCheck>(
         "objc-forbidden-subclassing");
     CheckFactories.registerCheck<PropertyDeclarationCheck>(
         "objc-property-declaration");
+    CheckFactories.registerCheck<TypeEncodingSizeCheck>(
+        "objc-type-encoding-size");
   }
 };
 
 // Register the ObjCTidyModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<ObjCModule> X(
     "objc-module",
     "Adds Objective-C lint checks.");
 
 } // namespace objc
 

clang-tidy/objc/TypeEncodingSizeCheck.h

+//===--- TypeEncodingSizeCheck.h - clang-tidy -----------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "../ClangTidy.h"
+#include "llvm/ADT/StringRef.h"
+
+namespace clang {
+namespace tidy {
+namespace objc {
+
+/// Finds Objective-C type encodings that exceed a configured size threshold.
+///
+/// For the user-facing documentation see:
+/// http:///clang.llvm.org/extra/clang-tidy/checks/objc-type-encoding-size.html
+class TypeEncodingSizeCheck : public ClangTidyCheck {
+public:
+  TypeEncodingSizeCheck(StringRef Name, ClangTidyContext *Context);
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+
+private:
+  const unsigned Threshold;
+};
+
+} // namespace objc
+} // namespace tidy
+} // namespace clang

clang-tidy/objc/TypeEncodingSizeCheck.cpp

+//===--- TypeEncodingSizeCheck.cpp - clang-tidy ---------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "TypeEncodingSizeCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace objc {
+
+TypeEncodingSizeCheck::TypeEncodingSizeCheck(StringRef Name,
+                                             ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      Threshold(Options.get("Threshold", 2000U)) {}
+
+void TypeEncodingSizeCheck::registerMatchers(MatchFinder *Finder) {
+  // This check should only be applied to Objective-C sources.
+  if (!getLangOpts().ObjC)
+    return;
+
+  Finder->addMatcher(
+      blockExpr(unless(isExpansionInSystemHeader())).bind("block"), this);
+  Finder->addMatcher(
+      objcIvarDecl(unless(isExpansionInSystemHeader())).bind("decl"), this);
+  Finder->addMatcher(
+      objcPropertyDecl(unless(isExpansionInSystemHeader()),
+                       anyOf(hasAncestor(objcInterfaceDecl().bind("interface")),
+                             hasAncestor(objcCategoryDecl().bind("category"))))
+          .bind("decl"),
+      this);
+  Finder->addMatcher(
+      objcMethodDecl(unless(isExpansionInSystemHeader())).bind("decl"), this);
+}
+
+void TypeEncodingSizeCheck::check(const MatchFinder::MatchResult &Result) {
+  if (const auto *Block = Result.Nodes.getNodeAs<BlockExpr>("block")) {
+    std::string BlockTypeEncoding =
+        Block->getBlockDecl()->getASTContext().getObjCEncodingForBlock(Block);
+
+    if (BlockTypeEncoding.size() <= Threshold)
+      return;
+
+    diag(Block->getCaretLocation(),
+         "Objective-C type encoding for block expression exceeds %0 "
+         "characters")
+        << Threshold;
+
+    return;
+  }
+
+  const auto *EncodedDecl = Result.Nodes.getNodeAs<NamedDecl>("decl");
+
+  std::string TypeEncoding;
+  if (const auto *IvarDecl = dyn_cast<ObjCIvarDecl>(EncodedDecl)) {
+    IvarDecl->getASTContext().getObjCEncodingForType(IvarDecl->getType(),
+                                                     TypeEncoding);
+  } else if (const auto *PropertyDecl =
+                 dyn_cast<ObjCPropertyDecl>(EncodedDecl)) {
+    const Decl *ContainerDecl = nullptr;
+
+    // Properties in interfaces and categories need respective implementations
+    // to determine the type encoding.
+    if (const auto *ClassDecl =
+            Result.Nodes.getNodeAs<ObjCInterfaceDecl>("interface")) {
+      ContainerDecl = ClassDecl->getImplementation();
+
+      // Bail out if we cannot find the implementation because this likely
+      // indicates a finding that cannot be acted upon.
+      if (ContainerDecl == nullptr)
+        return;
+    } else if (const auto *CategoryDecl =
+                   Result.Nodes.getNodeAs<ObjCCategoryDecl>("category")) {
+      ContainerDecl = CategoryDecl->getImplementation();
+
+      // Bail out if we cannot find the implementation because this likely
+      // indicates a finding that cannot be acted upon.
+      if (ContainerDecl == nullptr)
+        return;
+    }
+
+    TypeEncoding = PropertyDecl->getASTContext().getObjCEncodingForPropertyDecl(
+        PropertyDecl, ContainerDecl);
+  } else if (const auto *MethodDecl = dyn_cast<ObjCMethodDecl>(EncodedDecl)) {
+    TypeEncoding =
+        MethodDecl->getASTContext().getObjCEncodingForMethodDecl(MethodDecl);
+  }
+
+  if (TypeEncoding.size() <= Threshold)
+    return;
+
+  diag(EncodedDecl->getLocation(),
+       "Objective-C type encoding for %0 exceeds %1 characters")
+      << EncodedDecl << Threshold;
+}
+
+void TypeEncodingSizeCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "Threshold", Threshold);
+}
+
+} // namespace objc
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

   Checks for uses of nested namespaces in the form of
   ``namespace a { namespace b { ... }}`` and offers change to
   syntax introduced in C++17 standard: ``namespace a::b { ... }``.
 
 - New :doc:`modernize-deprecated-ios-base-aliases
   <clang-tidy/checks/modernize-deprecated-ios-base-aliases>` check.
 
   Detects usage of the deprecated member types of ``std::ios_base`` and replaces
   those that have a non-deprecated equivalent.
 
+- New :doc:`objc-type-encoding-size
+  <clang-tidy/checks/objc-type-encoding-size>` check.
+
+  Detects Objective-C type encodings that exceed a configured threshold.
+
 - New :doc:`readability-isolate-decl
   <clang-tidy/checks/readability-isolate-declaration>` check.
 
   Detects local variable declarations declaring more than one variable and
   tries to refactor the code to one statement per declaration.
 
 - New :doc:`readability-const-return-type
   <clang-tidy/checks/readability-const-return-type>` check.
 
   Checks for functions with a ``const``-qualified return type and recommends

docs/clang-tidy/checks/list.rst

    modernize-use-override
    modernize-use-transparent-functors
    modernize-use-uncaught-exceptions
    modernize-use-using
    mpi-buffer-deref
    mpi-type-mismatch
    objc-avoid-nserror-init
    objc-avoid-spinlock
    objc-forbidden-subclassing
    objc-property-declaration
+   objc-type-encoding-size
    performance-faster-string-find
    performance-for-range-copy
    performance-implicit-conversion-in-loop
    performance-inefficient-algorithm
    performance-inefficient-string-concatenation
    performance-inefficient-vector-operation
    performance-move-const-arg
    performance-move-constructor-init
    performance-noexcept-move-constructor
    performance-type-promotion-in-math-fn

docs/clang-tidy/checks/objc-type-encoding-size.rst

+.. title:: clang-tidy - objc-type-encoding-size
+
+objc-type-encoding-size
+=======================
+
+Detects Objective-C type encodings that exceed a configured threshold.
+
+Options
+-------
+
+.. option:: Threshold
+
+   Flag Objective-C type encodings that exceed this number of bytes.

test/clang-tidy/objc-type-encoding-size.m

+// RUN: %check_clang_tidy %s objc-type-encoding-size %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: objc-type-encoding-size.Threshold, value: 15}]}' \
+// RUN: -- -fblocks
+
+typedef struct {
+  int a1;
+  int a2;
+  int a3;
+  int a4;
+  int a5;
+  int a6;
+  int a7;
+  int a8;
+  int a9;
+  int a10;
+  int a11;
+  int a12;
+} SixteenCharStruct;
+
+typedef struct {
+  int a1;
+} FiveCharStruct;
+
+typedef void (^BlockType)(SixteenCharStruct);
+
+@interface Foo {
+  SixteenCharStruct _someStruct;
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: Objective-C type encoding for
+  // '_someStruct' exceeds 15 characters [objc-type-encoding-size]
+
+  int _anInteger;
+}
+
+@property(nonatomic) SixteenCharStruct anotherStruct;
+// CHECK-MESSAGES: :[[@LINE-1]]:40: warning: Objective-C type encoding for
+// 'anotherStruct' exceeds 15 characters [objc-type-encoding-size]
+// CHECK-MESSAGES: :[[@LINE-3]]:40: warning: Objective-C type encoding for
+// 'setAnotherStruct:' exceeds 15 characters [objc-type-encoding-size]
+
+@property(nonatomic, setter=setAsparagus:) FiveCharStruct bananas;
+// CHECK-MESSAGES: :[[@LINE-1]]:59: warning: Objective-C type encoding for
+// 'bananas' exceeds 15 characters [objc-type-encoding-size]
+
+@end
+
+@implementation Foo
+
+- (void)bar:(SixteenCharStruct)a {
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: Objective-C type encoding for 'bar:'
+// exceeds 15 characters [objc-type-encoding-size]
+}
+
+- (int)doThing {
+  int (^block)(SixteenCharStruct) = ^(SixteenCharStruct a) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:37: warning: Objective-C type encoding for
+    // block expression exceeds 15 characters [objc-type-encoding-size]
+    return a.a4 + 5;
+  };
+
+  return block(_someStruct);
+}
+
+- (const char *)str {
+  // We should not flag explicit encoding statements.
+  return @encode(SixteenCharStruct);
+}
+
+@end