Populate declaration signature comments, close coverage gaps, fix round-trip bug

CLAUDE.md

@@ -69,7 +69,12 @@ public entry point, optionally splicing `include <...>` files together first via
   (variables/functions/modules) per `Scope`, parent-chain lookup, last-write-wins (no
   shadowing diagnostics), matching the Python reference exactly. `Scope` owns its child scopes;
   `ASTNode::scope()` is a non-owning pointer into that tree, valid as long as the root `Scope`
-  from `buildScopes()` is alive.
+  from `buildScopes()` is alive. `buildScopes()`/`collectHoistedDeclarations()` each have a second
+  overload taking `const std::vector<ASTNode*>&` (raw, non-owning) instead of
+  `vector<unique_ptr<ASTNode>>` — added for a consumer (openscad_cpp_evaluator's `use <file>`
+  resolution) that needs to build one scope over declaration nodes drawn from more than one
+  independently-owned AST vector at once, which can't be expressed as a single owning vector. Both
+  overloads share one implementation internally; the existing owning-vector overload is unchanged.
 - **Comments** (`comments.cpp`, `inline_comment_attach.cpp`): comments are stripped by the
   lexer during the main parse, then re-extracted from the raw source text in a second pass and
   spliced back in — standalone comments/blank lines as top-level nodes, inline comments

include/openscad_cpp_parser/api.hpp

@@ -42,6 +42,11 @@ std::vector<std::unique_ptr<ASTNode>> parseAst(const std::string& code, const st
 // gets its scope() populated. Mirrors scope.py's build_scopes().
 std::unique_ptr<Scope> buildScopes(const std::vector<std::unique_ptr<ASTNode>>& ast);
 
+// Same, but for nodes referenced by raw pointer -- see
+// collectHoistedDeclarations's raw-pointer overload (scope_builder.hpp) for
+// why this exists. Never takes ownership.
+std::unique_ptr<Scope> buildScopes(const std::vector<ASTNode*>& ast);
+
 // Parses `code`. Throws ParseError (with the full caret diagnostic) on a
 // syntax error.
 //

include/openscad_cpp_parser/ast/scope_builder.hpp

@@ -16,4 +16,12 @@ class Scope;
 // level). Mirrors nodes.py's module-level `_collect_hoisted_declarations`.
 void collectHoistedDeclarations(const std::vector<std::unique_ptr<ASTNode>>& nodes, Scope& scope);
 
+// Same, but for nodes referenced by raw pointer rather than owned via
+// unique_ptr -- for building a scope over nodes that live in more than one
+// independently-owned AST vector at once (e.g. a `use <file>` statement's
+// evaluator-level resolution, which combines declarations pulled in from
+// another file's already-parsed-and-owned AST with the current file's own
+// nodes). Never takes ownership.
+void collectHoistedDeclarations(const std::vector<ASTNode*>& nodes, Scope& scope);
+
 } // namespace oscad

src/ast/scope_builder.cpp

