[PATCH] D42796: [clangd] Skip inline namespace when collecting scopes for index symbols.

[Sam McCall via cfe-commits]

My intuition was that the USRs would be different, that linkage would
either be included or not included from the USR, but it wouldn't affect
whether the namespace is included. (Reasoning: USRs model language
concepts, not linker ones)

But we're both wrong. If I'm reading USRGeneration correctly, hitting a
linkage spec while walking the scope tree sets the "ignore result" flag
which signals the result is unusable (and short-circuits some paths that
finish computing it). This seems like it may cause problems for us :-)
I wonder why the tests didn't catch it, maybe I'm misreading.

On Fri, Feb 2, 2018 at 1:46 PM, Ilya Biryukov <ibiryukov at google.com> wrote:

> Exactly. We should make sure we *don't* treat them as the same symbol. But
> I would expect there USRs to be the same and that's what we use to
> deduplicate.
>
>
> On Fri, Feb 2, 2018 at 1:45 PM Sam McCall <sammccall at google.com> wrote:
>
>> Right. And multiple TUs that *are* linked together would be fine too.
>> But in that case I don't think we need to be clever about treating these
>> as the same symbol. Indexing them twice is fine.
>>
>> On Fri, Feb 2, 2018 at 1:42 PM, Ilya Biryukov <ibiryukov at google.com>
>> wrote:
>>
>>> In a single translation unit, yes. In multiple translation units that
>>> aren't linked together it's totally fine (may actually refer to different
>>> entities).
>>>
>>>
>>> On Fri, Feb 2, 2018 at 1:04 PM Sam McCall <sammccall at google.com> wrote:
>>>
>>>> Yeah this is just a bug in clang's pprinter. I'll fix it.
>>>>
>>>> If you give multiple C++ names to the same linker symbol using extern
>>>> C, I'm pretty sure you're in UB land.
>>>>
>>>> On Fri, Feb 2, 2018, 12:04 Ilya Biryukov via Phabricator <
>>>> reviews at reviews.llvm.org> wrote:
>>>>
>>>>> ilya-biryukov added inline comments.
>>>>>
>>>>>
>>>>> ================
>>>>> Comment at: clangd/index/SymbolCollector.cpp:73
>>>>> +       Context = Context->getParent()) {
>>>>> +    if (llvm::isa<TranslationUnitDecl>(Context) ||
>>>>> +        llvm::isa<LinkageSpecDecl>(Context))
>>>>> ----------------
>>>>> ioeric wrote:
>>>>> > sammccall wrote:
>>>>> > > I'm not sure this is always correct: at least clang accepts this
>>>>> code:
>>>>> > >
>>>>> > >   namespace X { extern "C++" { int y; }}
>>>>> > >
>>>>> > > and you'll emit "y" instead of "X::y".
>>>>> > >
>>>>> > > I think the check you want is
>>>>> > >
>>>>> > >   if (Context->isTransparentContext() ||
>>>>> Context->isInlineNamespace())
>>>>> > >     continue;
>>>>> > >
>>>>> > >  isTransparentContext will handle the Namespace and Enum cases as
>>>>> you do below, including the enum/enum class distinction.
>>>>> > >
>>>>> > > (The code you have below is otherwise correct, I think - but a
>>>>> reader needs to think about more separate cases in order to see that)
>>>>> > In `namespace X { extern "C++" { int y; }}`, we would still want `y`
>>>>> instead of `X::y` since C-style symbol doesn't have scope.
>>>>> `printQualifiedName` also does the same thing printing `y`; I've added a
>>>>> test case for `extern C`.
>>>>> >
>>>>> > I also realized we've been dropping C symbols in `shouldFilterDecl`
>>>>> and fixed it in the same patch.
>>>>> I think we want `X::y`, not `y`.
>>>>>
>>>>> Lookup still finds it inside the namespace and does not find it in the
>>>>> global scope. So for our purposes they are actually inside the namespace
>>>>> and have the qualified name of this namespace. Here's an example:
>>>>> ```
>>>>> namespace ns {
>>>>> extern "C" int foo();
>>>>> }
>>>>>
>>>>> void test() {
>>>>>   ns::foo(); // ok
>>>>>   foo(); // error
>>>>>   ::foo(); // error
>>>>> }
>>>>> ```
>>>>>
>>>>> Note, however, that the tricky bit there is probably merging of the
>>>>> symbols, as it means symbols with the same USR (they are the same for all
>>>>> `extern "c"` declarations with the same name, right?) can have different
>>>>> qualified names and we won't know which one to choose.
>>>>>
>>>>> ```
>>>>> namespace a {
>>>>>  extern "C" int foo();
>>>>> }
>>>>> namespace b {
>>>>>   extern "C" int foo(); // probably same USR, different qname. Also,
>>>>> possibly different types.
>>>>> }
>>>>> ```
>>>>>
>>>>>
>>>>> Repository:
>>>>>   rL LLVM
>>>>>
>>>>> https://reviews.llvm.org/D42796
>>>>>
>>>>>
>>>>>
>>>>>
>>>
>>> --
>>> Regards,
>>> Ilya Biryukov
>>>
>>
>>
>
> --
> Regards,
> Ilya Biryukov
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180202/ef69164a/attachment.html>