[PATCH] D140915: [clangd] Fix getQueryScopes for using-directive with inline namespace
Tom Praschan via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Sun Jan 29 07:54:40 PST 2023
tom-anders marked an inline comment as done.
tom-anders added inline comments.
================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1714
+ }
+ llvm::copy_if(SpecifiedScopes.scopesForIndexQuery(),
+ std::back_inserter(QueryScopes),
----------------
Should I add a `reserve` here for QueryScopes and AccessibleScopes? Would make this a bit more complicated for a small performance boost.
================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:712
+ std::vector<std::string> EnclosingAtFrontForCompletion;
std::string EnclosingScope = printNamespaceScope(*CCSema.CurContext);
+ EnclosingAtFrontForIndex.push_back(EnclosingScope);
----------------
kadircet wrote:
> note that this is actually going to skip inline namespaces (and you're using that in the returned set)
Hm I should probably fix this and add another regression test for this..?
================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1660
// First scope is the (modified) enclosing scope.
QueryScopes = Scopes.scopesForIndexQuery();
ScopeProximity.emplace(QueryScopes);
----------------
kadircet wrote:
> we should be setting `AccessibleScopes` too
Ah thanks, that's what caused the one failing test. I just copied over QueryScopes here for now, looks like this doesn't need any special handling for inline namespaces, does it?
================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:674
+ else if (const auto *ND = dyn_cast<NamespaceDecl>(Context)) {
+ if (ND->isInlineNamespace())
+ Scopes.AccessibleScopes.push_back(printQualifiedName(*ND) + "::");
----------------
kadircet wrote:
> tom-anders wrote:
> > kadircet wrote:
> > > tom-anders wrote:
> > > > kadircet wrote:
> > > > > tom-anders wrote:
> > > > > > kadircet wrote:
> > > > > > > since we know that the `Context` is a `NamespaceDecl` it should be safe to use `printQualifiedName` always. any reason for the extra branching here (apart from minimizing the change to behaviour)? if not I think we can get rid of the special casing.
> > > > > > Unfortunately, this fails CompletionTest.EnclosingScopeComesFirst and CompletionTest.NoDuplicatedQueryScopes, I think because of anonymous namespaces (where printNameSpaceScope would return an empty string, but (printQualifiedName(*ND) + "::" does not).
> > > > > i see. taking a closer look at this `getQueryScopes` is used for two things:
> > > > > - The scopes to query with fuzzyfind requests, hence this should use the same "serialization" as symbolcollector (which only strips anon namespaces today, but initially it were to strip both anon & inline namespaces. it got changed inside clang without clangd tests catching it).
> > > > > - The shortening of the fully qualified name in `CodeCompletionBuilder`. Not having inline namespaces spelled in the available namespaces implies getting wrong qualifiers (such as the bug you're fixing).
> > > > >
> > > > > so considering the requirements here:
> > > > > - when querying index, we actually want to hide inline namespaces (as `ns::hidden::Foo` should be a viable alternative even if only `ns::` is accessible). so we should actually fix `printQualifiedName` to set `SuppressInlineNamespace` in printing policy to restore the old behaviour (and keep using `printNamespaceScope` here).
> > > > > - inside `CodeCompletionBuilder`, we shouldn't use the same scopes we use during index queries. we should use the visible namespaces while preserving inline namespace information and only ignoring the anonymous namespaces.
> > > > >
> > > > > hence can we have 2 separate scopes in `CodeCompleteFlow` instead?
> > > > > One called `QueryScopes`, which has the behavior we have today (fixing printQualifiedName is a separate issues).
> > > > > Other called `AccessibleScopes`, which has accessible namespaces spelled **with** inline namespaces, so that we can get proper qualification during code-complete.
> > > > >
> > > > > does that make sense?
> > > > tbh I'm a bit confused - I understand your requirements, but am not sure I understand your proposed solution. Can you expand a bit further? Looking at the code, there are already both `QueryScopes` and `AccessibleScopes` variables/fields in various classes, I'm not really sure at which places you want to make changes.
> > > sorry for the long and confusing answer :D
> > >
> > > I was talking about `CodeCompleteFlow` class specifically, inside `CodeComplete.cpp`. Currently it only has `QueryScopes`, derived from the visible contexts reported by Sema. Unfortunately it loses some granularity to fetch more symbols from index hence it should not be used when picking the required qualifier.
> > > My suggestion is to add a new field in `CodeCompleteFlow` called `AccessibleScope`, which is derived at the same stage as `QueryScopes`, while preserving inline namespaces, and used when creating `CodeCompletionBuilder` in `CodeCompleteFlow::toCodeCompletion` (instead of `QueryScopes`).
> > Ok, think I got it - To make this a bit less confusing, I first renamed SpecifiedScope::AccessibleScopes to SpecifiedScope::QueryScopes (that's the number under which it is (and was) stored in CodeCompleteFlow anyway) and then added a back a new field AccessibleScopes, that keeps the inline namespaces. This is then stored in CodeCompleteFlow:AccessibleScopes and passed to the CodeCompletionBuilder instead of QueryScopes.
> >
> > This now still passes all existing tests and I verified in my editor that it also fixes the original issue.
> >
> > I couldn't figure out how to write a regression test for this now though :(
> > I couldn't figure out how to write a regression test for this now though :(
>
> We've got a bunch of tests in `unittests/CodeCompleteTests.cpp`, something like:
> ```
> TEST(CompletionTest, QualificationWithInlineNamespace) {
> auto Results = completions(R"cpp(
> namespace a { inline namespace b {} }
> using namespace a::b;
> void f() { Foo^ }
> )cpp",
> {cls("a::Foo")});
> EXPECT_THAT(Results.Completions,
> UnorderedElementsAre(AllOf(qualifier("a::"), named("Foo"))));
> }
> ```
>
> should fail without your patch, and pass afterwards.
That works, thanks!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D140915/new/
https://reviews.llvm.org/D140915
More information about the cfe-commits
mailing list