Page MenuHomePhabricator

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

Authored by tom-anders on Jan 3 2023, 11:58 AM.



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


Diff Detail

Event Timeline

tom-anders created this revision.Jan 3 2023, 11:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 3 2023, 11:58 AM
Herald added a subscriber: arphaman. · View Herald Transcript
tom-anders requested review of this revision.Jan 3 2023, 11:58 AM
tom-anders edited the summary of this revision. (Show Details)Jan 3 2023, 11:59 AM
kadircet added inline comments.Jan 4 2023, 12:04 AM

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 added inline comments.Jan 4 2023, 10:55 AM

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 added inline comments.Jan 5 2023, 5:32 AM

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 added inline comments.Jan 8 2023, 10:19 AM

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 added inline comments.Jan 9 2023, 12:01 AM

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

Address review comment: Introduce new AccessibleScopes variable

tom-anders added inline comments.Jan 14 2023, 11:15 AM

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 added inline comments.Jan 18 2023, 5:04 AM

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.


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




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.


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^ }
              UnorderedElementsAre(AllOf(qualifier("a::"), named("Foo"))));

should fail without your patch, and pass afterwards.


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


we should be setting AccessibleScopes too

tom-anders marked 8 inline comments as done.

Address review comments, add regression test

tom-anders marked an inline comment as done.Jan 29 2023, 7:54 AM
tom-anders added inline comments.

That works, thanks!


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


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?


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

kadircet added inline comments.Feb 1 2023, 1:19 AM

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.


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.


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


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

tom-anders updated this revision to Diff 494074.Feb 1 2023, 2:29 PM
tom-anders marked 4 inline comments as done.

Move EnclosingNamespace logic to scopesForIndexQuery, add FIXME


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


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

kadircet accepted this revision.Feb 6 2023, 12:44 AM

LGTM, thanks!


nit: you can drop the braces


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.

This revision is now accepted and ready to land.Feb 6 2023, 12:44 AM

Thanks for reviewing!

This revision was landed with ongoing or failed builds.Feb 9 2023, 10:53 AM
This revision was automatically updated to reflect the committed changes.