[Serialization] Don't warn when a deserialized category is equivalent to an existing one.

[vsapsai]

Summary

A single class allows multiple categories to be defined for it. But if
two of such categories have the same name, we emit a warning. It's not a
hard error but a good indication of a potential mistake.

With modules, we can end up with the same category in different modules.
Diagnosing such a situation has little value as the categories in
different modules are equivalent and don't reflect the usage of the same
name for different purposes. When we deserialize a duplicate category,
compare it to an existing one and warn only when the new one is
different.

rdar://104582081

[vsapsai]

clang/lib/Serialization/ASTReaderDecl.cpp

4188

Decided to use StructuralEquivalenceContext here because we already use it to compare ObjCCategoryDecl in ASTReader.cpp.

[vsapsai]

Thanks for the review!

clang/lib/Serialization/ASTReaderDecl.cpp

 //===- ASTReaderDecl.cpp - Decl Deserialization ---------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements the ASTReader::readDeclRecord method, which is the
 // entrypoint for loading a decl.
 //
 //===----------------------------------------------------------------------===//
 
 #include "ASTCommon.h"
 #include "ASTReaderInternals.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/ASTStructuralEquivalence.h"
 #include "clang/AST/Attr.h"
 #include "clang/AST/AttrIterator.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclFriend.h"
 #include "clang/AST/DeclObjC.h"
 #include "clang/AST/DeclOpenMP.h"
