[modules] While merging ObjCInterfaceDecl definitions, merge them as decl contexts too.

[vsapsai]

Summary

While working on https://reviews.llvm.org/D110280 I've tried to merge
decl contexts as it seems to be correct and matching our handling of
decl contexts from different modules. It's not required for the fix in
https://reviews.llvm.org/D110280 but it revealed a missing diagnostic,
so separating this change into a separate commit.

Renamed some variables to distinguish diagnostic like "declaration of
'x' does not match" for different cases.

[vsapsai]

Rebase as the pre-requisite change has landed.

[vsapsai]

Pre-merge checks are passing now after the rebase. I believe it is a straightforward change and we are merging decl contexts for other Decls already, so merging them for ObjCInterfaceDecl makes sense. If there are no objections, I plan to land the change on Friday, October 22. If any issues come up later, post-commit reviews are welcome as always.

[dblaikie]

In D110287#3073804, @vsapsai wrote:

Pre-merge checks are passing now after the rebase. I believe it is a straightforward change and we are merging decl contexts for other Decls already, so merging them for ObjCInterfaceDecl makes sense. If there are no objections, I plan to land the change on Friday, October 22. If any issues come up later, post-commit reviews are welcome as always.

Generally this sort of "if no one says anything I'll commit at this time" thing is to be avoided: The idea is that once something's been sent for review/the author has requested a second opinion, we want to avoid people committing that code without review only due to lack of feedback. Please reach out to reviewers to get a second set of eyes on this before committing.

[rsmith]

clang/lib/Serialization/ASTReaderDecl.cpp

1181–1184

A couple of other things you should consider doing in this case:

