Pull request 1137 in include-what-you-use: add a flag to workaround cyclic include issues

[notifi...@include-what-you-use.org]

New pull request 1137 by yuvalif: add a flag to workaround cyclic include issues
https://github.com/include-what-you-use/include-what-you-use/pull/1137

this is an attemps to workaround this issue:
https://github.com/include-what-you-use/include-what-you-use/issues/424

Signed-off-by: yuval Lifshitz <ylif...@redhat.com>

[notifi...@include-what-you-use.org]

Comment #1 on pull request 1137 by danielh2942: add a flag to workaround cyclic include issues
https://github.com/include-what-you-use/include-what-you-use/pull/1137

Unit test?

[notifi...@include-what-you-use.org]

Comment #2 on pull request 1137 by yuvalif: add a flag to workaround cyclic include issues
https://github.com/include-what-you-use/include-what-you-use/pull/1137

> Unit test?

will add.

[notifi...@include-what-you-use.org]

Comment #3 on pull request 1137 by yuvalif: add a flag to workaround cyclic include issues
https://github.com/include-what-you-use/include-what-you-use/pull/1137

@danielh2942 added UTs in 2nd commit

[notifi...@include-what-you-use.org]

Comment #4 on pull request 1137 by bolshakov-a: add a flag to workaround cyclic include issues
https://github.com/include-what-you-use/include-what-you-use/pull/1137

I haven't read the full conversation in the related issue, but I think the need to turn off the internal assertion suggests that the assertion is wrong... In the case of real cycle, clang isn't able to build an AST, and IWYU analysis just don't start.
There is a deeper problem: in general, it is impossible to unambiguously identify "included units" just by their filenames, because the same header may declare different names and may even include different set of other headers depending on environment (defined macros).
At now, I'd prefer to remove the assertion rather than introduce a new flag.

[notifi...@include-what-you-use.org]

Comment #4 on pull request 1137 by bolshakov-a: add a flag to workaround cyclic include issues
https://github.com/include-what-you-use/include-what-you-use/pull/1137

I haven't read the full conversation in the related issue, but I think the need to turn off the internal assertion suggests that the assertion is wrong... In the case of real cycle, clang isn't able to build an AST, and IWYU analysis just doesn't start.

[notifi...@include-what-you-use.org]

Comment #4 on pull request 1137 by bolshakov-a: add a flag to workaround cyclic include issues
https://github.com/include-what-you-use/include-what-you-use/pull/1137

I haven't read the full conversation in the related issue, but I think the need to turn off the internal assertion suggests that the assertion is wrong... In the case of real cycle, clang isn't able to build an AST, and IWYU analysis just doesn't start.

There is a deeper problem: in general, it is impossible to unambiguously identify "included units" just by their filenames, because the same header may declare different names and may even include different sets of other headers depending on environment (defined macros).

[notifi...@include-what-you-use.org]

Comment #5 on pull request 1137 by yuvalif: add a flag to workaround cyclic include issues

https://github.com/include-what-you-use/include-what-you-use/pull/1137

> I haven't read the full conversation in the related issue, but I think the need to turn off the internal assertion suggests that the assertion is wrong... In the case of real cycle, clang isn't able to build an AST, and IWYU analysis just doesn't start.

in one of the tests, there is a real cycle, and IWYU detects it and asserts. so, it seems like it does start even if there is a real cycle

[notifi...@include-what-you-use.org]

Comment #6 on pull request 1137 by bolshakov-a: add a flag to workaround cyclic include issues
https://github.com/include-what-you-use/include-what-you-use/pull/1137

You haven't any real cycle because you have guarding macros `INCLUDE_CYCLE_*1_H`. On the second entrance in a header, preprocessor reveals that macro defined, and the header effectively declares and includes nothing in that case. Hence, the second entrance effectively introduces another (empty) "included unit" in my terminology. If you remove the macros, you'll get a real cycle and the clang error.
Moreover, as far as I understand, `CHECK_UNREACHABLE_` denotes an execution path that should never be really executed. To write a test that it really fires seems very strange...

[notifi...@include-what-you-use.org]

Comment #7 on pull request 1137 by kimgr: add a flag to workaround cyclic include issues
https://github.com/include-what-you-use/include-what-you-use/pull/1137

Hey all, thanks for patch and reviews. A couple of thoughts:

