[clangd] Fix getQueryScopes for using-directive with inline namespace

[tom-anders]

Summary

For example, in the following code

#include <string>

using namespace std::string_literals;

int main() {
    strin^ // Completes `string` instead of `std::string`
}

The using declaration would make completion drop the std namespace, even though it shouldn't.

printNamespaceScope() skips inline namespaces, so to fix this use
printQualifiedName() instead

See https://github.com/clangd/clangd/issues/1451

[kadircet]

clang-tools-extra/clangd/CodeComplete.cpp

707

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.

[tom-anders]

clang-tools-extra/clangd/CodeComplete.cpp

707

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).

[kadircet]

clang-tools-extra/clangd/CodeComplete.cpp

707

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?

[tom-anders]

clang-tools-extra/clangd/CodeComplete.cpp

707

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.

[kadircet]

clang-tools-extra/clangd/CodeComplete.cpp

707

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).

[tom-anders]

Address review comment: Introduce new AccessibleScopes variable

[tom-anders]

clang-tools-extra/clangd/CodeComplete.cpp

707

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 :(

[kadircet]

clang-tools-extra/clangd/CodeComplete.cpp

656

could you actually change the comments above to showcase an inline namespace and keep this as AccessibleScopes.

Then introduce QueryScopes with a comment saying that This is an overestimate of AccessibleScopes, e.g. it ignores inline namespaces, to fetch more relevant symbols from index.

664

`Scopes that are accessible from current context. Used for dropping unnecessary namespecifiers.

665

s/scopesForCodeCompletion/scopesForQualificaiton

699–700

can we have a dedicated struct for return type here? especially the "enclosing namespace first" logic seem to be getting out of control. so what about updating SpecificedScope to have two new fields std::optional<std::string> EnclosingNamespace; and a bool AllowAllScopes;? that way we can just call getQueryScopes/getAccessibleScopes on needed places and make sure they're putting the EnclosingNamespace (if it exists) in front.

707

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.

729–730

note that this is actually going to skip inline namespaces (and you're using that in the returned set)

1672–1673

we should be setting AccessibleScopes too

[tom-anders]

Address review comments, add regression test

[tom-anders]

clang-tools-extra/clangd/CodeComplete.cpp

707

That works, thanks!

729–730

Hm I should probably fix this and add another regression test for this..?

1672–1673

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?

1733

Should I add a reserve here for QueryScopes and AccessibleScopes? Would make this a bit more complicated for a small performance boost.

[kadircet]

clang-tools-extra/clangd/CodeComplete.cpp

729–730

yeah, let's just leave a fixme.
i also think we should be setting it in all cases, i can't think of a reason for only setting it in a single case. i believe current scope should always get a boost whenever it's part of query scopes.

1672–1673

looks like this doesn't need any special handling for inline namespaces, does it?

well it probably does, but there isn't much we can do. as this code path happens with only heuristic parsing of the main-file code.

1728

i was actually suggesting to put this logic inside SpecifiedScope::scopesForIndexQuery any reason for only including it in this code path?

1738

AccessibleScopes doesn't need any particular ordering. we can use scopesForQualification as-is.

[tom-anders]

Move EnclosingNamespace logic to scopesForIndexQuery, add FIXME

clang-tools-extra/clangd/CodeComplete.cpp

1728

Ah I just misunderstood, moved the logic to scopesForIndexQuery now.

1738

Might as well just make it a std::set instead of std::vector?

[kadircet]

LGTM, thanks!

clang-tools-extra/clangd/CodeComplete.cpp

683

nit: you can drop the braces

1738

let's leave it as-is for now. i feel like this part can benefit from a bigger refactoring going forward and thinking about vectors is easier then having a bunch of references or sets.

[tom-anders]

Thanks for reviewing!