[CodeCompletion] Reset the hidden declaration obtained after lookup when caching UsingShadowDecls

[arphaman]

Summary

This patch fixes an issue where cached global code completion results for a using declaration included the namespace incorrectly. The issue arises because the global code completion caching performs the lookup in the entire translation unit, which then reports that the UsingDecl is being hidden by UsingShadowDecl. This leads to the nested name qualifiers being applied incorrectly to the completion result.

[ahatanak]

lib/Sema/SemaCodeComplete.cpp

961

I'm not sure about this, but is it correct for VisibleDeclsRecord::checkHidden to return the UsingDecl when the UsingShadowDecl is being passed? I'm thinking perhaps the bug is in that function, since it seems like it should just return a nullptr instead if UsingDecl doesn't hide UsingShadowDecl, .

[arphaman]

Avoid the hidden declaration in VisibleDeclsRecord::checkHidden instead of the decl consumer in code-completion

lib/Sema/SemaCodeComplete.cpp

961

That makes sense I think, I've updated the code to fix VisibleDeclsRecord::checkHidden instead.

[ahatanak]

Looks good to me, but one more comment/question.

lib/Sema/SemaLookup.cpp

3433 ↗

(On Diff #84842)

Do we have to check that ND's UsingDecl is equal to D? Or we don't have to worry about it?

[arphaman]

lib/Sema/SemaLookup.cpp

3433 ↗

(On Diff #84842)

I don't think a check like that would be necessary from what I've observed so far. I believe a UsingShadowDecl could have been hidden previously by a different UsingDecl, but that can happen only in invalid code, like in the example below:

namespace Foo {
    class C { };
}
namespace Bar { class C { }; }
using Foo::C;
using Bar::C;

I don't think it would make sense for Bar::C using decl to hide Foo::C using shadow decl.

[ahatanak]

I had something like the following code in mind (which is not invalid). Does usingdecl Bar::C hide Foo::C ?

namespace Foo {
    class C { };
}
namespace Bar { class C { }; }

using Foo::C;
{
using Bar::C;
}

[ahatanak]

This is not invalid:

namespace Foo {
    class C { };
}
namespace Bar { class C { }; }

void foo1() {
using Foo::C;
{
using Bar::C;
}
}

[arphaman]

In D28514#651675, @ahatanak wrote:

This is not invalid:

namespace Foo {
    class C { };
}
namespace Bar { class C { }; }

void foo1() {
using Foo::C;
{
using Bar::C;
}
}

I see, that could happen if the previous UsingShadow is in an outer scope. Yes, I guess then Foo::C using decl should hide Bar::C using shadow decl. I'll post the patch which checks that the two decls have the same decl context (should be faster than checking if a using shadow decl is contained in a using decl).

[arphaman]

Ensure that the UsingShadowDecl and UsingDecl are in the same DeclContext.

[ahatanak]

In the example I showed, wouldn't the DeclContext for all the UsingDecls and UsingShadowDecls be the FunctionDecl for foo1? Maybe you can check whether UsingShadowDecl::getUsingDecl() (which returns the UsingDecl which is tied to the UsingShadowDecl) is equal to the UsingDecl instead?

[ahatanak]

If they are equal, the loop can continue because a UsingDecl doesn't hide a UsingShadowDecl that is tied to it.

[arphaman]

In D28514#651724, @ahatanak wrote:

If they are equal, the loop can continue because a UsingDecl doesn't hide a UsingShadowDecl that is tied to it.

You're right, that would be better, I didn't notice that method before.

[arphaman]

Verify that the using shadow decl can be hidden by its owning using decl.

[ahatanak]

Thanks LGTM.

lib/Sema/SemaCodeComplete.cpp

void ResultBuilder::AddResult(Result R, DeclContext *CurContext,
   if (R.Kind != Result::RK_Declaration) {
     // For non-declaration results, just add the result.
     Results.push_back(R);
     return;
   }
 
   // Look through using declarations.
   if (const UsingShadowDecl *Using = dyn_cast<UsingShadowDecl>(R.Declaration)) {
+    // When we are gathering global code completions for caching, the lookup
+    // is performed in the translation unit context which leads to a situation
+    // when a UsingShadowDecl hides a UsingDecl, and nested name specifiers are
+    // then incorrectly applied to the target declaration. This can be avoided
+    // by resetting the declaration that's being hidden.
+    if (Hiding && isa<UsingDecl>(Hiding))
+      Hiding = nullptr;
     AddResult(Result(Using->getTargetDecl(),
                      getBasePriority(Using->getTargetDecl()),
                      R.Qualifier),
               CurContext, Hiding);
     return;
   }
   
   bool AsNestedNameSpecifier = false;

test/Index/complete-cached-globals.cpp

+// Note: the run lines follow their respective tests, since line/column
+// matter in this test.
+
+namespace SomeNamespace {
+    class SomeClass {
+    };
+    void SomeFunction();
+}
+
+using SomeNamespace::SomeClass;
+using SomeNamespace::SomeFunction;
+
+static void foo() {
+  return;
+}
+
+// rdar://23454249
+
+// RUN: c-index-test -code-completion-at=%s:14:3 %s | FileCheck -check-prefix=CHECK-CC1 %s
+// RUN: env CINDEXTEST_EDITING=1 CINDEXTEST_COMPLETION_CACHING=1 c-index-test -code-completion-at=%s:14:3 %s | FileCheck -check-prefix=CHECK-CC1 %s
+
+// CHECK-CC1: ClassDecl:{TypedText SomeClass} (50)
+// CHECK-CC1: FunctionDecl:{ResultType void}{TypedText SomeFunction}{LeftParen (}{RightParen )} (50)
+// CHECK-CC1-NOT: {Text SomeNamespace::}{TypedText SomeClass}
+// CHECK-CC1-NOT: {Text SomeNamespace::}{TypedText SomeFunction}