[ASTStructuralEquivalence] Fix crash when ObjCCategoryDecl doesn't have corresponding ObjCInterfaceDecl.

[vsapsai]

Summary

When this happens, it is invalid code and there is diagnostic

error: cannot find interface declaration for '...'

But clang shouldn't crash even if code is invalid. Though subsequent
diagnostic can be imperfect because without ObjCInterfaceDecl we don't have
a type for error messages.

rdar://108818430

[vsapsai]

Kinda a follow-up to D121176.

One big alternative that I've considered is to store interface name in ObjCCategoryDecl because when we parse @interface InterfaceName(CategoryName) we know the interface name. The intention was to allow

@interface A(X) @end

and

@interface A @end
@interface A(X) @end

as equivalent. But I'm not convinced it is the right call and that's why it doesn't justify the extra effort.

[shafik]

clang/lib/AST/ASTStructuralEquivalence.cpp

2062

I think this would be easier to read if you checked Intf1 != Intf2 and then checked for nullptr

[vsapsai]

clang/lib/AST/ASTStructuralEquivalence.cpp

2062

I am totally up to the style that is more readable and consistent. I was just trying to mimic the check for Template1 and Template2. I agree that 1 (one) datapoint isn't representative, so I can check this file more thoroughly for the prevalent style. Do you have any other places in mind that are worth checking? I'll look for something more representative but it would help if you have something in mind already.

2291–2294

Another example of checking 2 elements for nullptr.

[ahatanak]

In D151523#4374808, @vsapsai wrote:

Kinda a follow-up to D121176.

One big alternative that I've considered is to store interface name in ObjCCategoryDecl because when we parse @interface InterfaceName(CategoryName) we know the interface name. The intention was to allow

@interface A(X) @end

and

@interface A @end
@interface A(X) @end

as equivalent. But I'm not convinced it is the right call and that's why it doesn't justify the extra effort.

Are there any cases you are aware of where this change would fix a bug? In any case, it sounds like that is something that can be done as a follow-up patch after fixing the crash.

clang/lib/AST/ASTStructuralEquivalence.cpp

2062

!Intf1 != !Intf2 should work too.

[vsapsai]

Use different check for nullptr + rebase.

[vsapsai]

In D151523#4396010, @ahatanak wrote:

In D151523#4374808, @vsapsai wrote:

Kinda a follow-up to D121176.

One big alternative that I've considered is to store interface name in ObjCCategoryDecl because when we parse @interface InterfaceName(CategoryName) we know the interface name. The intention was to allow

@interface A(X) @end

and

@interface A @end
@interface A(X) @end

as equivalent. But I'm not convinced it is the right call and that's why it doesn't justify the extra effort.

Are there any cases you are aware of where this change would fix a bug? In any case, it sounds like that is something that can be done as a follow-up patch after fixing the crash.

No, I don't know about any such cases. My conclusion is that a category without an interface is invalid code, so trying to do something smart in this situation is a questionable approach. That's why I've decided not to do anything smart for a missing interface.

clang/lib/AST/ASTStructuralEquivalence.cpp

2062

So after checking for the common patterns I've found the following examples

if (!Child1 || !Child2)
  return false;

if (!S1 || !S2)
  return S1 == S2;

if (T1.isNull() || T2.isNull())
  return T1.isNull() && T2.isNull();

if (TemplateDeclN1 && TemplateDeclN2) {
  ...
} else if (TemplateDeclN1 || TemplateDeclN2)
  return false;

So I went for (!Intf1 || !Intf2) check and added (Intf1 != Intf2) because we can check further if both interfaces are nullptr. The boolean expression is more complex than before but it captures my thought process better.

[Bigcheese]

Looks good as long as people are happy with the condition.

[vsapsai]

If anybody has further comments about the condition style, I'm happy to change the code post-commit.