• If there's an AST invariant that there is only one definition, think about how to maintain that invariant. For other kinds of declaration, we would update NewDD to make it act as a forward-declaration, but I'm not sure we can do much about it here given that an @interface is never a forward-declaration. Maybe for @interface there's nothing we can really do, and code dealing with these declarations just needs to handle there possibly being more than one definition?
• Merge the definition visibility of the new definition into the canonical definition. This is necessary to ensure that in code for which the new definition is imported and the canonical definition is not, we still treat the canonical definition as being visible. (Eg, module A has the canonical definition, module B has another definition, module C imports A but doesn't re-export it, and C also imports B and re-exports it, and then you import C and try to use the class.)

For the latter, Reader.mergeDefinitionVisibility(DD.Definition, NewDD.Definition) should be sufficient, assuming that all code treats the first definition as being the canonical one (for example, name lookups are performed into the first definition).

[vsapsai]

In D110287#3074082, @dblaikie wrote:

In D110287#3073804, @vsapsai wrote:

Pre-merge checks are passing now after the rebase. I believe it is a straightforward change and we are merging decl contexts for other Decls already, so merging them for ObjCInterfaceDecl makes sense. If there are no objections, I plan to land the change on Friday, October 22. If any issues come up later, post-commit reviews are welcome as always.

Generally this sort of "if no one says anything I'll commit at this time" thing is to be avoided: The idea is that once something's been sent for review/the author has requested a second opinion, we want to avoid people committing that code without review only due to lack of feedback. Please reach out to reviewers to get a second set of eyes on this before committing.

What would be a better way to deal with changes in areas where I'm more experienced in (Obj-C decl merging on AST deserialization) and still want to run pre-merge checks on other platforms? I got unofficial sign-offs internally but people defer to me because I've worked more in this area.

I believe this change is pretty trivial, while for D110452 I want another opinion. But one-line change in D110453 is trivial too and I don't think that waiting for reviews for half a year would improve its quality substantially.

[vsapsai]

clang/lib/Serialization/ASTReaderDecl.cpp

1181–1184

Change to merge visibility is D110453 and you are right, the code is simple, the main complexity is the test.

As for maintaining a single definition, we are already doing that (see my comment a few lines below in ASTDeclReader::VisitObjCInterfaceDecl). But not doing good job with ObjCInterfaceType::getDecl. The fix for that is D110452 but the summary is that ObjCInterfaceType stores the last encountered definition, it is never updated when we discard this definition, that not-so-definition-anymore is treated as a canonical definition.

Updating visibility doesn't work without D110452 because we might end up looking up a different "definition", not the one with correct[ed] visibility. And D110452 seems to be different enough, so I've split it out as a separate patch. I've made current change a separate patch for clear indication what code changes lead to changes in clang/test/Modules/odr_hash.mm, there is no technical reason it cannot be merged with other changes.

Also I should note that @interface tracking story isn't over yet, I still need to check and write tests for merging categories. They cannot have ivars, so don't expect IRGen issues but there is still enough room for other bugs.

1202–1206

Here we keep a single definition. Have a last chance to keep extra information from the second definition but afterwards just overwrite ID->Data.

[rsmith]

clang/lib/Serialization/ASTReaderDecl.cpp

1181–1184

OK, separating out those changes with separate tests makes sense to me. Thanks!

[dblaikie]

In D110287#3074175, @vsapsai wrote:

In D110287#3074082, @dblaikie wrote:

In D110287#3073804, @vsapsai wrote:

Pre-merge checks are passing now after the rebase. I believe it is a straightforward change and we are merging decl contexts for other Decls already, so merging them for ObjCInterfaceDecl makes sense. If there are no objections, I plan to land the change on Friday, October 22. If any issues come up later, post-commit reviews are welcome as always.

Generally this sort of "if no one says anything I'll commit at this time" thing is to be avoided: The idea is that once something's been sent for review/the author has requested a second opinion, we want to avoid people committing that code without review only due to lack of feedback. Please reach out to reviewers to get a second set of eyes on this before committing.

What would be a better way to deal with changes in areas where I'm more experienced in (Obj-C decl merging on AST deserialization) and still want to run pre-merge checks on other platforms? I got unofficial sign-offs internally but people defer to me because I've worked more in this area.

Oh, sure - you can use Phabricator's draft support to just send the change for CI testing without sending a review to the mailing list: https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-command-line

I believe this change is pretty trivial, while for D110452 I want another opinion. But one-line change in D110453 is trivial too and I don't think that waiting for reviews for half a year would improve its quality substantially.

Reviews shouldn't/generally don't take half a year, especially for smaller, clearly correct changes. Pinging reviews if they're not getting traction can help get some attention, though sometimes more direct calls for assistance may be necessary/beneficial.

[vsapsai]

Thanks for the review! I'll commit this and other changes in the stack after testing with the current "main" and making sure the bots on other platforms are happy.

[vsapsai]

In D110287#3075590, @dblaikie wrote:

In D110287#3074175, @vsapsai wrote:

What would be a better way to deal with changes in areas where I'm more experienced in (Obj-C decl merging on AST deserialization) and still want to run pre-merge checks on other platforms? I got unofficial sign-offs internally but people defer to me because I've worked more in this area.

Oh, sure - you can use Phabricator's draft support to just send the change for CI testing without sending a review to the mailing list: https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-command-line

D'oh, haven't realized that. Thanks for the advice!

I believe this change is pretty trivial, while for D110452 I want another opinion. But one-line change in D110453 is trivial too and I don't think that waiting for reviews for half a year would improve its quality substantially.

Reviews shouldn't/generally don't take half a year, especially for smaller, clearly correct changes. Pinging reviews if they're not getting traction can help get some attention, though sometimes more direct calls for assistance may be necessary/beneficial.

Thanks, appreciate the advice. It is a sore subject for me, so I'll refrain from digging myself a deeper hole.

clang/lib/Serialization/ASTReaderDecl.cpp

void ASTDeclReader::ReadObjCDefinitionData(
   for (unsigned I = 0; I != NumProtocols; ++I)
     Protocols.push_back(readDeclAs<ObjCProtocolDecl>());
   Data.AllReferencedProtocols.set(Protocols.data(), NumProtocols,
                                   Reader.getContext());
 }
 
 void ASTDeclReader::MergeDefinitionData(ObjCInterfaceDecl *D,
          struct ObjCInterfaceDecl::DefinitionData &&NewDD) {
+  struct ObjCInterfaceDecl::DefinitionData &DD = D->data();
+  if (DD.Definition != NewDD.Definition) {
+    Reader.MergedDeclContexts.insert(
+        std::make_pair(NewDD.Definition, DD.Definition));
+  }
+
   // FIXME: odr checking?
 }
 
 void ASTDeclReader::VisitObjCInterfaceDecl(ObjCInterfaceDecl *ID) {
   RedeclarableResult Redecl = VisitRedeclarable(ID);
   VisitObjCContainerDecl(ID);
   DeferredTypeID = Record.getGlobalTypeID(Record.readInt());
   mergeRedeclarable(ID, Redecl);
 
   ID->TypeParamList = ReadObjCTypeParamList();
   if (Record.readInt()) {
     // Read the definition.
     ID->allocateDefinitionData();
 
     ReadObjCDefinitionData(ID->data());
     ObjCInterfaceDecl *Canon = ID->getCanonicalDecl();
     if (Canon->Data.getPointer()) {
       // If we already have a definition, keep the definition invariant and
       // merge the data.
       MergeDefinitionData(Canon, std::move(ID->data()));
       ID->Data = Canon->Data;
     } else {
       // Set the definition data of the canonical declaration, so other
       // redeclarations will see it.
       ID->getCanonicalDecl()->Data = ID->Data;
 
       // We will rebuild this list lazily.
       ID->setIvarList(nullptr);
     }

clang/test/Modules/odr_hash.mm

 #if defined(FIRST)
 @interface Interface4 <T : I1 *> {
 @public
   T<P1> x;
 }
 @end
 @interface Interface5 <T : I1 *> {
 @public
-  T<P1> x;
+  T<P1> y;
 }
 @end
 @interface Interface6 <T1 : I1 *, T2 : I2 *> {
 @public
-  T1 x;
+  T1 z;
 }
 @end
 #elif defined(SECOND)
 @interface Interface4 <T : I1 *> {
 @public
   T<P2> x;
 }
 @end
 @interface Interface5 <T : I1 *> {
 @public
-  T<P1, P2> x;
+  T<P1, P2> y;
 }
 @end
 @interface Interface6 <T1 : I1 *, T2 : I2 *> {
 @public
-  T2 x;
+  T2 z;
 }
 @end
+#else
+// expected-error@first.h:* {{'Interface4::x' from module 'FirstModule' is not present in definition of 'Interface4' in module 'SecondModule'}}
+// expected-note@second.h:* {{declaration of 'x' does not match}}
+// expected-error@first.h:* {{'Interface5::y' from module 'FirstModule' is not present in definition of 'Interface5' in module 'SecondModule'}}
+// expected-note@second.h:* {{declaration of 'y' does not match}}
+// expected-error@first.h:* {{'Interface6::z' from module 'FirstModule' is not present in definition of 'Interface6' in module 'SecondModule'}}
+// expected-note@second.h:* {{declaration of 'z' does not match}}
 #endif
 
 namespace Types {
 namespace ObjCTypeParam {
 #if defined(FIRST) || defined(SECOND)
 struct Invalid1 {
   Interface4 *I;
   decltype(I->x) x;
 };
 struct Invalid2 {
   Interface5 *I;
-  decltype(I->x) x;
+  decltype(I->y) y;
 };
 struct Invalid3 {
   Interface6 *I;
-  decltype(I->x) x;
+  decltype(I->z) z;
 };
 #else
 Invalid1 i1;
 // expected-error@first.h:* {{'Types::ObjCTypeParam::Invalid1::x' from module 'FirstModule' is not present in definition of 'Types::ObjCTypeParam::Invalid1' in module 'SecondModule'}}
 // expected-note@second.h:* {{declaration of 'x' does not match}}
 Invalid2 i2;
-// expected-error@first.h:* {{'Types::ObjCTypeParam::Invalid2::x' from module 'FirstModule' is not present in definition of 'Types::ObjCTypeParam::Invalid2' in module 'SecondModule'}}
-// expected-note@second.h:* {{declaration of 'x' does not match}}
+// expected-error@first.h:* {{'Types::ObjCTypeParam::Invalid2::y' from module 'FirstModule' is not present in definition of 'Types::ObjCTypeParam::Invalid2' in module 'SecondModule'}}
+// expected-note@second.h:* {{declaration of 'y' does not match}}
 Invalid3 i3;
-// expected-error@first.h:* {{'Types::ObjCTypeParam::Invalid3::x' from module 'FirstModule' is not present in definition of 'Types::ObjCTypeParam::Invalid3' in module 'SecondModule'}}
-// expected-note@second.h:* {{declaration of 'x' does not match}}
+// expected-error@first.h:* {{'Types::ObjCTypeParam::Invalid3::z' from module 'FirstModule' is not present in definition of 'Types::ObjCTypeParam::Invalid3' in module 'SecondModule'}}
+// expected-note@second.h:* {{declaration of 'z' does not match}}
 #endif
 
 }  // namespace ObjCTypeParam
 }  // namespace Types
 
 namespace CallMethods {
 #if defined(FIRST)
 void invalid1(EmptySelectorSlot *obj) {