Interest in a -Wusing-namespace-in-headers?

Attached is a in-progress patch for a -Wusing-namespace-in-headers.
I'm not sure if this is something people would be interested in. If
there is, I'll continue working on this.

I do have two questions though:

1) How exactly should I be ordering the definitions in the .td files?
Is what I did fine?
2) The tests that I wrote as is don't work. The warning gets emitted,
but the test harness doesn't expect it despite the comment. I assume
this is because FindExpectedDiags() in DiagChecker.cpp explicitly only
looks in the main file for expected-* diagnostics. Is there any simple
way to fix this? Since the purpose of the warning is to trigger only
when not in the main file, I'll need to fix this somehow before I
submit this for real review...

Regards,

-- Elliot Glaysher

using-namespace.diff (3.15 KB)

Attached is a in-progress patch for a -Wusing-namespace-in-headers.
I'm not sure if this is something people would be interested in. If
there is, I'll continue working on this.

Seems like a reasonable opt-in warning. Are there other constructs that we should warn about in headers? If so, should they be grouped under the same warning flag?

I do have two questions though:

1) How exactly should I be ordering the definitions in the .td files?
Is what I did fine?

That's fine.

2) The tests that I wrote as is don't work. The warning gets emitted,
but the test harness doesn't expect it despite the comment. I assume
this is because FindExpectedDiags() in DiagChecker.cpp explicitly only
looks in the main file for expected-* diagnostics. Is there any simple
way to fix this? Since the purpose of the warning is to trigger only
when not in the main file, I'll need to fix this somehow before I
submit this for real review...

There's no good way around this, but there is a hack: FindExpectedDiags() only looks at the line number, so you can put the using directive on line 17 of the header, and put the corresponding expected-warning on line 17 of the .cpp file, and it'll work.

I'd prefer if the warning referred to these entities as "using directives" (both for the warning flag and in the text), since that's the term the standard uses.

  - Doug

IMO it should only warn in the global namespace. Importing into a
namespace or function is valid, and often essential. For example,
libstdc++ uses it to avoid ABI issues and boost uses it for functions
that should be in the 'std' namespace, but aren't on some platforms.

Daniel

Are there other constructs that we should warn about in headers? If so, should they be grouped under the same warning flag?

Off the top of my head, the only other warning I can think of is:

namespace {} // expect-warning {{code in anonymous namespaces in
headers may be compiled in multiple translation units}}

I'd prefer if the warning referred to these entities as "using directives" (both for the warning flag and in the text), since that's the term the standard uses.

I can do that. I included the phrase "using namespace" as I was
worried about users think this would ban UsingDecl, not just
UsingDirectiveDecl.

Regards,

-- Elliot Glaysher

IIRC, libstdc++ does it with "strong using" directives (which Clang doesn't support).

Boost, I assume, only does this for using declarations; that's not what the patch is about.

  - Doug

Are there other constructs that we should warn about in headers? If so, should they be grouped under the same warning flag?

Off the top of my head, the only other warning I can think of is:

namespace {} // expect-warning {{code in anonymous namespaces in
headers may be compiled in multiple translation units}}

Okay. I'm mainly concerned that there's a steady stream of such opt-in warnings, and that users would be better served by having them grouped under some more-general "-Wheader-hygiene" flag, rather than having to find all of the individual -W flags that they want to turn on.

I'd prefer if the warning referred to these entities as "using directives" (both for the warning flag and in the text), since that's the term the standard uses.

I can do that. I included the phrase "using namespace" as I was
worried about users think this would ban UsingDecl, not just
UsingDirectiveDecl.

Yeah. Actually, I think "using namespace directive" is fine in the warning text, but let's keep the warning option name precise (-Wusing-directive-in-system-header) or go ahead with the -Wheader-hygiene idea.

  - Doug

