Serene
/
include-what-you-use
2
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Silently break cycles in mappings 

A long-standing problem with IWYU has been the preprocessor
metaprogramming techniques used in Boost.

IWYU is fine with cycles in the include graph, but did not accept cycles
in the corresponding mapping graph (a potential rationale might have
been to let people know if they explicitly added contradictory mappings,
but we really don't know what drove this decision).

Looking at the algorithm expanding the transitive closure of the mapping
graph, it expands all nodes in depth-first order. Cycle detection is
necessary to protect the algorithm from infinite recursion in the
presence of a cycle. But there should be no difference in the transitive
closure between a graph with cycles and one without. DFS will still
visit all nodes, it will just avoid visiting the same subtree twice.

Once the expansion is done, mapping lookup is an inherently
non-recursive operation (made cheaper by performing the recursive
expansion up-front). See the GetCandidateHeadersForFilepath family of
functions and their respective callers.

Two conclusions:

* Cycles are already detected by the transitive closure algorithm
* No code _after_ the transitive closure is affected by any cycles

Therefore, update the transitive closure expansion to ignore cycles
instead of failing on them. Add diagnostic logging at level 8 so we can
look into its decision-making if necessary.

Fixes issue #424
This commit is contained in:
Kim Gräsman 2022-12-04 16:30:31 +01:00
parent 278ba9de27
commit 3511ccdedd
1 changed files with 13 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

26
iwyu_include_picker.cc
View File

@ -1092,20 +1092,20 @@ void MakeNodeTransitive(IncludePicker::IncludeMap* filename_map,
const string& key) {
// If we've already calculated this node's transitive closure, we're done.
const TransitiveStatus status = (*seen_nodes)[key];
if (status == kCalculating) { // means there's a cycle in the mapping
// TODO: Reconsider cycle handling; the include_cycle test fails without
// this special-casing, but it seems we should handle this more generally.
if (key.find("internal/") != string::npos) {
VERRS(4) << "Ignoring a cyclical mapping involving " << key << "\n";
return;
}
}
if (status == kCalculating) {
VERRS(0) << "Cycle in include-mapping:\n";
if (status == kCalculating) { // means there's a cycle in the mapping
// Note that cycles in mappings are generally benign, the cycle detection
// here is only necessary to protect the recursive algorithm from infinite
// regress. We will still expand all reachable nodes in the graph to a
// plain sequence representing the transitive closure.
// The expanded mappings are only used for simple lookup, never followed
// recursively (which could have necessitated preserving cycles and handling
// them in that traversal too).
// Log cycles at a high verbosity level to aid debugging.
VERRS(8) << "Ignored cycle in include mappings: ";
for (const string& node : *node_stack)
VERRS(0) << " " << node << " ->\n";
VERRS(0) << " " << key << "\n";
CHECK_UNREACHABLE_("Cycle in include-mapping"); // cycle is a fatal error
VERRS(8) << node << " -> ";
VERRS(8) << key << "\n";
return;
}
if (status == kDone)
return;