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

[Ilya Biryukov via llvm-commits]

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/e1e1c86c/attachment.html>