[modules] Fix marking `ObjCMethodDecl::isOverriding` when there a no overrides.

[vsapsai]

Summary

Incorrect isOverriding flag triggers the assertion
!Overridden.empty() in ObjCMethodDecl::getOverriddenMethods when a
method is marked as overriding but we cannot find any overrides.

When a method is declared in a category and defined in implementation,
we don't treat it as an override because it is the same method with
a separate declaration and a definition. But with modules we can find
a method declaration both in a modular category and a non-modular category
with different memory addresses. Thus we erroneously conclude the method
is overriding. Fix by comparing canonical declarations that are the same
for equal entities coming from different modules.

rdar://92845511

[egorzhdan]

LGTM!

[vsapsai]

Thanks for the quick review! During the commit I will also fix the typo s/there a no overrides/there are no overrides/

clang/test/Modules/override.m

31–35

Actually, only this interface triggers the assertion. Added the rest for completeness and for better coverage.

clang/lib/Sema/SemaDeclObjC.cpp

void Sema::CheckObjCMethodDirectOverrides(ObjCMethodDecl *method,
   }
 }
 
 void Sema::CheckObjCMethodOverrides(ObjCMethodDecl *ObjCMethod,
                                     ObjCInterfaceDecl *CurrentClass,
                                     ResultTypeCompatibilityKind RTC) {
   if (!ObjCMethod)
     return;
+  auto IsMethodInCurrentClass = [CurrentClass](const ObjCMethodDecl *M) {
+    // Checking canonical decl works across modules.
+    return M->getClassInterface()->getCanonicalDecl() ==
+           CurrentClass->getCanonicalDecl();
+  };
   // Search for overridden methods and merge information down from them.
   OverrideSearch overrides(*this, ObjCMethod);
   // Keep track if the method overrides any method in the class's base classes,
   // its protocols, or its categories' protocols; we will keep that info
   // in the ObjCMethodDecl.
   // For this info, a method in an implementation is not considered as
   // overriding the same method in the interface or its categories.
   bool hasOverriddenMethodsInBaseOrProtocol = false;
   for (ObjCMethodDecl *overridden : overrides) {
     if (!hasOverriddenMethodsInBaseOrProtocol) {
       if (isa<ObjCProtocolDecl>(overridden->getDeclContext()) ||
-          CurrentClass != overridden->getClassInterface() ||
-          overridden->isOverriding()) {
+          !IsMethodInCurrentClass(overridden) || overridden->isOverriding()) {
         CheckObjCMethodDirectOverrides(ObjCMethod, overridden);
         hasOverriddenMethodsInBaseOrProtocol = true;
       } else if (isa<ObjCImplDecl>(ObjCMethod->getDeclContext())) {
         // OverrideSearch will return as "overridden" the same method in the
         // interface. For hasOverriddenMethodsInBaseOrProtocol, we need to
         // check whether a category of a base class introduced a method with the
         // same selector, after the interface method declaration.
         // To avoid unnecessary lookups in the majority of cases, we use the
         // extra info bits in GlobalMethodPool to check whether there were any
         // category methods with this selector.
         GlobalMethodPool::iterator It =
             MethodPool.find(ObjCMethod->getSelector());
         if (It != MethodPool.end()) {
           ObjCMethodList &List =
             ObjCMethod->isInstanceMethod()? It->second.first: It->second.second;
           unsigned CategCount = List.getBits();
           if (CategCount > 0) {
             // If the method is in a category we'll do lookup if there were at
             // least 2 category methods recorded, otherwise only one will do.
             if (CategCount > 1 ||
                 !isa<ObjCCategoryImplDecl>(overridden->getDeclContext())) {
               OverrideSearch overrides(*this, overridden);
               for (ObjCMethodDecl *SuperOverridden : overrides) {
                 if (isa<ObjCProtocolDecl>(SuperOverridden->getDeclContext()) ||
-                    CurrentClass != SuperOverridden->getClassInterface()) {
+                    !IsMethodInCurrentClass(SuperOverridden)) {
                   CheckObjCMethodDirectOverrides(ObjCMethod, SuperOverridden);
                   hasOverriddenMethodsInBaseOrProtocol = true;
                   overridden->setOverriding(true);
                   break;
                 }
               }
             }
           }

clang/test/Modules/override.m

+// UNSUPPORTED: -aix
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -fsyntax-only -I%t/include %t/test.m \
+// RUN:            -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache -fmodule-name=CheckOverride
+
+// Test that if we have the same method in a different module, it's not an
+// override as it is the same method and it has the same DeclContext but a
+// different object in the memory.
+
+
+//--- include/CheckOverride.h
+@interface NSObject
+@end
+
+@interface CheckOverrideInterfaceOnly: NSObject
+- (void)potentialOverrideInterfaceOnly;
+@end
+
+@interface CheckOverrideCategoryOnly: NSObject
+@end
+@interface CheckOverrideCategoryOnly(CategoryOnly)
+- (void)potentialOverrideCategoryOnly;
+@end
+
+@interface CheckOverrideImplementationOfInterface: NSObject
+- (void)potentialOverrideImplementationOfInterface;
+@end
+
+@interface CheckOverrideImplementationOfCategory: NSObject
+@end
+@interface CheckOverrideImplementationOfCategory(CategoryImpl)
+- (void)potentialOverrideImplementationOfCategory;
+@end
+
+//--- include/Redirect.h
+// Ensure CheckOverride is imported as the module despite all `-fmodule-name` flags.
+#import <CheckOverride.h>
+
+//--- include/module.modulemap
+module CheckOverride {
+  header "CheckOverride.h"
+}
+module Redirect {
+  header "Redirect.h"
+  export *
+}
+
+//--- test.m
+#import <CheckOverride.h>
+#import <Redirect.h>
+
+@implementation CheckOverrideImplementationOfInterface
+- (void)potentialOverrideImplementationOfInterface {}
+@end
+
+@implementation CheckOverrideImplementationOfCategory
+- (void)potentialOverrideImplementationOfCategory {}
+@end
+
+void triggerOverrideCheck(CheckOverrideInterfaceOnly *intfOnly,
+                          CheckOverrideCategoryOnly *catOnly,
+                          CheckOverrideImplementationOfInterface *intfImpl,
+                          CheckOverrideImplementationOfCategory *catImpl) {
+  [intfOnly potentialOverrideInterfaceOnly];
+  [catOnly potentialOverrideCategoryOnly];
+  [intfImpl potentialOverrideImplementationOfInterface];
+  [catImpl potentialOverrideImplementationOfCategory];
+}