[PATCH] D144976: [clangd] Add provider info on symbol hover.
Viktoriia Bakalova via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Mar 17 10:33:32 PDT 2023
VitaNuo added a comment.
Thanks for the comments and the discussions!
================
Comment at: clang-tools-extra/clangd/Hover.cpp:1118
+
+ for (const auto &H : Headers) {
+ if (H.kind() == include_cleaner::Header::Physical &&
----------------
hokein wrote:
> now the for-range loop doesn't seem necessary, we could always use the `Headers.front()`, right?
Ok. It seems like I need to put an `empty` check on `Headers`, however, because contrary to the loop, `front()` might actually crash AFAIU.
================
Comment at: clang-tools-extra/clangd/Hover.cpp:1219
maybeAddCalleeArgInfo(N, *HI, PP);
+ maybeAddSymbolProviders(AST, *HI, include_cleaner::Symbol{*DeclToUse});
} else if (const Expr *E = N->ASTNode.get<Expr>()) {
----------------
hokein wrote:
> I wonder how well it works for the `NamespaceDecl`. NamespaceDecl is usually declared in many files, we will basically show a random provider in hover. We're not interested in `NamesapceDecl`, we probably need to ignore it.
>
> No action required here, but this is something we should bear in mind.
Thank you.
================
Comment at: clang-tools-extra/clangd/Hover.cpp:1138
+ if (H.kind() == include_cleaner::Header::Physical &&
+ H.physical() == SM.getFileEntryForID(SM.getMainFileID()))
+ continue;
----------------
hokein wrote:
> VitaNuo wrote:
> > hokein wrote:
> > > MainFile provider is a special case (I don't recall the details).
> > >
> > > IIUC, the model is:
> > >
> > > 1) a symbol usage that is satisfied (e.g. any of its providers that are directly included in the main file), we show the one with highest rank of these included providers
> > > 2) a symbol usage that is not satisfied (we choose the highest rank of all providers)
> > > 3) If the provider is the main-file, we don't show it in the hover card.
> > >
> > > Based on 1), if the main-file provider is the highest, we will not show it in the hover based on 3). However, the current implementation doesn't match this behavior
> > > -- on L1123 `ConvertedIncludes.match(H)` is always false if H is a main-file, and we will choose a lower-rank provider if the main-file is the first element of `Headers`
> > > -- the logic here doesn't seem to work, we should do a `break` on L1139 rather than `continue`, which means we always use the `Headers[0]` element.
> > >
> > > Not sure we have discussed 3), one alternative is to show the information for main-file provider as well, it seems fine to me that the hover shows `provided by the current file` text (not the full path).
> > > we should do a break on L1139 rather than continue
> >
> > Ok. I'm not sure if this is of great practical importance (what are the chances that the main file is the first provider, and there are more providers for the same symbol?),
> > but you're right that we shouldn't show the lower-ranked provider for this case.
> >
> > > one alternative is to show the information for main-file provider as well
> >
> > Yeah, this is possible ofc, but I'm not sure what the use would be. The general intention of this feature (or feature set, even) is to help users eliminate unnecessary dependencies, and they can hardly get rid of the main file :)
> > So of the two options, I'd rather opt for not showing anything at all.
> > I'm not sure if this is of great practical importance
> I think the code should match our mental model, otherwise it is hard to reason about whether the actual behavior is expected or a bug.
>
> > (what are the chances that the main file is the first provider, and there are more providers for the same symbol?),
>
> I think the pattern is not rare (especially for headers), think of the case below.
>
> ```
> #include "other.h" // other.h transitively includes the `foo.h`
>
> class Foo;
> const Foo& createFoo();
> ```
>
> although the main-file provider is not the top1 given the current ranking implementation, we have a plan to address it, see the FIXME in https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/include-cleaner/lib/FindHeaders.cpp#L219. After addressing the FIXME, the main-file could be the top1.
>
> > on L1123 ConvertedIncludes.match(H) is always false if H is a main-file, and we will choose a lower-rank provider if the main-file is the first element of Headers
>
> This comment doesn't seem to be addressed, now it is L1105.
>
> Given the following case, if the returned providers is [main-file, `foo.h`],
> the current code will show `foo.h` in hover.
> However, based on our mental model:
> 1) the main-file is one of the `Foo` providers, and it is the top1, we choose it
> 2) we don't show main-file provider in hover
> -> we should not show any information in hover.
>
> ```
> #include "foo.h"
>
> class Foo;
> Foo* b; // hover on `Foo`.
> ```
> I think the pattern is not rare (especially for headers), think of the case below.
Makes sense, I didn't think about headers at all.
> This comment doesn't seem to be addressed, now it is L1105.
Oops. See the current version please.
================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:258
+std::vector<include_cleaner::SymbolReference>
+collectMacroReferences(ParsedAST &AST) {
+ const auto &SM = AST.getSourceManager();
----------------
hokein wrote:
> let's keep it unchanged, we don't need any change for this function in this patch.
oh it's not changed, just got moved because of the other two. But no worries, I can put it exactly in the previous position.
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2897
+ const std::function<void(HoverInfo &)> ExpectedBuilder;
+ } Cases[] = {
+ {Annotations(
----------------
hokein wrote:
> I have some trouble following the test here:
>
> - this is a long list of testcases (see my other comments)
> - we construct each testcase with five fields, it is hard to track which content is for foo.h/bar.h/foobar.h
>
> I'd suggest to find way to simplify it, some ideas
> - we can use a fixed content foo.h, foo_fwd.h for all testcases (I guess something like below should be enough to cover the major cases), so that we don't need to specify all these content in every testcase.
> - only test critical cases 1) symbol ref is satisfied (by the main-file provider or by regular headers) 2) symbol ref is not satisfied, verify we show first provider of `Providers`. And test 3 different symbol kinds (regular decl, macro, operator)
>
>
> // foo.h
> #define FOO 1
> class Foo {};
> Foo& operator+(const Foo, const Foo);
>
> // foo_fwd.h
> class Foo;
>
> // all.h
> #include "foo.h"
> #include "foo_fwd.h"
Ok, I have simplified the test considerably. Please take a look.
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2922
+ #include "foo.h"
+ int F = [[^foo]]();
+ )cpp"),
----------------
hokein wrote:
> unclear to me the intention of this testcase.
Testing that `foo.h` is ranked higher since it's providing the symbol directly. But maybe this makes no sense, I will remove it then.
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:2948
+
+ Foo [[f^oo]] = 42;
+ )cpp"),
----------------
hokein wrote:
> this case seems to be duplicated with the first one, can be removed, I think.
yeah this is also provided by the main file
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:3008
+ "#define MACRO 5",
+ "#define MACRO 10", "",
+ [](HoverInfo &HI) { HI.Provider = "\"foo.h\""; }},
----------------
hokein wrote:
> This will have macro-redefined warnings, `foo.h` is better than `bar.h` because the MACRO in main-file refers to the one defined in `foo.h`. Not sure the value of this testcase, I would suggest dropping it for simplicity.
>
> The same for the below one.
ok.
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:3023
+ #include "foo.h"
+ int [[^F]] = MACRO(5);
+ )cpp"),
----------------
hokein wrote:
> this seems to be duplicated with the first testcase, nothing related to macro, can be dropped as well.
ok.
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:3123
+
+ HoverInfo HIFooBar;
+ HIFooBar.Name = "foo";
----------------
hokein wrote:
> What's the difference between `HIFoo` and `HIFooBar`? Looks like they are the same.
>
> I guess you want to check the `<>` and `""` cases?
yes, thank you.
================
Comment at: clang-tools-extra/clangd/unittests/HoverTests.cpp:3139
+ #include "absl_string_view.h"
+ absl::str^ing_view abc; // hover on string_view
+ )cpp");
----------------
hokein wrote:
> I think we can simplify this test and merge it to the above `TEST(Hover, Providers)`.
>
> Just verifying the hover symbol respects the using decl is sufficient, something like
>
> ```
> #include "foo.h"
>
> namespace ns {
> using ::Foo;
> }
>
> ns::F^oo d; // verify that provider in hover is the main-file.
> ```
Ok, I will translate this to a "foobar" example. But I think the correct answer in this example is actually `foo.h` since we want the provider of the underlying decl, not the using decl.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D144976/new/
https://reviews.llvm.org/D144976
More information about the cfe-commits
mailing list