From 3511ccdedd8cd33b68fa9a7fa9b2b3048e88a7bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kim=20Gr=C3=A4sman?= Date: Sun, 4 Dec 2022 16:30:31 +0100 Subject: [PATCH] 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 --- iwyu_include_picker.cc | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/iwyu_include_picker.cc b/iwyu_include_picker.cc index 328050c..8ee61f8 100644 --- a/iwyu_include_picker.cc +++ b/iwyu_include_picker.cc @@ -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;