IIRC, libstdc++ does it with "strong using" directives (which Clang doesn't support).

Sorry, I meant libc++. It certainly uses using directives.

Boost, I assume, only does this for using declarations; that's not what the patch is about.

No, because we don't know if the function is in the std namespace, so
we can't use a using declaration. They're used for other reasons as
well.

But it doesn't really matter where it is or isn't used. What matters
is that a properly scoped import is usually safe, and warning about
one would be counter productive.

Daniel

IIRC, libstdc++ does it with "strong using" directives (which Clang doesn't support).

Sorry, I meant libc++. It certainly uses using directives.

I see using directives only at function scope in libc++ (and we certainly don't want to warn about those). But anyway, I agree with your meta-point.

Boost, I assume, only does this for using declarations; that's not what the patch is about.

No, because we don't know if the function is in the std namespace, so
we can't use a using declaration. They're used for other reasons as
well.

But it doesn't really matter where it is or isn't used. What matters
is that a properly scoped import is usually safe, and warning about
one would be counter productive.

I agree.

  - Doug

Attached is a new version, using Douglas's suggestion for a
-Wheader-hygiene flag, as I intend to add the anonymous namespace
warning next.

using-namespace-2.diff (3.63 KB)

Cool. One other comment:

Index: lib/Sema/SemaDeclCXX.cpp

Adds a -Wheader-hygiene warning for warnings that should only trigger
in #included files.

The first -Wheader-hygiene check is to make sure a using directive
isn't placed in the global context in a header.

using-namespace-3.diff (3.47 KB)

Committed in r127881, thanks!

  - Doug

Do we want to warn about this?

extern "C++" {
  using namespace foo;
}

Because the fact that extern blocks are decl contexts has bitten me before, and the above suggestion would fall prey to the same issue.

Sebastian

Adds a -Wheader-hygiene warning for warnings that should only trigger
in #included files.

The first -Wheader-hygiene check is to make sure a using directive
isn't placed in the global context in a header.

You don't need to perform the getDiagnosticLevel() check yourself, because the diagnostic system will handle warning suppression itself.

However, I do suggest performing the CurContext->getDeclKind() == Decl::TranslationUnit check before the isFromMainFile() check, since the former is cheaper.

Done.

Committed in r127881, thanks!

Do we want to warn about this?

extern "C++" {
using namespace foo;
}

Because the fact that extern blocks are decl contexts has bitten me before, and the above suggestion would fall prey to the same issue.

Good point! Yes, we do want to warn about that.

Is the attached patch sufficient, or are there other cases I should be
checking for?

-- Elliot

using-namespace-4.diff (1.68 KB)

-------- Original-Nachricht --------

Datum: Wed, 23 Mar 2011 12:22:11 -0700
Von: "Elliot Glaysher (Chromium)" <erg@chromium.org>
An: Douglas Gregor <dgregor@apple.com>
CC: Sebastian Redl <sebastian.redl@getdesigned.at>, "cfe-dev@cs.uiuc.edu" <cfe-dev@cs.uiuc.edu>, "cfe-commits@cs.uiuc.edu" <cfe-commits@cs.uiuc.edu>, Daniel James <dnljms@gmail.com>
Betreff: Re: [cfe-commits] [PATCH] Re: [cfe-dev] Interest in a -Wusing-namespace-in-headers?

>> Do we want to warn about this?
>>
>> extern "C++" {
>> using namespace foo;
>> }
>>
>> Because the fact that extern blocks are decl contexts has bitten me
before, and the above suggestion would fall prey to the same issue.
>
> Good point! Yes, we do want to warn about that.

Is the attached patch sufficient, or are there other cases I should be
checking for?

That's not the right way to do it, since now you would warn about this:

namespace ns1 {
  extern "C++" {
    using namespace ns2;
  }
}

What you have to do is call DeclContext::getEnclosingNamespaceContext() on the context that contains the using directive, and check whether that is a TranslationUnitDecl or a NamespaceDecl.

Sebastian

I don't think that's right either, as now you would warn on this:

void foo() {
  using namespace ns2;
}

I believe the correct thing to do is to iterate through
declaration context parents until you reach something that is not a
LinkageSpecDecl, and warn if it is a TranslationUnitDecl.

Thanks,

Is something like the attached closer to what you want? This new
version recursively walks up the DeclContext chain, and I've
integrated all the examples I've seen in this thread into the tests.

-- Elliot

using-namespace-5.diff (2.79 KB)

Committed in r128352, thanks!

  - Doug

Hi Elliot,

thanks for this feature! It looks like it warns about the following:

$ cat test.h
namespace foo {}
#define USE_NAMESPACE using namespace foo;
$ cat test.cc
#include "test.h"

USE_NAMESPACE
$ clang -c test.cc -Wheader-hygiene
test.cc:3:1: warning: using namespace directive in global context in
header [-Wheader-hygiene]
USE_NAMESPACE
^
In file included from test.cc:1:
./test.h:2:39: note: instantiated from:
#define USE_NAMESPACE using namespace foo;
                                      ^
1 warning generated.

That seems wrong. This pattern is used by ICU.

Nico