Extra detail namespaces are causing confusion

[Pennycook]

Describe the bug
Different headers make different assumptions about the meaning of detail, which can lead to issues when headers are moved or included in new places. There are many instances of this, but I'll give one concrete example below to illustrate the problem.

sub_group_mask.hpp defines sycl::detail::Builder:

llvm/sycl/include/sycl/ext/oneapi/sub_group_mask.hpp

Lines 17 to 21 in c5c7ac2

	namespace sycl {
	__SYCL_INLINE_VER_NAMESPACE(_V1) {
	namespace detail {
	class Builder;
	} // namespace detail

...and then refers to that definition as detail::Builder inside of a class defined in sycl::ext::oneapi:

llvm/sycl/include/sycl/ext/oneapi/sub_group_mask.hpp

Lines 23 to 33 in c5c7ac2

	namespace ext::oneapi {
	#if defined(__SYCL_DEVICE_ONLY__) && defined(__AMDGCN__) && \
	(__AMDGCN_WAVEFRONT_SIZE == 64)
	#define BITS_TYPE uint64_t
	#else
	#define BITS_TYPE uint32_t
	#endif
	struct sub_group_mask {
	friend class detail::Builder;

Everything in sycl::ext::oneapi is in sycl, so the reference to detail resolves to sycl::detail in this case when the header is compiled in isolation. However, if any header defines a detail namespace in sycl::ext or sycl::ext::oneapi, detail resolves to sycl::ext::detail or sycl::ext::oneapi::detail and Builder cannot be found.

There are several such nested detail namespaces around, e.g. in atomic_ref.hpp:

llvm/sycl/include/sycl/ext/oneapi/atomic_ref.hpp

Lines 24 to 30 in c5c7ac2

	namespace sycl {
	__SYCL_INLINE_VER_NAMESPACE(_V1) {
	namespace ext::oneapi {
	namespace detail {
	// Import from detail:: into ext::oneapi::detail:: to improve readability later
	using namespace ::sycl::detail;

My suggestion is to use sycl::detail:: everywhere, and remove all of these nested namespaces. Details are details.

To Reproduce
Include sub_group_mask.hpp after a definition of a nested detail namespace (e.g. sycl::ext::oneapi::detail).

Environment (please complete the following information):

• OS: Linux
• Target device and vendor: N/A
• DPC++ version: c5c7ac2
• Dependencies version: N/A

Additional context
@bader has suggested previously that it is desirable to be able to include separate headers for different SYCL functionality. I think cleaning this up is a precursor to that -- if we encourage developers to start including headers that aren't sycl.hpp, it's very likely they'll encounter issues like this. I encountered it while trying to add some new functionality, and @steffenlarsen has encountered this issue previously as well.

[aelovikov-intel]

My suggestion is to use sycl::detail:: everywhere, and remove all of these nested namespaces.

That makes those details' implementation much more verbose - we now have to use the full extension namespace qualifiers inside helpers.

[Pennycook]

That makes those details' implementation much more verbose - we now have to use the full extension namespace qualifiers inside helpers.

Sorry, I'm not sure I follow. Are you saying that we might have a situation where we have to write this:

namespace sycl::detail {
void helper(ext::oneapi::experimental::Foo bar);
}

instead of:

namespace sycl::ext::oneapi::experimental::detail {
void helper(Foo bar);
}

I hadn't considered that.

I agree there's a trade-off here, but I think we have to pay the verbosity cost somewhere. If we leave things as they are, every usage of something declared in a detail namespace should be fully qualified (to avoid unexpected compilation errors).

[aelovikov-intel]

Are you saying that we might have a situation where we have to write...

yes.

If we leave things as they are, every usage of something declared in a detail namespace should be fully qualified (to avoid unexpected compilation errors).

Or every inner detail namespace has to introduce outer detail namespaces into itself, like is done in the example you provided above:

llvm/sycl/include/sycl/ext/oneapi/atomic_ref.hpp

Lines 24 to 30 in c5c7ac2

	namespace sycl {
	__SYCL_INLINE_VER_NAMESPACE(_V1) {
	namespace ext::oneapi {
	namespace detail {
	// Import from detail:: into ext::oneapi::detail:: to improve readability later
	using namespace ::sycl::detail;

I think that's relatively minor thing to do.

[Pennycook]

Or every inner detail namespace has to introduce outer detail namespaces into itself, like is done in the example you provided above:

llvm/sycl/include/sycl/ext/oneapi/atomic_ref.hpp

Lines 24 to 30 in c5c7ac2

	namespace sycl {
	__SYCL_INLINE_VER_NAMESPACE(_V1) {
	namespace ext::oneapi {
	namespace detail {
	// Import from detail:: into ext::oneapi::detail:: to improve readability later
	using namespace ::sycl::detail;

I think that's relatively minor thing to do.

If you think that would be easier, that works for me. Do you think we could test for this, or would it just have to be something that folks look out for during code review?

[aelovikov-intel]

I think the latter, because it's dependent on the include order and, since our headers aren't "self-contained", we can only verify the one used in <sycl/sycl.hpp>.

[aelovikov-intel]

I think introducing using namespace ::sycl::detail; isn't going to break things (unlike changing the namespaces we are actually using), so we can probably just give it a try and see how it would go.

[aelovikov-intel]

I don't think we're going to address that specifically, but it's not unlikely it will resolve (if haven't yet) organically on its own using using namespace ::sycl::detail "idiom" above. We also have tests to verify our includes are "self-contained", so this shouldn't be as important/common as before (if exists at all).