llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
29.11k stars 12.01k forks source link

Unexpected internal linkage when using an alias to an private class #36627

Open llvmbot opened 6 years ago

llvmbot commented 6 years ago
Bugzilla Link 37279
Version unspecified
OS All
Reporter LLVM Bugzilla Contributor
CC @dwblaikie,@DougGregor,@zygoloid

Extended Description

I am not sure if clang's behaviour is wrong, but the behaviour is unexpected. The basic issue is that I can have a public alias to a private type, but if the alias appears in a method declaration the method is given internal linkage.

Here's a trivial example (compile with clang++ -c):

namespace Foo {
    namespace {
         struct _X {
             int y;
        };
    }
    using X = _X;
}
namespace Foo {
    struct Bar {
        X womp();
    };
}
Foo::Bar *thing();
int main() {
    thing()->womp();
    return 0;
}

Bug 20296 seems to be similar, but that seems to be related to class protection rather than anonymous namespaces

Given that the X alias public it seems expected that the method should not receive internal linkage. That would be my /expectation/, if the actual specification is different, it would be great if the warning actually said why a method got internal linkage.

llvmbot commented 6 years ago

[...] are you saying that because _X is in an anonymous namespace it gets a unique type per definition?

Yes. Each translation unit including that header has a completely distinct and separate class _X, because _X is in an inline namespace. As a result, Foo::Bar can only be defined in a single translation unit (including it into two translation units would violate the One Definition Rule because Foo::Bar::womp would return different types in different translation units).

Maybe if the warning were

“Has internal linkage due to [parameter/return] type X”

I think this is a nice idea. Perhaps we could add some notes to explain why the type has no linkage (in the same way we explain other type properties, like abstractness, with notes):

test.cc:11:11: error: function 'Foo::Bar::womp' is used but not defined in this translation unit, and cannot be defined in any other translation unit because its type does not have linkage test.cc:3:10: note: return type 'Foo::::_X' declared in anonymous namespace

Yeah, that seems nice to me -- I spent an hour trying to work out why I was getting internal linkage :D

ec04fc15-fa35-46f2-80e1-5d271f2ef708 commented 6 years ago

[...] are you saying that because _X is in an anonymous namespace it gets a unique type per definition?

Yes. Each translation unit including that header has a completely distinct and separate class _X, because _X is in an inline namespace. As a result, Foo::Bar can only be defined in a single translation unit (including it into two translation units would violate the One Definition Rule because Foo::Bar::womp would return different types in different translation units).

Maybe if the warning were

“Has internal linkage due to [parameter/return] type X”

I think this is a nice idea. Perhaps we could add some notes to explain why the type has no linkage (in the same way we explain other type properties, like abstractness, with notes):

test.cc:11:11: error: function 'Foo::Bar::womp' is used but not defined in this translation unit, and cannot be defined in any other translation unit because its type does not have linkage
test.cc:3:10: note: return type 'Foo::<anonymous namespace>::_X' declared in anonymous namespace
llvmbot commented 6 years ago

How would you define 'womp' in another translation unit? It's not possible, because a using declaration is transparent and cannot be forward declared, only defined (in which case a definition (or declaration) of the type it aliases would be required and since the type is internal, it wouldn't be possible to declare it again in another translation unit (it would be a different type))

It's sort of "fruit of the poisonous/internal tree",

All of the constructors for _X are available as you’ve included the header that defines _X, so anyone encounter Foo has to have access to them, so womp can be declared, or are you saying that because _X is in an anonymous namespace it gets a unique type per definition? (Ignoring that logically the mangling names should produce the same symbol)

That said this sounds like it is correct behavior so I think the problem is that the warning does not provide enough information.

Maybe if the warning were

“Has internal linkage due to [parameter/return] type X”

?

dwblaikie commented 6 years ago

How would you define 'womp' in another translation unit? It's not possible, because a using declaration is transparent and cannot be forward declared, only defined (in which case a definition (or declaration) of the type it aliases would be required and since the type is internal, it wouldn't be possible to declare it again in another translation unit (it would be a different type))

It's sort of "fruit of the poisonous/internal tree",