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

[Sam McCall via cfe-commits]

Talked to Ben, he thinks this is probably unintentional and that it's
probably OK to change.
I'll see if it breaks anything.

On Fri, Feb 2, 2018 at 2:11 PM, Sam McCall <sammccall at google.com> wrote:

> I was misreading: we set isIgnored if we're trying to generate a USR for a
> linkagespecdecl itself (not a symbol in it).
> For other e.g. a var, we check if the DC is a NamedDecl and if so, visit
> it before visiting the var.
> Linkagespec isn't a nameddecl, so this is a no-op. Result: things
> (directly) under extern {} blocks don't get any outer scope info in their
> USR. But not sure if this is intended (it's certainly not what *we* want!)
>
> On Fri, Feb 2, 2018 at 2:05 PM, Ilya Biryukov <ibiryukov at google.com>
> wrote:
>
>> At least now we know they might cause problems. Thanks for digging into
>> this.
>>
>>
>> On Fri, Feb 2, 2018 at 1:53 PM Sam McCall <sammccall at google.com> wrote:
>>
>>> 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
>>>>
>>>
>>>
>>
>> --
>> Regards,
>> Ilya Biryukov
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180202/0721e1ff/attachment-0001.html>