[Modules][ObjC] Add protocol redefinition to the current scope/context

[bruno]

Summary

Not doing so causes the AST writter to assert since the decl in question never gets emitted. This is fine when modules is not used, but otherwise we need to serialize something other than garbage.

rdar://problem/39844933

[bruno]

Ping!

[arphaman]

lib/Sema/SemaDeclObjC.cpp

1216

Is this a problem only for modules or does this show up in PCHs too? What would be the cost of using PushOnScopeChains unconditionally?

[bruno]

lib/Sema/SemaDeclObjC.cpp

1216

Is this a problem only for modules or does this show up in PCHs too?

It only shows up in PCHs if Modules are being used, otherwise works as expected, because we don't have to serialize a different module which contains the dup, we just ignore it.

What would be the cost of using PushOnScopeChains unconditionally?

I'm afraid that without the modules visibility rules, adding it unconditionally will make it available for name lookup, which we don't want here.

[arphaman]

Thanks for explaining! LGTM

lib/Sema/SemaDeclObjC.cpp

if (ObjCProtocolDecl *Def = PrevDecl? PrevDecl->getDefinition() : nullptr) {
 
     // Create a new protocol that is completely distinct from previous
     // declarations, and do not make this protocol available for name lookup.
     // That way, we'll end up completely ignoring the duplicate.
     // FIXME: Can we turn this into an error?
     PDecl = ObjCProtocolDecl::Create(Context, CurContext, ProtocolName,
                                      ProtocolLoc, AtProtoInterfaceLoc,
                                      /*PrevDecl=*/nullptr);
+
+    // If we are using modules, add the decl to the context in order to
+    // serialize something meaningful.
+    if (getLangOpts().Modules)
+      PushOnScopeChains(PDecl, TUScope);
     PDecl->startDefinition();
   } else {
     if (PrevDecl) {
       // Check for circular dependencies among protocol declarations. This can
       // only happen if this protocol was forward-declared.
       ObjCList<ObjCProtocolDecl> PList;
       PList.set((ObjCProtocolDecl *const*)ProtoRefs, NumProtoRefs, Context);
       err = CheckForwardProtocolDeclarationForCircularDependency(

test/Modules/Inputs/protocol-redefinition/Base.framework/Headers/Base.h

+@protocol Foo
+- (void)someMethodOnFoo;
+@end

test/Modules/Inputs/protocol-redefinition/Base.framework/Modules/module.modulemap

+framework module Base {
+  header "Base.h"
+  export *
+}
 No newline at end of file

test/Modules/Inputs/protocol-redefinition/Kit.framework/Headers/Kit.h

+#import <Base/Base.h>
+
+// REDECLARATION
+@protocol Foo
+- (void)someMethodOnFoo;
+@end

test/Modules/Inputs/protocol-redefinition/Kit.framework/Modules/module.modulemap

+framework module Kit {
+  header "Kit.h"
+  export *
+}
 No newline at end of file

test/Modules/protocol-redefinition.m

+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -F%S/Inputs/protocol-redefinition -fsyntax-only %s -Wno-private-module -verify
+
+// expected-no-diagnostics
+
+@import Kit;