class ObjCCategoriesVisitor {
     void add(ObjCCategoryDecl *Cat) {
       // Only process each category once.
       if (!Deserialized.erase(Cat))
         return;
 
       // Check for duplicate categories.
       if (Cat->getDeclName()) {
         ObjCCategoryDecl *&Existing = NameCategoryMap[Cat->getDeclName()];
-        if (Existing &&
-            Reader.getOwningModuleFile(Existing)
-                                          != Reader.getOwningModuleFile(Cat)) {
-          // FIXME: We should not warn for duplicates in diamond:
-          //
-          //   MT     //
-          //  /  \    //
-          // ML  MR   //
-          //  \  /    //
-          //   MB     //
-          //
-          // If there are duplicates in ML/MR, there will be warning when
-          // creating MB *and* when importing MB. We should not warn when
-          // importing.
+        if (Existing && Reader.getOwningModuleFile(Existing) !=
+                            Reader.getOwningModuleFile(Cat)) {
+          llvm::DenseSet<std::pair<Decl *, Decl *>> NonEquivalentDecls;
+          StructuralEquivalenceContext Ctx(
+              Cat->getASTContext(), Existing->getASTContext(),
+              NonEquivalentDecls, StructuralEquivalenceKind::Default,
+              /*StrictTypeSpelling =*/false,
+              /*Complain =*/false,
+              /*ErrorOnTagTypeMismatch =*/true);
+          if (!Ctx.IsEquivalent(Cat, Existing)) {
+            // Warn only if the categories with the same name are different.
             Reader.Diag(Cat->getLocation(), diag::warn_dup_category_def)
                 << Interface->getDeclName() << Cat->getDeclName();
-          Reader.Diag(Existing->getLocation(), diag::note_previous_definition);
+            Reader.Diag(Existing->getLocation(),
+                        diag::note_previous_definition);
+          }
         } else if (!Existing) {
           // Record this category.
           Existing = Cat;
         }
       }
 
       // Add this category to the end of the chain.
       if (Tail)

clang/test/Modules/compare-objc-interface.m

 // expected-note-re@second.h:* {{but in {{'Second'|definition here}} found property 'propAttributes' with different 'readonly' attribute}}
 CompareFirstImplAttribute *compareFirstImplAttribute;
 // expected-error@first.h:* {{'CompareFirstImplAttribute' has different definitions in different modules; first difference is definition in module 'First.Hidden' found property 'firstImplAttribute' with default 'readonly' attribute}}
 // expected-note-re@second.h:* {{but in {{'Second'|definition here}} found property 'firstImplAttribute' with different 'readonly' attribute}}
 CompareLastImplAttribute *compareLastImplAttribute;
 // expected-error@first.h:* {{'CompareLastImplAttribute' has different definitions in different modules; first difference is definition in module 'First.Hidden' found property 'lastImplAttribute' with 'direct' attribute}}
 // expected-note-re@second.h:* {{but in {{'Second'|definition here}} found property 'lastImplAttribute' with different 'direct' attribute}}
 #endif
+
+#if defined(FIRST)
+@interface CompareMatchingCategories: NSObject @end
+@interface CompareMatchingCategories(Matching)
+- (int)testMethod;
+@end
+
+@interface CompareMismatchingCategories1: NSObject @end
+@interface CompareMismatchingCategories1(Category1)
+- (void)presentMethod;
+@end
+@interface CompareMismatchingCategories2: NSObject @end
+@interface CompareMismatchingCategories2(Category2)
+@end
+
+@interface CompareDifferentCategoryNames: NSObject @end
+@interface CompareDifferentCategoryNames(CategoryFirst)
+- (void)firstMethod:(int)x;
+@end
+#elif defined(SECOND)
+@interface CompareMatchingCategories: NSObject @end
+@interface CompareMatchingCategories(Matching)
+- (int)testMethod;
+@end
+
+@interface CompareMismatchingCategories1: NSObject @end
+@interface CompareMismatchingCategories1(Category1)
+@end
+@interface CompareMismatchingCategories2: NSObject @end
+@interface CompareMismatchingCategories2(Category2)
+- (void)presentMethod;
+@end
+
+@interface CompareDifferentCategoryNames: NSObject @end
+@interface CompareDifferentCategoryNames(CategorySecond)
+- (void)secondMethod;
+@end
+#else
+CompareMatchingCategories *compareMatchingCategories; // no diagnostic
+CompareMismatchingCategories1 *compareMismatchingCategories1;
+#ifdef TEST_MODULAR
+// expected-warning@second.h:* {{duplicate definition of category 'Category1' on interface 'CompareMismatchingCategories1'}}
+// expected-note@first.h:* {{previous definition is here}}
+#endif
+CompareMismatchingCategories2 *compareMismatchingCategories2;
+#ifdef TEST_MODULAR
+// expected-warning@second.h:* {{duplicate definition of category 'Category2' on interface 'CompareMismatchingCategories2'}}
+// expected-note@first.h:* {{previous definition is here}}
+#endif
+CompareDifferentCategoryNames *compareDifferentCategoryNames; // no diagnostic
+#endif

clang/test/Modules/hidden-duplicates.m

 @protocol ForwardDeclaredProtocolWithoutDefinition;
 
 id<TestProtocol> protocolDefinition(id<TestProtocol> t);
 id<ForwardDeclaredProtocolWithoutDefinition> forwardDeclaredProtocol(
     id<ForwardDeclaredProtocolWithoutDefinition> t);
 
 @interface NSObject @end
 @class ForwardDeclaredInterfaceWithoutDefinition;
+@interface NSObject(CategoryForTesting) @end
 
 NSObject *interfaceDefinition(NSObject *o);
 NSObject *forwardDeclaredInterface(NSObject *o);
 
 #endif
 
 //--- include/empty.h
 //--- include/initially_hidden.h

clang/test/Modules/objc-categories.m

 // RUN: rm -rf %t
 // RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -x objective-c -fmodule-name=category_top -emit-module %S/Inputs/module.map
 // RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -x objective-c -fmodule-name=category_left -emit-module %S/Inputs/module.map
 // RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -x objective-c -fmodule-name=category_right -emit-module %S/Inputs/module.map
 // RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -x objective-c -fmodule-name=category_bottom -emit-module %S/Inputs/module.map
 // RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -x objective-c -fmodule-name=category_other -emit-module %S/Inputs/module.map
 // RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs %s -verify
 
 @import category_bottom;
 
-// expected-note@Inputs/category_left.h:14 {{previous definition}}
-// expected-warning@Inputs/category_right.h:12 {{duplicate definition of category}}
 // expected-note@Inputs/category_top.h:1 {{receiver is instance of class declared here}}
 
 @interface Foo(Source)
 -(void)source; 
 @end
 
 void test(Foo *foo, LeftFoo *leftFoo) {
   [foo source];