clang/lib/AST/ASTStructuralEquivalence.cpp

 }
 
 static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
                                      ObjCCategoryDecl *D1,
                                      ObjCCategoryDecl *D2) {
   if (!IsStructurallyEquivalent(D1->getIdentifier(), D2->getIdentifier()))
     return false;
 
-  if (!IsStructurallyEquivalent(D1->getClassInterface()->getIdentifier(),
-                                D2->getClassInterface()->getIdentifier()))
+  const ObjCInterfaceDecl *Intf1 = D1->getClassInterface(),
+                          *Intf2 = D2->getClassInterface();
+  if ((!Intf1 || !Intf2) && (Intf1 != Intf2))
+    return false;
+
+  if (Intf1 &&
+      !IsStructurallyEquivalent(Intf1->getIdentifier(), Intf2->getIdentifier()))
     return false;
 
   // Compare protocols.
   ObjCCategoryDecl::protocol_iterator Protocol2 = D2->protocol_begin(),
                                       Protocol2End = D2->protocol_end();
   for (ObjCCategoryDecl::protocol_iterator Protocol1 = D1->protocol_begin(),
                                            Protocol1End = D1->protocol_end();
        Protocol1 != Protocol1End; ++Protocol1, ++Protocol2) {
     if (Protocol2 == Protocol2End)
       return false;
     if (!IsStructurallyEquivalent((*Protocol1)->getIdentifier(),
                                   (*Protocol2)->getIdentifier()))
       return false;
   }
   if (Protocol2 != Protocol2End)
     return false;
 
   // Compare ivars.
-  QualType D2Type = Context.ToCtx.getObjCInterfaceType(D2->getClassInterface());
+  QualType D2Type =
+      Intf2 ? Context.ToCtx.getObjCInterfaceType(Intf2) : QualType();
   ObjCCategoryDecl::ivar_iterator Ivar2 = D2->ivar_begin(),
                                   Ivar2End = D2->ivar_end();
   for (ObjCCategoryDecl::ivar_iterator Ivar1 = D1->ivar_begin(),
                                        Ivar1End = D1->ivar_end();
        Ivar1 != Ivar1End; ++Ivar1, ++Ivar2) {
     if (Ivar2 == Ivar2End)
       return false;
     if (!IsStructurallyEquivalent(Context, *Ivar1, *Ivar2, D2Type))
bool StructuralEquivalenceContext::IsEquivalent(Stmt *S1, Stmt *S2) {
 
   return !Finish();
 }
 
 bool StructuralEquivalenceContext::CheckCommonEquivalence(Decl *D1, Decl *D2) {
   // Check for equivalent described template.
   TemplateDecl *Template1 = D1->getDescribedTemplate();
   TemplateDecl *Template2 = D2->getDescribedTemplate();
   if ((Template1 != nullptr) != (Template2 != nullptr))
     return false;
   if (Template1 && !IsStructurallyEquivalent(*this, Template1, Template2))
     return false;
 
   // FIXME: Move check for identifier names into this function.
 
   return true;
 }
 
 bool StructuralEquivalenceContext::CheckKindSpecificEquivalence(
     Decl *D1, Decl *D2) {

clang/unittests/AST/StructuralEquivalenceTest.cpp

 
 TEST_F(StructuralEquivalenceObjCCategoryTest, CategoriesWithDifferentNames) {
   auto t = makeDecls<ObjCCategoryDecl>("@interface A @end @interface A(X) @end",
                                        "@interface A @end @interface A(Y) @end",
                                        Lang_OBJC, objcCategoryDecl());
   EXPECT_FALSE(testStructuralMatch(t));
 }
 
+TEST_F(StructuralEquivalenceObjCCategoryTest, CategoriesWithoutInterfaces) {
+  auto t = makeDecls<ObjCCategoryDecl>("                  @interface A(X) @end",
+                                       "@interface A @end @interface A(X) @end",
+                                       Lang_OBJC, objcCategoryDecl());
+  EXPECT_FALSE(testStructuralMatch(t));
+
+  auto t2 = makeDecls<ObjCCategoryDecl>("@interface A(X) @end",
+                                        "@interface A(X) @end",
+                                        Lang_OBJC, objcCategoryDecl());
+  EXPECT_TRUE(testStructuralMatch(t2));
+}
+
 TEST_F(StructuralEquivalenceObjCCategoryTest, CategoryAndExtension) {
   auto t = makeDecls<ObjCCategoryDecl>("@interface A @end @interface A(X) @end",
                                        "@interface A @end @interface A() @end",
                                        Lang_OBJC, objcCategoryDecl());
   EXPECT_FALSE(testStructuralMatch(t));
 }
 
 TEST_F(StructuralEquivalenceObjCCategoryTest, MatchingProtocols) {