* The assertion checks cycles in _include mappings_, not cycles in _includes_. The former can be triggered by the latter, but possibly by other inputs
* I'm curious what the follow-on effects are from ignoring the cycle like this. An assertion in general can guard from either already-broken invariants, or something that is known to break assumed invariants in some future path. I'm not sure which case this is, but I suspect the latter.
* With an include graph like this: `A <-> B -> C -> D`, won't we risk missing the `B -> C -> D` branch because traversal is aborted when the `A <-> B` cycle is detected?
* If this is the right fix (and unfortunately I suspect it isn't), I agree that it shouldn't be hidden behind a flag

So could we discuss the motivation for the solution, and what downstream problems it might or might not cause? Thanks!

[notifi...@include-what-you-use.org]

Comment #8 on pull request 1137 by yuvalif: add a flag to workaround cyclic include issues
https://github.com/include-what-you-use/include-what-you-use/pull/1137

the original issue was submitted in 2017 and hasn't been addressed since.
there is a real need for running iwyu together with boost, where these false positives are aborting the tool.
therefore I suggested using a flag as a workaround, or stopgap until a better solution is implemented.

[notifi...@include-what-you-use.org]

Comment #9 on pull request 1137 by kimgr: add a flag to workaround cyclic include issues
https://github.com/include-what-you-use/include-what-you-use/pull/1137

I'd prefer to try and get to the bottom of this, instead of adding workarounds that might cause subtle problems down the line. There's some more context here if you want to keep digging: https://github.com/include-what-you-use/include-what-you-use/issues/424#issuecomment-1312820812.

[notifi...@include-what-you-use.org]

Comment #10 on pull request 1137 by petersteneteg: add a flag to workaround cyclic include issues
https://github.com/include-what-you-use/include-what-you-use/pull/1137

I think this should be merged now, as a workaround since no one has been able to fix this for several years, and it is a complete showstopper for many cases. boost, Qt, etc.

But getting to the bottom if it would of course be good. From looking at this code to work around this my self I was somewhat depressed by the amount of hacks and hard coded google specific workarounds like
https://github.com/include-what-you-use/include-what-you-use/blob/f9a6325326995e98b5b6f536092d4cd55937c860/iwyu_include_picker.cc#L1098 and the fact that included using "" and <> are treated completely differently, all without any mention in the documentation.
So I think any attempt on making this work should be encouraged. Even if it is introducing an other hack. This is still much less of an hack then the current one ot just ignore all "internal" headers.

[notifi...@include-what-you-use.org]

Comment #11 on pull request 1137 by kimgr: add a flag to workaround cyclic include issues
https://github.com/include-what-you-use/include-what-you-use/pull/1137

I can only be frank and say that I will not be taking any command-line flag patches to mask this bug.

Let's try and understand what the desired behavior is instead, and make it so.

[notifi...@include-what-you-use.org]

Comment #11 on pull request 1137 by yuvalif: add a flag to workaround cyclic include issues
https://github.com/include-what-you-use/include-what-you-use/pull/1137

closing, given the above comment

[notifi...@include-what-you-use.org]

Comment #13 on pull request 1137 by kimgr: add a flag to workaround cyclic include issues
https://github.com/include-what-you-use/include-what-you-use/pull/1137

@yuvalif @petersteneteg Could you guys see if https://github.com/include-what-you-use/include-what-you-use/pull/1150 does anything to improve the situation in your codebases? Thanks.

[notifi...@include-what-you-use.org]

Comment #14 on pull request 1137 by yuvalif: add a flag to workaround cyclic include issues
https://github.com/include-what-you-use/include-what-you-use/pull/1137

> @yuvalif @petersteneteg Could you guys see if #1150 does anything to improve the situation in your codebases? Thanks.

i'm using your PR:
```
$ include-what-you-use --version
include-what-you-use 0.20 (git:d8d49b3) based on clang version 16.0.0 (https://github.com/llvm/llvm-project.git 98fa95492f3bbd5befdeb36c88a3ac5ef2740b4e)
```

but i can still see the boost issues. once difference is that it continues to run in-spite of the issue:
```
Cycle in include-mapping:
<boost/preprocessor/iteration/detail/iter/limits/forward1_256.hpp> ->
<boost/preprocessor/iteration/detail/iter/forward1.hpp> ->
<boost/mpl/aux_/template_arity.hpp> ->
<boost/preprocessor/iteration/detail/iter/limits/forward1_256.hpp> ->
<boost/preprocessor/iteration/detail/iter/limits/forward1_256.hpp>
/home/ylifshit/projects/include-what-you-use/iwyu_include_picker.cc:1108: Assertion failed: Cycle in include-mapping
```
```
Cycle in include-mapping:
<boost/preprocessor/iteration/detail/iter/limits/forward1_256.hpp> ->
<boost/preprocessor/iteration/detail/iter/forward1.hpp> ->
<boost/mpl/apply_wrap.hpp> ->
<boost/mpl/apply_wrap.hpp>
/home/ylifshit/projects/include-what-you-use/iwyu_include_picker.cc:1108: Assertion failed: Cycle in include-mapping
```
```
Cycle in include-mapping:
<boost/preprocessor/iteration/detail/iter/limits/forward2_256.hpp> ->
<boost/preprocessor/iteration/detail/iter/forward2.hpp> ->
<boost/mpl/aux_/advance_forward.hpp> ->
<boost/mpl/aux_/advance_forward.hpp>
/home/ylifshit/projects/include-what-you-use/iwyu_include_picker.cc:1108: Assertion failed: Cycle in include-mapping
```

[notifi...@include-what-you-use.org]

Comment #15 on pull request 1137 by kimgr: add a flag to workaround cyclic include issues
https://github.com/include-what-you-use/include-what-you-use/pull/1137

Thanks!

> but i can still see the boost issues. once difference is that it continues to run in-spite of the issue:

This is probably something in your run-environment, `Assertion failed` indicates that IWYU calls `abort`, so it definitely terminates.

I didn't really expect the patch to fix it, so thanks for verifying. Could you turn up to verbose-level 8 (using `-Xiwyu -v8` switches) and report any `Adding dynamic mapping` logs before assertions? Might be interesting to see if Boost comes in some particular flow consistently.

[notifi...@include-what-you-use.org]

Comment #16 on pull request 1137 by petersteneteg: add a flag to workaround cyclic include issues
https://github.com/include-what-you-use/include-what-you-use/pull/1137

@kimgr Hej, it seems my qt issues has been resolved somehow, I done a bunch of updates since this September, newer clang, newer qt, so my qt problems has disappeared. And currently I'm not able to reproduce the issue, so I can't really say what changes fixed it unfortunately.

[notifi...@include-what-you-use.org]

Comment #17 on pull request 1137 by petersteneteg: add a flag to workaround cyclic include issues
https://github.com/include-what-you-use/include-what-you-use/pull/1137

And thanks for a great tool!!!

[notifi...@include-what-you-use.org]

Comment #18 on pull request 1137 by yuvalif: add a flag to workaround cyclic include issues
https://github.com/include-what-you-use/include-what-you-use/pull/1137

> I didn't really expect the patch to fix it, so thanks for verifying. Could you turn up to verbose-level 8 (using `-Xiwyu -v8` switches) and report any `Adding dynamic mapping` logs before assertions? Might be interesting to see if Boost comes in some particular flow consistently.

uploaded verbose log here: https://0x0.st/okY3.log

[notifi...@include-what-you-use.org]

Comment #19 on pull request 1137 by kimgr: add a flag to workaround cyclic include issues
https://github.com/include-what-you-use/include-what-you-use/pull/1137

@yuvalif Thanks! So it is a reverse macro dependency, just with a longer cycle. My simplistic patch only handles X<=>X cycles. But I think I've convinced myself that we can remove the cycle diagnostics here entirely: https://github.com/include-what-you-use/include-what-you-use/issues/424#issuecomment-1336190336 -- let me know what you think?

[notifi...@include-what-you-use.org]

Comment #20 on pull request 1137 by yuvalif: add a flag to workaround cyclic include issues
https://github.com/include-what-you-use/include-what-you-use/pull/1137

> @yuvalif Thanks! So it is a reverse macro dependency, just with a longer cycle. My simplistic patch only handles X<=>X cycles. But I think I've convinced myself that we can remove the cycle diagnostics here entirely: [#424 (comment)](https://github.com/include-what-you-use/include-what-you-use/issues/424#issuecomment-1336190336) -- let me know what you think?

makes perfect sense. thanks.