Implement partial include support#305
Conversation
There was a problem hiding this comment.
Pull request overview
Adds partial #include preservation by converting include directives into emitted D imports (or // FIXME: import ...; placeholders) so downstream tooling can resolve dependencies later.
Changes:
- Parse
#includedirectives in the preprocessor layer and expose them viaIncludeDirective. - Feed parsed include directives into
IncludeHandler, and always emit placeholder// FIXME: import ...;lines for unhandled includes. - Update unit tests’ expected translated output to include the newly emitted imports.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
dstep/translator/Preprocessor.d |
Introduces IncludeDirective and parseInclude to recognize #include directives. |
dstep/translator/Context.d |
Collects parsed include directives and forwards them to IncludeHandler. |
dstep/translator/IncludeHandler.d |
Adjusts include ingestion and import emission; adds sys/ioctl mapping and emits // FIXME: import ...; for unknown includes. |
dstep/translator/Options.d |
Removes keepUntranslatable option (unhandled includes now always emitted). |
tests/unit/issues/Issue85.d |
Updates expected output to include core.stdc.stddef import. |
tests/unit/issues/Issue8.d |
Updates expected output to include core.stdc.stdint import. |
tests/unit/issues/Issue199.d |
Updates expected output to include core.stdc.stdint import. |
tests/unit/UnitTests.d |
Updates expected outputs to include stdc imports (incl. wchar_) when headers are present. |
tests/unit/MacroTranslTests.d |
Updates expected output to include core.stdc.stdint import. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import std.path : stripExtension, baseName, dirName; | ||
|
|
||
| include = stripExtension(baseName(include)); | ||
| auto baseInclude = stripExtension(baseName(include)); | ||
| auto includeWithFolder = dirName(include) ~ "/" ~ baseInclude; | ||
|
|
||
| if (auto ptr = include in knownIncludes) | ||
| if (auto ptr = baseInclude in knownIncludes) | ||
| return *ptr; | ||
| else if (auto ptr = includeWithFolder in knownIncludes) | ||
| return *ptr; |
There was a problem hiding this comment.
isKnownInclude does not normalize absolute paths before attempting the folder-based lookup. For example, an include discovered via clang may come through as /usr/include/sys/ioctl.h; dirName(include) then becomes /usr/include/sys, so includeWithFolder is /usr/include/sys/ioctl, which will never match keys like sys/ioctl in knownIncludes. Consider stripping common system include prefixes (same set as in toImports) or otherwise converting absolute include paths to a normalized sys/ioctl-style form before doing the baseInclude / includeWithFolder lookups.
| if (acceptPunctuation!("<")(tokens)) | ||
| { | ||
| directive.system = true; | ||
| directive.path = tokens.until!((token) => token.spelling == ">") | ||
| .fold!((str, token) => str ~= token.spelling)(""); | ||
| } else if (acceptPunctuation!("\"")(tokens)) | ||
| { | ||
| directive.system = false; | ||
| directive.path = tokens.until!((token) => token.spelling == "\"") | ||
| .fold!((str, token) => str ~= token.spelling)(""); | ||
| } |
There was a problem hiding this comment.
#include "..." is likely tokenized by libclang as a single literal token (e.g. "foo.h"), not separate " punctuation tokens. Using acceptPunctuation!("\"") + until(token.spelling == "\"") will then fail, leaving directive.path empty and silently dropping quoted includes. Consider using acceptStringLiteral (from MacroParser) to capture the header name and strip the surrounding quotes; optionally also handle the case where <...> comes through as a single literal token.
| Directive parseInclude(Token[] tokens) | ||
| { | ||
| import std.algorithm.searching; | ||
| import std.algorithm.iteration; | ||
|
|
||
| if (acceptDirective!"include"(tokens)) | ||
| { | ||
| auto directive = new IncludeDirective(); | ||
| directive.kind = DirectiveKind.include; | ||
| directive.tokens = tokens; | ||
| directive.extent = tokens.extent; | ||
|
|
||
| if (acceptPunctuation!("<")(tokens)) | ||
| { | ||
| directive.system = true; | ||
| directive.path = tokens.until!((token) => token.spelling == ">") | ||
| .fold!((str, token) => str ~= token.spelling)(""); | ||
| } else if (acceptPunctuation!("\"")(tokens)) | ||
| { | ||
| directive.system = false; | ||
| directive.path = tokens.until!((token) => token.spelling == "\"") | ||
| .fold!((str, token) => str ~= token.spelling)(""); | ||
| } | ||
| return directive; | ||
| } |
There was a problem hiding this comment.
parseInclude is newly implemented but there are no unit tests asserting that #include <...> / #include "..." produce an IncludeDirective with the expected system flag and path. Since this module already has extensive unittest coverage for other directives, adding include-specific tests would help prevent regressions in tokenization/parsing behavior.
|
Unfortunately there's no one-to-one mapping of header files and D modules, that's why handling of includes is incomplete. I'm not sure this is the right approach. I've been thinking about supporting generating bindings for a whole directory. Then it would be possible to look up where a declaration is located and add imports based on that instead of the includes. There's also no new test case added. |
Currently
#includeheaders are dropped and not converted to D import statements.This PR implements partial support so that include headers will be converted from:
to:
// FIXME: import b;This allows to later process and fix them (for example with
sed).This is important when headers contain complicated inter-dependencies and without this information it's difficult to know which other files would need to be imported.