@@ -6,16 +6,23 @@
 
 namespace oscad {
 
-void collectHoistedDeclarations(const std::vector<std::unique_ptr<ASTNode>>& nodes, Scope& scope) {
-    for (const auto& n : nodes) {
-        if (auto* a = dynamic_cast<Assignment*>(n.get())) {
+void collectHoistedDeclarations(const std::vector<ASTNode*>& nodes, Scope& scope) {
+    for (ASTNode* n : nodes) {
+        if (auto* a = dynamic_cast<Assignment*>(n)) {
             scope.defineVariable(a->name->name, a);
-        } else if (auto* f = dynamic_cast<FunctionDeclaration*>(n.get())) {
+        } else if (auto* f = dynamic_cast<FunctionDeclaration*>(n)) {
             scope.defineFunction(f->name->name, f);
-        } else if (auto* m = dynamic_cast<ModuleDeclaration*>(n.get())) {
+        } else if (auto* m = dynamic_cast<ModuleDeclaration*>(n)) {
             scope.defineModule(m->name->name, m);
         }
     }
 }
 
+void collectHoistedDeclarations(const std::vector<std::unique_ptr<ASTNode>>& nodes, Scope& scope) {
+    std::vector<ASTNode*> raw;
+    raw.reserve(nodes.size());
+    for (const auto& n : nodes) raw.push_back(n.get());
+    collectHoistedDeclarations(raw, scope);
+}
+
 } // namespace oscad

src/comment_attach_internal.hpp

@@ -3,6 +3,7 @@
 #include "openscad_cpp_parser/ast.hpp"
 
 #include <memory>
+#include <string>
 #include <vector>
 
 namespace oscad {
@@ -16,4 +17,22 @@ namespace oscad {
 void attachInlineComments(std::vector<std::unique_ptr<ASTNode>>& astNodes,
                            std::vector<std::unique_ptr<ASTNode>> inlineComments);
 
+// Claims `/* */` block comments (CommentSpan only -- these fields' own
+// declared type) that sit in the structural gaps a FunctionDeclaration or
+// ModuleDeclaration doesn't expose as any Expression slot: before the name,
+// between the name and the parameter list, between adjacent parameters
+// (including a parameter with no default value, which classifyNode's own
+// exprField walk never visits at all), and between the parameter list and
+// the body. Mutates `astNodes` (populating each declaration's own
+// preNameComments/postNameComments/postParamsComments and each
+// ParameterDeclaration's leadingComments/trailingComments) and nulls out
+// whatever it claims from `comments`, matching attachInlineComments' own
+// "claim by nulling the shared vector slot" convention. Must run BEFORE
+// attachInlineComments/the inline-vs-standalone split -- a `/* */` comment
+// alone on its own line before a parameter is classified *standalone* by
+// isInlineComment, which this pass still needs to claim, so it operates on
+// the full extracted comment list, not just the inline subset.
+void attachDeclarationComments(std::vector<std::unique_ptr<ASTNode>>& astNodes, const std::string& code,
+                                std::vector<std::unique_ptr<ASTNode>>& comments);
+
 } // namespace oscad

src/comments.cpp

@@ -184,9 +184,13 @@ std::vector<std::unique_ptr<ASTNode>> injectComments(std::vector<std::unique_ptr
 std::vector<std::unique_ptr<ASTNode>> attachComments(std::vector<std::unique_ptr<ASTNode>> astNodes, const std::string& code,
                                                        const std::string& origin) {
     auto comments = extractComments(code, origin);
+    attachDeclarationComments(astNodes, code, comments);
     std::vector<std::unique_ptr<ASTNode>> inlineComments;
     std::vector<std::unique_ptr<ASTNode>> standalone;
     for (auto& c : comments) {
+        if (!c) {
+            continue;
+        }
         if (isInlineComment(*c, code)) {
             inlineComments.push_back(std::move(c));
         } else {

src/inline_comment_attach.cpp

@@ -374,6 +374,96 @@ void classifyNode(ASTNode& node, std::vector<ExprSlot>& exprFields, std::vector<
 int startOffsetOf(const ASTNode& n) { return n.position().start_offset; }
 int endOffsetOf(const ASTNode& n) { return n.position().end_offset; }
 
+// Scans raw source from `from` for the next occurrence of `ch`, skipping
+// over any already-extracted comment span encountered along the way (so a
+// stray '(' or ')' inside a comment's own text is never mistaken for the
+// real token). Returns -1 if not found. The grammar doesn't capture a
+// location for the parameter list's own '(' / ')' tokens (only NAME and the
+// whole declaration's span are available -- see driver.cpp's
+// makeFunctionDeclaration/makeModuleDeclaration/makeParameterDeclaration),
+// so this is the only way to recover their real positions.
+int findCharSkippingComments(const std::string& code, int from, char ch,
+                              const std::vector<std::unique_ptr<ASTNode>>& comments) {
+    size_t i = static_cast<size_t>(from);
+    while (i < code.size()) {
+        bool skipped = false;
+        for (const auto& c : comments) {
+            if (c && startOffsetOf(*c) == static_cast<int>(i)) {
+                i = static_cast<size_t>(endOffsetOf(*c));
+                skipped = true;
+                break;
+            }
+        }
+        if (skipped) {
+            continue;
+        }
+        if (code[i] == ch) {
+            return static_cast<int>(i);
+        }
+        ++i;
+    }
+    return -1;
+}
+
+// Moves every still-live CommentSpan in `comments` whose start offset falls
+// in [lo, hi) into `out`, nulling its slot in `comments` (same "claim by
+// nulling" convention attachInlineComments/walkAttach already use).
+void claimCommentSpansInRange(int lo, int hi, std::vector<std::unique_ptr<CommentSpan>>& out,
+                               std::vector<std::unique_ptr<ASTNode>>& comments) {
+    if (lo >= hi) {
+        return;
+    }
+    for (auto& c : comments) {
+        if (!c || c->kind() != NodeKind::CommentSpan) {
+            continue;
+        }
+        int cs = startOffsetOf(*c);
+        if (cs >= lo && cs < hi) {
+            out.push_back(std::unique_ptr<CommentSpan>(static_cast<CommentSpan*>(c.release())));
+        }
+    }
+}
+
+// Claims comments in every structural gap of one FunctionDeclaration/
+// ModuleDeclaration signature: before the name, between the name and '(',
+// between adjacent parameters (or leading/trailing a lone parameter),
+// between the last parameter and ')', and between ')' and the body.
+//
+// ponytail: when there are zero parameters, the "between name and '('" and
+// "between '(' and ')'" gaps are both real but there's no parameter to own
+// the latter -- folded into postParamsComments rather than adding a
+// dedicated field nothing in either language's AST declares.
+void claimDeclSignatureComments(const ASTNode& decl, std::unique_ptr<Identifier>& name,
+                                 std::vector<std::unique_ptr<ParameterDeclaration>>& parameters, int bodyStart,
+                                 std::vector<std::unique_ptr<CommentSpan>>& preName,
+                                 std::vector<std::unique_ptr<CommentSpan>>& postName,
+                                 std::vector<std::unique_ptr<CommentSpan>>& postParams, const std::string& code,
+                                 std::vector<std::unique_ptr<ASTNode>>& comments) {
+    int nameStart = startOffsetOf(*name);
+    int nameEnd = endOffsetOf(*name);
+    claimCommentSpansInRange(startOffsetOf(decl), nameStart, preName, comments);
+
+    int openParen = findCharSkippingComments(code, nameEnd, '(', comments);
+    int parenOpenPos = (openParen >= 0) ? openParen : nameEnd;
+    claimCommentSpansInRange(nameEnd, parenOpenPos, postName, comments);
+    int cursor = (openParen >= 0) ? openParen + 1 : nameEnd;
+
+    for (auto& p : parameters) {
+        claimCommentSpansInRange(cursor, startOffsetOf(*p), p->leadingComments, comments);
+        cursor = endOffsetOf(*p);
+    }
+
+    int closeParen = findCharSkippingComments(code, cursor, ')', comments);
+    int parenClosePos = (closeParen >= 0) ? closeParen : cursor;
+    if (!parameters.empty()) {
+        claimCommentSpansInRange(cursor, parenClosePos, parameters.back()->trailingComments, comments);
+    } else {
+        claimCommentSpansInRange(cursor, parenClosePos, postParams, comments);
+    }
+    cursor = (closeParen >= 0) ? closeParen + 1 : cursor;
+    claimCommentSpansInRange(cursor, bodyStart, postParams, comments);
+}
+
 // Recursively attaches unused entries of `comments` (sorted by
 // start_offset; a used entry is left null) to expressions within `node`'s
 // span. Mirrors _walk_attach.
@@ -496,8 +586,45 @@ void attachTrailingToLastExpr(ASTNode& node, std::unique_ptr<ASTNode> comment) {
     slot.put(std::move(wrapped));
 }
 
+// Recurses through `node` looking for FunctionDeclaration/ModuleDeclaration
+// nodes (which can nest inside a module's own children), claiming their
+// signature-gap comments via claimDeclSignatureComments. Reuses
+// classifyNode purely as a generic "what are this node's children" walk,
+// exactly like walkAttach does.
+void walkAttachDeclComments(ASTNode& node, const std::string& code, std::vector<std::unique_ptr<ASTNode>>& comments) {
+    if (node.kind() == NodeKind::FunctionDeclaration) {
+        auto& n = static_cast<FunctionDeclaration&>(node);
+        claimDeclSignatureComments(node, n.name, n.parameters, startOffsetOf(*n.expr), n.preNameComments, n.postNameComments,
+                                    n.postParamsComments, code, comments);
+    } else if (node.kind() == NodeKind::ModuleDeclaration) {
+        auto& n = static_cast<ModuleDeclaration&>(node);
+        int bodyStart = n.children.empty() ? endOffsetOf(node) : startOffsetOf(*n.children.front());
+        claimDeclSignatureComments(node, n.name, n.parameters, bodyStart, n.preNameComments, n.postNameComments,
+                                    n.postParamsComments, code, comments);
+    }
+
+    std::vector<ExprSlot> exprFields;
+    std::vector<ASTNode*> nonExprChildren;
+    classifyNode(node, exprFields, nonExprChildren);
+    for (auto* child : nonExprChildren) {
+        walkAttachDeclComments(*child, code, comments);
+    }
+    for (auto& slot : exprFields) {
+        walkAttachDeclComments(*slot.current, code, comments);
+    }
+}
+
 } // namespace
 
+void attachDeclarationComments(std::vector<std::unique_ptr<ASTNode>>& astNodes, const std::string& code,
+                                std::vector<std::unique_ptr<ASTNode>>& comments) {
+    for (auto& node : astNodes) {
+        if (node) {
+            walkAttachDeclComments(*node, code, comments);
+        }
+    }
+}
+
 void attachInlineComments(std::vector<std::unique_ptr<ASTNode>>& astNodes,
                            std::vector<std::unique_ptr<ASTNode>> inlineComments) {
     if (inlineComments.empty()) {

src/pretty_print.cpp

@@ -250,7 +250,36 @@ std::string fmtMultilineArgsGeneric(const std::string& head, const std::vector<s
     std::vector<std::string> lines;
     for (size_t i = 0; i < formattedArgs.size(); ++i) {
         std::string terminator = (i + 1 < formattedArgs.size()) ? "," : "";
-        lines.push_back(innerPad + appendTerminatorSafely(formattedArgs[i], terminator));
+        const std::string& item = formattedArgs[i];
+        // A rendered arg beginning with a bare "//" line (from a leading
+        // CommentLine on this value -- see fmtCommentedExpr) can't stay on
+        // its own fresh line here: nothing would precede it there, so
+        // re-parsing would misclassify it as a *standalone* comment
+        // instead of staying attached to this argument, losing the
+        // association (a real round-trip bug this fixes). Move that first
+        // line onto the end of the previous argument's own line instead --
+        // mirrors how fmtListComprehension already renders the same
+        // situation for list elements. The remainder (if any) already
+        // carries fmtCommentedExpr's own innerPad-width indent, baked in
+        // when it joined that value onto its own continuation line, so it
+        // must NOT be indented again here.
+        if (startsWith(item, "//")) {
+            size_t nl = item.find('\n');
+            std::string commentLine = (nl == std::string::npos) ? item : item.substr(0, nl);
+            std::string rest = (nl == std::string::npos) ? "" : item.substr(nl + 1);
+            if (!lines.empty()) {
+                lines.back() += "  " + commentLine;
+            } else {
+                lines.push_back(innerPad + commentLine);
+            }
+            if (!rest.empty()) {
+                lines.push_back(appendTerminatorSafely(rest, terminator));
+            } else {
+                lines.back() = appendTerminatorSafely(lines.back(), terminator);
+            }
+            continue;
+        }
+        lines.push_back(innerPad + appendTerminatorSafely(item, terminator));
     }
     return head + "(\n" + join(lines, "\n") + "\n" + pad + ")";
 }

src/scope.cpp

@@ -13,4 +13,13 @@ std::unique_ptr<Scope> buildScopes(const std::vector<std::unique_ptr<ASTNode>>&
     return root;
 }
 
+std::unique_ptr<Scope> buildScopes(const std::vector<ASTNode*>& ast) {
+    auto root = std::make_unique<Scope>();
+    collectHoistedDeclarations(ast, *root);
+    for (ASTNode* node : ast) {
+        node->buildScope(*root);
+    }
+    return root;
+}
+
 } // namespace oscad