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

Fri Feb 2 06:16:45 PST 2018

Fixed prettyprinter in r324081 and USRs in r324093.

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

> 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/llvm-commits/attachments/20180202/038245f8/attachment.html>