wip

src/analyze_includes/main.py

@@ -76,10 +76,16 @@ def main(args: Namespace) -> int:
         implementation_deps=args.implementation_deps,
     )
     all_includes_from_public = get_relevant_includes_from_files(
-        files=args.public_files, ignored_includes=ignored_includes, defines=system_under_inspection.defines
+        files=args.public_files,
+        ignored_includes=ignored_includes,
+        defines=system_under_inspection.defines,
+        include_paths=system_under_inspection.include_paths,
     )
     all_includes_from_private = get_relevant_includes_from_files(
-        files=args.private_files, ignored_includes=ignored_includes, defines=system_under_inspection.defines
+        files=args.private_files,
+        ignored_includes=ignored_includes,
+        defines=system_under_inspection.defines,
+        include_paths=system_under_inspection.include_paths,
     )
 
     result = evaluate_includes(

src/analyze_includes/parse_source.py

@@ -4,27 +4,33 @@
 from pathlib import Path
 from typing import List, Optional
 
-from pcpp import Preprocessor
+from pcpp.preprocessor import Action, OutputDirective, Preprocessor
 
 
 class SimpleParsingPreprocessor(Preprocessor):
     """
-    This preprocessor configuration is used to prune commented code and to resolve preprocessor statements for defines
-    which are injected through Bazel. We do not resolve include statements. Meaning each file is analyzed only for
-    itself.
+    This preprocessor configuration is used to prune commented code and to resolve preprocessor statements. The main
+    points for us is resolving branching statements (e.g. '#ifdef') to analyze the correct code parts.
     """
 
-    def on_file_open(self, _, __):
+    def on_include_not_found(self, is_malformed, is_system_include, curdir, includepath):
         """
-        Raising here prevents include statements being resolved
+        We ignore missing include statements. Mostly they are system includes from the standard headers which are not
+        available for the DWYU analysis. Those don't matter to the DWYU analysis. Even if we would fail to other headers
+        this is not dramatic. The worst that can happen is that we miss defines from included headers. Given until
+        recently were not able to parse defines from included headers at all, we accept this shortcoming for now.
         """
-        raise OSError("Do not open file")
+        raise OutputDirective(Action.IgnoreAndPassThrough)
 
-    def on_error(self, _, __, ___):
-        """
-        Since unresolved include statements cause errors we silence error reporting
-        """
-        pass
+
+def make_pre_processor() -> SimpleParsingPreprocessor:
+    """
+    We can't overwrite member values from the base class in the ctor of our derived class and thus set them after
+    construction.
+    """
+    processor = SimpleParsingPreprocessor()
+    processor.passthru_includes = re.compile(".*")
+    return processor
 
 
 @dataclass
@@ -65,19 +71,34 @@ def is_ignored(self, include: str) -> bool:
         return is_ignored_path or is_ignored_pattern
 
 
-def get_includes_from_file(file: Path, defines: List[str]) -> List[Include]:
+def get_includes_from_file(file: Path, defines: List[str], include_paths: List[str]) -> List[Include]:
     """
     Parse a C/C++ file and extract include statements which are neither commented nor disabled through a define.
     """
     with open(file, encoding="utf-8") as fin:
-        pre_processor = SimpleParsingPreprocessor()
+        pre_processor = make_pre_processor()
+
         for define in defines:
-            pre_processor.define(define)
-        pre_processor.parse(fin.read())
+            # TODO investigate why 'FOO="bar"' is bad, but 'FOO ="bar' not
+            # TODO investigate did this cause some analysis failing and we simply never catched it by tests?
+            # pre_processor.define(define.replace("=", " = "))
+            # TODO double check: It seems '=' is actually invalid inside C++ for defines and merely a syntax outside in the compiler CLI
+            pre_processor.define(define.replace("=", " "))
+        for path in include_paths:
+            pre_processor.add_path(path)
+
+        # print("###########")
+        # print(pre_processor.macros)
+        # print("###########")
 
+        pre_processor.parse(fin.read())
         output_sink = StringIO()
         pre_processor.write(output_sink)
 
+        # print("-------------------")
+        # print(output_sink.getvalue())
+        # print("-------------------")
+
         return [
             Include(file=file, include=include)
             for include in re.findall(r'^\s*#include\s*["<](.+)[">]', output_sink.getvalue(), re.MULTILINE)
@@ -94,11 +115,11 @@ def filter_includes(includes: List[Include], ignored_includes: IgnoredIncludes)
 
 
 def get_relevant_includes_from_files(
-    files: Optional[List[str]], ignored_includes: IgnoredIncludes, defines: List[str]
+    files: Optional[List[str]], ignored_includes: IgnoredIncludes, defines: List[str], include_paths: List[str]
 ) -> List[Include]:
     all_includes = []
     if files:
         for file in files:
-            includes = get_includes_from_file(file=Path(file), defines=defines)
+            includes = get_includes_from_file(file=Path(file), defines=defines, include_paths=include_paths)
             all_includes.extend(includes)
     return filter_includes(includes=all_includes, ignored_includes=ignored_includes)

src/analyze_includes/test/BUILD

@@ -28,7 +28,9 @@ py_test(
         "data/commented_includes/single_line_comments.h",
         "data/empty_header.h",
         "data/header_with_defines.h",
+        "data/some_defines.h",
         "data/some_header.h",
+        "data/use_defines.h",
     ],
     deps = ["//src/analyze_includes:lib"],
 )

src/analyze_includes/test/data/some_defines.h

@@ -0,0 +1,2 @@
+#define MY_DEFINE
+#define THE_ANSWER 42

src/analyze_includes/test/data/use_defines.h

@@ -0,0 +1,13 @@
+#include "src/analyze_includes/test/data/some_defines.h"
+
+#ifdef MY_DEFINE
+#include "expected/include_a.h"
+#else
+#include "bad/include_a.h"
+#endif
+
+#if THE_ANSWER > 40
+#include "expected/include_b.h"
+#else
+#include "bad/include_b.h"
+#endif

src/analyze_includes/test/parse_source_test.py

@@ -100,19 +100,19 @@ def test_filter_includes_for_patterns(self):
 class TestGetIncludesFromFile(unittest.TestCase):
     def test_empty_header(self):
         test_file = Path("src/analyze_includes/test/data/empty_header.h")
-        result = get_includes_from_file(test_file, defines=[])
+        result = get_includes_from_file(test_file, defines=[], include_paths=[])
 
         self.assertEqual(result, [])
 
     def test_single_include(self):
         test_file = Path("src/analyze_includes/test/data/another_header.h")
-        result = get_includes_from_file(test_file, defines=[])
+        result = get_includes_from_file(test_file, defines=[], include_paths=[])
 
         self.assertEqual(result, [Include(file=test_file, include="foo/bar.h")])
 
     def test_multiple_includes(self):
         test_file = Path("src/analyze_includes/test/data/some_header.h")
-        result = get_includes_from_file(test_file, defines=[])
+        result = get_includes_from_file(test_file, defines=[], include_paths=[])
 
         self.assertEqual(len(result), 4)
         self.assertTrue(Include(file=test_file, include="bar.h") in result)
@@ -121,15 +121,15 @@ def test_multiple_includes(self):
 
     def test_commented_includes_single_line_comments(self):
         test_file = Path("src/analyze_includes/test/data/commented_includes/single_line_comments.h")
-        result = get_includes_from_file(test_file, defines=[])
+        result = get_includes_from_file(test_file, defines=[], include_paths=[])
 
         self.assertEqual(len(result), 2)
         self.assertTrue(Include(file=test_file, include="active_a.h") in result)
         self.assertTrue(Include(file=test_file, include="active_b.h") in result)
 
     def test_commented_includes_block_comments(self):
         test_file = Path("src/analyze_includes/test/data/commented_includes/block_comments.h")
-        result = get_includes_from_file(test_file, defines=[])
+        result = get_includes_from_file(test_file, defines=[], include_paths=[])
 
         self.assertEqual(len(result), 8)
         self.assertTrue(Include(file=test_file, include="active_a.h") in result)
@@ -143,27 +143,37 @@ def test_commented_includes_block_comments(self):
 
     def test_commented_includes_mixed_style(self):
         test_file = Path("src/analyze_includes/test/data/commented_includes/mixed_style.h")
-        result = get_includes_from_file(test_file, defines=[])
+        result = get_includes_from_file(test_file, defines=[], include_paths=[])
 
         self.assertEqual(result, [Include(file=test_file, include="active.h")])
 
     def test_includes_selected_through_defines(self):
         test_file = Path("src/analyze_includes/test/data/header_with_defines.h")
-        result = get_includes_from_file(test_file, defines=["FOO", "BAZ 42"])
+        result = get_includes_from_file(test_file, defines=["FOO", "BAZ 42"], include_paths=[])
 
         self.assertEqual(len(result), 4)
         self.assertTrue(Include(file=test_file, include="has_internal.h") in result)
         self.assertTrue(Include(file=test_file, include="has_foo.h") in result)
         self.assertTrue(Include(file=test_file, include="no_bar.h") in result)
         self.assertTrue(Include(file=test_file, include="baz_greater_40.h") in result)
 
+    def test_includes_selected_through_defines_from_header(self):
+        test_file = Path("src/analyze_includes/test/data/use_defines.h")
+        result = get_includes_from_file(test_file, defines=[], include_paths=[])
+
+        self.assertEqual(len(result), 3)
+        self.assertTrue(Include(file=test_file, include="src/analyze_includes/test/data/some_defines.h") in result)
+        self.assertTrue(Include(file=test_file, include="expected/include_a.h") in result)
+        self.assertTrue(Include(file=test_file, include="expected/include_b.h") in result)
+
 
 class TestGetRelevantIncludesFromFiles(unittest.TestCase):
     def test_get_relevant_includes_from_files(self):
         result = get_relevant_includes_from_files(
             files=["src/analyze_includes/test/data/some_header.h", "src/analyze_includes/test/data/another_header.h"],
             ignored_includes=IgnoredIncludes(paths=["vector"], patterns=[]),
             defines=[],
+            include_paths=[],
         )
 
         self.assertEqual(len(result), 4)

src/aspect/dwyu.bzl

@@ -187,9 +187,11 @@ def dwyu_aspect_impl(target, ctx):
     if _do_ensure_private_deps(ctx):
         args.add("--implementation_deps_available")
 
+    all_hdrs = target[CcInfo].compilation_context.headers.to_list()
+    analysis_inputs = [ctx.file._config, processed_target] + processed_deps + processed_implementation_deps + private_files + all_hdrs
     ctx.actions.run(
         executable = ctx.executable._dwyu_binary,
-        inputs = [ctx.file._config, processed_target] + public_files + private_files + processed_deps + processed_implementation_deps,
+        inputs = analysis_inputs,
         outputs = [report_file],
         mnemonic = "CompareIncludesToDependencies",
         progress_message = "Analyze dependencies of {}".format(target.label),

test/aspect/defines/BUILD

@@ -1,7 +1,7 @@
 cc_library(
     name = "in_file_defines",
     hdrs = ["in_file_defines.h"],
-    deps = ["//test/aspect/defines/lib:a"],
+    deps = ["//test/aspect/defines/support:lib_a"],
 )
 
 cc_library(
@@ -10,14 +10,23 @@ cc_library(
     copts = ["-DSOME_COPT 42"],
     defines = ["SOME_DEFINE"],
     local_defines = ["LOCAL_DEFINE"],
-    deps = ["//test/aspect/defines/lib:a"],
+    deps = ["//test/aspect/defines/support:lib_a"],
 )
 
 cc_library(
     name = "transitive_defines_from_bazel_target",
     hdrs = ["transitive_defines_from_bazel_target.h"],
     deps = [
-        "//test/aspect/defines/lib:a",
+        "//test/aspect/defines/support:lib_a",
         "//test/aspect/defines/support:transitive_define",
     ],
 )
+
+cc_library(
+    name = "use_defines_from_dependency_header",
+    hdrs = ["use_defines_from_dependency_header.h"],
+    deps = [
+        "//test/aspect/defines/support:conditional_defines",
+        "//test/aspect/defines/support:lib_b",
+    ],
+)

test/aspect/defines/README.md

@@ -1,11 +1,2 @@
 Defines can influence which include statements are relevant.
-
-These tests concentrate on parsing single files based on defines:
-
-- specified in the parsed file itself
-- coming from the C/C++ toolchain
-- defined by the Bazel target attributes `defines`, `local_defines` or `cops`
-
-Defines can also be imported into a file via an included header which specifies a define.
-This use case is not yet supported.
-We might add it at a later stage.
+In these test we ensure our preprocessor behaves as expected and chooses the desired code parts for analysis.

test/aspect/defines/defines_from_bazel_target.h

@@ -1,17 +1,17 @@
 #ifdef SOME_DEFINE
-#include "test/aspect/defines/lib/a.h"
+#include "test/aspect/defines/support/a.h"
 #else
-#include "test/aspect/defines/lib/b.h"
+#include "test/aspect/defines/support/b.h"
 #endif
 
 #ifdef LOCAL_DEFINE
-#include "test/aspect/defines/lib/a.h"
+#include "test/aspect/defines/support/a.h"
 #else
-#include "test/aspect/defines/lib/b.h"
+#include "test/aspect/defines/support/b.h"
 #endif
 
 #if SOME_COPT > 40
-#include "test/aspect/defines/lib/a.h"
+#include "test/aspect/defines/support/a.h"
 #else
-#include "test/aspect/defines/lib/b.h"
+#include "test/aspect/defines/support/b.h"
 #endif

test/aspect/defines/in_file_defines.h

@@ -1,19 +1,19 @@
 #define USE_A
 
 #ifdef USE_A
-#include "test/aspect/defines/lib/a.h"
+#include "test/aspect/defines/support/a.h"
 #else
-#include "test/aspect/defines/lib/b.h"
+#include "test/aspect/defines/support/b.h"
 #endif
 
 #ifdef NON_EXISTING_DEFINE
-#include "test/aspect/defines/lib/b.h"
+#include "test/aspect/defines/support/b.h"
 #endif
 
 #define SOME_VALUE 42
 
 #if SOME_VALUE > 40
-#include "test/aspect/defines/lib/a.h"
+#include "test/aspect/defines/support/a.h"
 #else
-#include "test/aspect/defines/lib/b.h"
+#include "test/aspect/defines/support/b.h"
 #endif

test/aspect/defines/support/BUILD

@@ -1,7 +1,34 @@
+package(default_visibility = ["//test/aspect/defines:__pkg__"])
+
 cc_library(
     name = "transitive_define",
     copts = ["-DLOCAL_COPT"],  # should not influence other targets
     defines = ["TRANSITIVE_DEFINE"],
     local_defines = ["LOCAL_DEFINE"],  # should not influence other targets
-    visibility = ["//test/aspect/defines:__pkg__"],
+)
+
+cc_library(
+    name = "conditional_defines",
+    hdrs = ["conditional_defines.h"],
+    deps = [":some_defines"],
+)
+
+cc_library(
+    name = "some_defines",
+    hdrs = ["some_defines.h"],
+)
+
+cc_library(
+    name = "lib_a",
+    hdrs = ["a.h"],
+)
+
+cc_library(
+    name = "lib_b",
+    hdrs = ["b.h"],
+)
+
+cc_library(
+    name = "lib_c",
+    hdrs = ["c.h"],
 )

test/aspect/defines/support/a.h

@@ -0,0 +1 @@
+// some content

test/aspect/defines/support/c.h

@@ -0,0 +1 @@
+// some content