Using declaration in namespace in header is mishandled

[vmiklos]

Hi,

Steps to reproduce:

1. Create a compat.h:

#pragma once

#if !__has_include(<optional>)

... use boost here...

#else

#include <optional>
namespace compat {
using std::optional;
}

#endif

2. Create a test.cc:

#include "compat.h"

int main() {
  compat::optional<int> o;
}

3. Run IWYU on this:

$ include-what-you-use -std=c++2a test.cc

Expected result: IWYU says there is nothing to do.

Actual result:

test.cc should add these lines:
#include <optional>  // for optional

test.cc should remove these lines:
- #include "compat.h"  // lines 1-1

The full include-list for test.cc:
#include <optional>  // for optional
---

But if I do what it says, I get:

test.cc:4:3: error: use of undeclared identifier 'compat'
  compat::optional<int> o;
  ^

Note that the problem is there even without the boost ifdef, I left it there from the original code to give you an idea why would anyone do using std::optional; in a namespace.

Version:

$ include-what-you-use --version
include-what-you-use 0.14 (git:d90fe9d) based on clang version 10.0.0 (git://github.com/llvm/llvm-project a3f896481329f64aac845e03cfda8f1154ce6079)

i.e. this is meant to be ~latest iwyu master vs llvm-project.git master.

As usual, if there are code pointers on what should be fixed here, I'm not against attempting to fix this myself, but I'm unsure where to start. :-)

Also, feel free to rename the issue, I'm just guessing what may be the root cause here.

Thanks,

Miklos

[kimgr]

Thanks for the report. I experimented a little bit with this and can't find anything obvious.

I tried with a non-template user-defined type instead of std::optional, and that appears to work. So I suspect it's some kind of template resugaring problem.

As a workaround, you could add // IWYU pragma: export in compat.h:

#include <optional>  // IWYU pragma: export
namespace compat {
using std::optional;
}

[vmiklos]

My immediate need for this is gone, LibreOffice now moved to using std::optional directly. Do you prefer to keep the issue open (since it's still a bug) or OK to close this, so you spend your time on issues which are useful to fix in practice?

[Bu11etmagnet]

Even though there's one less person affected, this still seems to be a real bug in iwyu.

[kimgr]

Yeah, I agree, let's leave it open. Thanks for coming back with status update, that makes it easier to prioritize.

[shivanshu3]

I'm able to repro this bug in the following simple test, and there are no templates involved:

foo.h:

namespace Foo
{
    extern int num;
}

bar.h:

namespace Bar
{
    using namespace Foo;
}

main.cpp:

#include "foo.h"
#include "bar.h"

int dummy()
{
    return Bar::num;
}

IWYU Output:

>include-what-you-use.exe main.cpp

main.cpp should add these lines:

main.cpp should remove these lines:
- #include "bar.h"  // lines 2-2

The full include-list for main.cpp:
#include "foo.h"  // for num
---

We have a lot of code which uses a similar pattern, so adding the IWYU export pragma in all those places is not really feasible.

I'm on the following commit on master from May 4, 2020:
a8cd91a62beae51379109f194d7a6f988fa4cc2a

I'll try to debug it, but also any help will be greatly appreciated :)

[shivanshu3]

Not sure if this is a different bug, I feel like it's a simpler repro of the same bug. IWYU outputs the same thing as my last comment:

foo.h:

namespace Foo
{
	extern int num;
}

bar.h:

using namespace Foo;

main.cpp:

#include "foo.h"
#include "bar.h"

int dummy()
{
	return num;
}

[kimgr]

@shivanshu3 Thanks for the nice reduced testcases. If you're interested in digging deeper, running with verbose logging is usually a good start -- -Xiwyu -v7 -- but I suppose you've tried that already.

[shivanshu3]

So looks like IWYU does support clang::UsingDecl (like using std::vector;), but clang::UsingDirectiveDecl is not supported (like using namespace std;).

Adding support for clang::UsingDecl was probably relatively easy because that class has a shadows() method which gives access to all the clang::NamedDecls that were referenced using it, via clang::UsingShadowDecl.

clang::UsingDirectiveDecl does not have information about which clang::NamedDecls were referenced using it. I was looking at Clang's code which performs lookups given an identifier name in the current namespace, all parent namespaces, and all the namespaces visible via clang::UsingDirectiveDecls at the location where lookup was requested. That code looks pretty complicated and we should probably not try to replicate that in IWYU. Instead, making a change to Clang to keep track of which clang::UsingDirectiveDecl was used to resolve an identifier during a lookup and storing that information in the corresponding clang::DeclRefExpr might be the best thing to do.

Then, we could use that information inside IwyuAstConsumer::VisitDeclRefExpr to add a OneUse for the clang::UsingDirectiveDecl.

Thoughts? How common is it to make a change to Clang to achieve something in IWYU? I've never done that and I'm not sure what that process looks like.

[kimgr]

Interesting!

I've run into a similar lookup problem before: #636. I tried to get my head around the lookup machinery in Clang when I looked at that, but I never managed completely.

I suspect templates might be a challenge. Say you have this:

foo.h:

namespace Foo
{
	extern int num;
}

bar.h:

using namespace Foo;

tee.h:

#include "foo.h"
#include "bar.h"

template<class T>
struct Dummy {
  int dummy() {
    return num;
  }
};

If that template is never instantiated, I don't believe there will be a lookup. But the IWYU analysis should still anchor both foo.h and bar.h with tee.h, as they're both implementation details of that file.

[kimgr]

Re making changes to Clang -- we have made a small number of patches over the years, but nothing substantial (actually, I suspect the RecursiveASTVisitor design was mostly born out of IWYU's needs, but that was before my time).

I'm not a regular contributor to Clang, so I've found it hard to break through the review wall with changes that don't improve the compiler itself, and that no other tooling (e.g. clangd) obviously needs. Off the top of my head, it feels like modifying the lookup process, and storing more data on every single DeclRefExpr might be met with some resistance.

But I never got my head around it, as I said above, so I might be overestimating how complex such a change would be. If a Clang-side patch seems straightforward to you, it might be worth vetting it on the clang-dev list to see what they say.

[shivanshu3]

So in your example with the template, bar.h is not technically required for the code to compile and be correct. Like you said, since the template is not instantiated, none of the variables inside the function need to resolve. You could even name the variable num2, and it would still compile.

A clang::DeclRefExpr only gets created when this template is instantiated, and that's where we would theoretically store the clang::UsingDirectiveDecl which was used to do the lookup.

[kimgr]

Yes, but the use must still be attributed to tee.h, so it needs to work even if Dummy is never instantiated.

[leinjay]

So is there a solution to this problem now?
For example：
In “foo.h”，I used using namespace std.
In file "bar.h",I declared #include "foo.h"and #include \<vector\>at first, then i defined vector\<int\> vec;directly
.According to the syntax of C++, it can be compiled completely.
but when I used "fix_include.py" to remove #include foo.h in "bar.h" as iwyu suggested ,it occurs an error : "vector is not declare"
Do you have a better way to solve this problem