Page MenuHomePhabricator

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

Authored by arphaman on Jan 10 2017, 4:50 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

arphaman updated this revision to Diff 83796.Jan 10 2017, 4:50 AM
arphaman retitled this revision from to [CodeCompletion] Reset the hidden declaration obtained after lookup when caching UsingShadowDecls.
arphaman updated this object.
arphaman added reviewers: manmanren, ahatanak, akyrtzi.
arphaman set the repository for this revision to rL LLVM.
arphaman added a subscriber: cfe-commits.
ahatanak added inline comments.Jan 17 2017, 1:05 PM
lib/Sema/SemaCodeComplete.cpp
961 ↗(On Diff #83796)

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 updated this revision to Diff 84842.Jan 18 2017, 8:25 AM
arphaman marked an inline comment as done.

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

lib/Sema/SemaCodeComplete.cpp
961 ↗(On Diff #83796)

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

ahatanak edited edge metadata.Jan 19 2017, 12:02 PM

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 added inline comments.Jan 20 2017, 9:22 AM
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.

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;
}

This is not invalid:

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

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

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 updated this revision to Diff 85151.Jan 20 2017, 9:47 AM

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

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?

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

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 updated this revision to Diff 85398.Jan 23 2017, 7:58 AM

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

ahatanak accepted this revision.Jan 23 2017, 9:08 AM

Thanks LGTM.

This revision is now accepted and ready to land.Jan 23 2017, 9:08 AM
This revision was automatically updated to reflect the committed changes.