kadircet added inline comments.
================ Comment at: clang-tools-extra/clangd/CodeComplete.cpp:646 // "" for global namespace, "ns::" for normal namespace. + std::vector<std::string> QueryScopes; + // Like QueryScopes, but also contains inline namespaces. ---------------- 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`. ================ Comment at: clang-tools-extra/clangd/CodeComplete.cpp:653 + // Construct scopes to use for code completion. The results are deduplicated. + std::vector<std::string> scopesForCodeCompletion() { ---------------- `Scopes that are accessible from current context. Used for dropping unnecessary namespecifiers. ================ Comment at: clang-tools-extra/clangd/CodeComplete.cpp:654 + // Construct scopes to use for code completion. The results are deduplicated. + std::vector<std::string> scopesForCodeCompletion() { + std::set<std::string> Results; ---------------- s/scopesForCodeCompletion/scopesForQualificaiton ================ Comment at: clang-tools-extra/clangd/CodeComplete.cpp:676 // (e.g. enclosing namespace). -std::pair<std::vector<std::string>, bool> +std::tuple<std::vector<std::string>, std::vector<std::string>, bool> getQueryScopes(CodeCompletionContext &CCContext, const Sema &CCSema, ---------------- 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. ================ Comment at: clang-tools-extra/clangd/CodeComplete.cpp:712 + std::vector<std::string> EnclosingAtFrontForCompletion; std::string EnclosingScope = printNamespaceScope(*CCSema.CurContext); + EnclosingAtFrontForIndex.push_back(EnclosingScope); ---------------- note that this is actually going to skip inline namespaces (and you're using that in the returned set) ================ Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1660 // First scope is the (modified) enclosing scope. QueryScopes = Scopes.scopesForIndexQuery(); ScopeProximity.emplace(QueryScopes); ---------------- we should be setting `AccessibleScopes` too ================ 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) + "::"); ---------------- 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140915/new/ https://reviews.llvm.org/D140915 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits