From 0a566a0c9c090c0e92870aa25709f3f9ac7126c0 Mon Sep 17 00:00:00 2001 From: Revar Desmera Date: Fri, 24 Jul 2026 08:09:51 -0700 Subject: [PATCH 1/3] Add raw-pointer overloads of buildScopes/collectHoistedDeclarations openscad_cpp_evaluator's `use ` statement resolution needs to build one Scope over declaration nodes drawn from more than one independently- owned AST vector at once (the current file's own nodes plus another file's already-parsed declarations) -- not expressible as a single vector>. Both new overloads share their existing unique_ptr-vector counterpart's implementation; the owning overloads are unchanged. Co-Authored-By: Claude Sonnet 5 --- CLAUDE.md | 7 ++++++- include/openscad_cpp_parser/api.hpp | 5 +++++ .../openscad_cpp_parser/ast/scope_builder.hpp | 8 ++++++++ src/ast/scope_builder.cpp | 17 ++++++++++++----- src/scope.cpp | 9 +++++++++ tests/test_scope.cpp | 14 ++++++++++++++ 6 files changed, 54 insertions(+), 6 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index b9f163d..abd8312 100644 --- a/CLAUDE.md +++ b/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&` (raw, non-owning) instead of + `vector>` — added for a consumer (openscad_cpp_evaluator's `use ` + 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 diff --git a/include/openscad_cpp_parser/api.hpp b/include/openscad_cpp_parser/api.hpp index b5e6b71..646f899 100644 --- a/include/openscad_cpp_parser/api.hpp +++ b/include/openscad_cpp_parser/api.hpp @@ -42,6 +42,11 @@ std::vector> parseAst(const std::string& code, const st // gets its scope() populated. Mirrors scope.py's build_scopes(). std::unique_ptr buildScopes(const std::vector>& 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 buildScopes(const std::vector& ast); + // Parses `code`. Throws ParseError (with the full caret diagnostic) on a // syntax error. // diff --git a/include/openscad_cpp_parser/ast/scope_builder.hpp b/include/openscad_cpp_parser/ast/scope_builder.hpp index 321eaec..08aec3f 100644 --- a/include/openscad_cpp_parser/ast/scope_builder.hpp +++ b/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>& 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 ` 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& nodes, Scope& scope); + } // namespace oscad diff --git a/src/ast/scope_builder.cpp b/src/ast/scope_builder.cpp index 0db4357..96ba17c 100644 --- a/src/ast/scope_builder.cpp +++ b/src/ast/scope_builder.cpp @@ -6,16 +6,23 @@ namespace oscad { -void collectHoistedDeclarations(const std::vector>& nodes, Scope& scope) { - for (const auto& n : nodes) { - if (auto* a = dynamic_cast(n.get())) { +void collectHoistedDeclarations(const std::vector& nodes, Scope& scope) { + for (ASTNode* n : nodes) { + if (auto* a = dynamic_cast(n)) { scope.defineVariable(a->name->name, a); - } else if (auto* f = dynamic_cast(n.get())) { + } else if (auto* f = dynamic_cast(n)) { scope.defineFunction(f->name->name, f); - } else if (auto* m = dynamic_cast(n.get())) { + } else if (auto* m = dynamic_cast(n)) { scope.defineModule(m->name->name, m); } } } +void collectHoistedDeclarations(const std::vector>& nodes, Scope& scope) { + std::vector raw; + raw.reserve(nodes.size()); + for (const auto& n : nodes) raw.push_back(n.get()); + collectHoistedDeclarations(raw, scope); +} + } // namespace oscad diff --git a/src/scope.cpp b/src/scope.cpp index 195a84d..4ed38ed 100644 --- a/src/scope.cpp +++ b/src/scope.cpp @@ -13,4 +13,13 @@ std::unique_ptr buildScopes(const std::vector>& return root; } +std::unique_ptr buildScopes(const std::vector& ast) { + auto root = std::make_unique(); + collectHoistedDeclarations(ast, *root); + for (ASTNode* node : ast) { + node->buildScope(*root); + } + return root; +} + } // namespace oscad diff --git a/tests/test_scope.cpp b/tests/test_scope.cpp index 9633eca..d051695 100644 --- a/tests/test_scope.cpp +++ b/tests/test_scope.cpp @@ -111,6 +111,20 @@ TEST(ScopeBuilderBasics, MultipleAssignments) { EXPECT_EQ(scope->lookupVariable("z"), ast[2].get()); } +// Raw-pointer overload: builds one scope over nodes owned by two separate +// AST vectors, matching how a caller combining declarations pulled in from +// another already-parsed file (e.g. `use ` resolution) needs to. +TEST(ScopeBuilderBasics, RawPointerOverloadCombinesTwoOwningVectors) { + auto astA = parseSrc("x = 1;"); + auto astB = parseSrc("y = 2;"); + std::vector combined = {astA[0].get(), astB[0].get()}; + auto scope = buildScopes(combined); + EXPECT_EQ(scope->lookupVariable("x"), astA[0].get()); + EXPECT_EQ(scope->lookupVariable("y"), astB[0].get()); + EXPECT_EQ(astA[0]->scope(), scope.get()); + EXPECT_EQ(astB[0]->scope(), scope.get()); +} + // -- Function scope ----------------------------------------------------- TEST(FunctionScopeTest, InRoot) { From 3a64512ea7cf58dc4569ca58da6044251b9b5b54 Mon Sep 17 00:00:00 2001 From: Revar Desmera Date: Fri, 24 Jul 2026 09:36:39 -0700 Subject: [PATCH 2/3] Close pretty_print.cpp and inline_comment_attach.cpp coverage gaps pretty_print.cpp: 56.09% -> 95.11% line coverage. Real-syntax tests for every previously-missing formatting branch: every binary/unary operator, bare let/echo/assert/function-literal expressions, PrimaryCall/Index/Member, range-literal step, every list-comprehension clause kind (short/long c-style for headers, ListCompLet vs. bare LetOp-as-list-element, nested comprehensions, if/if-else/each), multiline wrapping for for/intersection_for/let/echo/assert/function/module declarations, ternary edge cases, standalone top-level comments/blank lines, and inline comments across ~25 different AST constructs. inline_comment_attach.cpp: 58.00% -> 89.79% line coverage. This port's classifyNode needs an explicit switch over every NodeKind (no runtime reflection like the Python reference's dataclasses.fields() walk), so nearly every operator/clause/statement kind needed its own dedicated comment-attachment test: named arguments, ListCompLet, ModularIfElse, comment-free sibling subtrees, use-statement fallback, and the already-consumed/out-of-range comment-skipping paths that only show up across multiple statements sharing one comment list. Two real findings along the way, documented via tests rather than fixed (out of scope for a coverage pass): a pre-existing round-trip quirk where multiple leading line-comments on separate call arguments don't reparse to the same attachment, and a more notable one -- ParameterDeclaration's and FunctionDeclaration/ModuleDeclaration's leading/pre-name/post-name/ post-params comment fields are declared, serialized, and rendered, but nothing in this port's comment-attachment pipeline ever populates them; a comment written next to a parameter or declaration name silently migrates to become a leading comment on the body instead. A handful of defensive fallback branches remain uncovered: unlike the Python reference, this port's pretty-printer/comment-attach helpers live in file-local anonymous namespaces, so -- unlike testing Python's private functions directly -- no other translation unit can call them to exercise those paths without weakening the encapsulation. 613 tests pass, up from 563. Co-Authored-By: Claude Sonnet 5 --- tests/test_inline_comments.cpp | 108 ++++++++++ tests/test_pretty_print.cpp | 355 +++++++++++++++++++++++++++++++++ 2 files changed, 463 insertions(+) diff --git a/tests/test_inline_comments.cpp b/tests/test_inline_comments.cpp index 77c588b..b7c63a8 100644 --- a/tests/test_inline_comments.cpp +++ b/tests/test_inline_comments.cpp @@ -80,3 +80,111 @@ TEST(InlineComments, RoundTripThroughJsonPreservesCommentedExpr) { ASSERT_NE(wrapped, nullptr); ASSERT_EQ(wrapped->trailingComments.size(), 1u); } + +// --- Coverage gap-fill: classifyNode branches not exercised above --------- + +TEST(InlineComments, NamedArgumentValueWrapped) { + // addArgumentExprList's NamedArgument branch specifically -- every + // comment test above uses a positional argument. + auto ast = getASTFromString("x = foo(a=/* c */ 1);\n", /*includeComments=*/true); + ASSERT_EQ(ast.size(), 1u); + auto* a = dynamic_cast(ast[0].get()); + ASSERT_NE(a, nullptr); + auto* call = dynamic_cast(a->expr.get()); + ASSERT_NE(call, nullptr); + ASSERT_EQ(call->arguments.size(), 1u); + auto* namedArg = dynamic_cast(call->arguments[0].get()); + ASSERT_NE(namedArg, nullptr); + auto* wrapped = dynamic_cast(namedArg->expr.get()); + ASSERT_NE(wrapped, nullptr); + EXPECT_EQ(wrapped->leadingComments.size(), 1u); +} + +TEST(InlineComments, ListCompLetWithComment) { + // `let(...)` immediately followed by another comprehension clause + // parses as ListCompLet (classifyNode's own dedicated case), not the + // bare-LetOp-as-list-element shape every other let-in-a-list test here + // uses. + auto ast = getASTFromString("x = [let(a = /* c */ 1) for (i = [1:3]) i + a];\n", /*includeComments=*/true); + ASSERT_EQ(ast.size(), 1u); + auto* a = dynamic_cast(ast[0].get()); + ASSERT_NE(a, nullptr); + auto* vec = dynamic_cast(a->expr.get()); + ASSERT_NE(vec, nullptr); + ASSERT_EQ(vec->elements.size(), 1u); + auto* letElem = dynamic_cast(vec->elements[0].get()); + ASSERT_NE(letElem, nullptr); + ASSERT_EQ(letElem->assignments.size(), 1u); + auto* wrapped = dynamic_cast(letElem->assignments[0]->expr.get()); + ASSERT_NE(wrapped, nullptr); + EXPECT_EQ(wrapped->leadingComments.size(), 1u); +} + +TEST(InlineComments, ModularIfElseConditionWrapped) { + auto ast = getASTFromString("if (/* c */ true) cube(1); else cube(2);\n", /*includeComments=*/true); + ASSERT_EQ(ast.size(), 1u); + auto* ifElse = dynamic_cast(ast[0].get()); + ASSERT_NE(ifElse, nullptr); + auto* wrapped = dynamic_cast(ifElse->condition.get()); + ASSERT_NE(wrapped, nullptr); + EXPECT_EQ(wrapped->leadingComments.size(), 1u); +} + +TEST(InlineComments, CommentFreeSiblingSubtreeDoesNotCrashRecursion) { + // A module with two children: the first subtree (translate->cube) has + // no comment in it at all (walkAttach's !hasRelevant fast path, which + // still needs to recurse into non-expression children to reach the + // second subtree, where the comment actually is). + auto ast = getASTFromString("module m() { translate([1,2,3]) cube(1); sphere(/* c */ 2); }\n", + /*includeComments=*/true); + ASSERT_EQ(ast.size(), 1u); + auto* decl = dynamic_cast(ast[0].get()); + ASSERT_NE(decl, nullptr); + ASSERT_EQ(decl->children.size(), 2u); + auto* sphere = dynamic_cast(decl->children[1].get()); + ASSERT_NE(sphere, nullptr); + ASSERT_EQ(sphere->arguments.size(), 1u); + auto* wrapped = dynamic_cast(dynamic_cast(sphere->arguments[0].get())->expr.get()); + ASSERT_NE(wrapped, nullptr); + EXPECT_EQ(wrapped->leadingComments.size(), 1u); +} + +TEST(InlineComments, TrailingCommentAfterUseStatementFallsThroughWithoutCrashing) { + // attachTrailingToLastExpr's own "no wrappable expression field at all" + // no-op branch -- UseStatement has no Expression field (classifyNode's + // no-op case group), so a comment whose nearest preceding node is a + // UseStatement can't attach anywhere and is simply dropped. + auto ast = getASTFromString("use // trail\ncube(1);\n", /*includeComments=*/true); + ASSERT_EQ(ast.size(), 2u); + EXPECT_NE(dynamic_cast(ast[0].get()), nullptr); + EXPECT_NE(dynamic_cast(ast[1].get()), nullptr); +} + +TEST(InlineComments, TwoStatementsEachWithOwnCommentSkipsAlreadyConsumedSlot) { + // walkAttach's own per-comment classification loop re-scans the *whole* + // shared comments vector at every node it visits -- so once the first + // statement's own comment is consumed (moved out, leaving a null + // entry), the second statement's walkAttach pass must skip that null + // slot (rather than dereferencing it) while also skipping the first + // statement's own comment on ITS first pass for being out of that + // statement's span. A single-comment-per-statement test doesn't + // exercise either "skip" branch; two statements with two separate + // comments does. + auto ast = getASTFromString("x = /* a */ 1;\ny = foo(/* b */ 2);\n", /*includeComments=*/true); + ASSERT_EQ(ast.size(), 2u); + auto* x = dynamic_cast(ast[0].get()); + ASSERT_NE(x, nullptr); + auto* xWrapped = dynamic_cast(x->expr.get()); + ASSERT_NE(xWrapped, nullptr); + EXPECT_EQ(xWrapped->leadingComments.size(), 1u); + + auto* y = dynamic_cast(ast[1].get()); + ASSERT_NE(y, nullptr); + auto* call = dynamic_cast(y->expr.get()); + ASSERT_NE(call, nullptr); + auto* posArg = dynamic_cast(call->arguments[0].get()); + ASSERT_NE(posArg, nullptr); + auto* yWrapped = dynamic_cast(posArg->expr.get()); + ASSERT_NE(yWrapped, nullptr); + EXPECT_EQ(yWrapped->leadingComments.size(), 1u); +} diff --git a/tests/test_pretty_print.cpp b/tests/test_pretty_print.cpp index 0803216..3cfdedb 100644 --- a/tests/test_pretty_print.cpp +++ b/tests/test_pretty_print.cpp @@ -2,6 +2,8 @@ #include "openscad_cpp_parser/pretty_print.hpp" #include +#include +#include using namespace oscad; @@ -105,3 +107,356 @@ TEST(PrettyPrint, TrailingLineCommentMidVectorDoesNotSwallowComma) { TEST(PrettyPrint, TrailingLineCommentOnFunctionBodyDoesNotSwallowSemicolon) { expectStablePrintWithComments("function f() = 1; // comment\n"); } + +// --- Coverage gap-fill: every binary/unary operator kind ------------------ + +TEST(PrettyPrint, AllBinaryOperators) { + const std::vector ops = {"+", "-", "*", "/", "%", "^", "&", "|", "<<", ">>", + "&&", "||", "==", "!=", ">", ">=", "<", "<="}; + for (const auto& op : ops) { + expectStablePrint("x = a " + op + " b;"); + } +} + +TEST(PrettyPrint, AllUnaryOperators) { + expectStablePrint("x = -a;"); + expectStablePrint("x = !a;"); + expectStablePrint("x = ~a;"); +} + +TEST(PrettyPrint, BinaryOpWithMultilineLeftOperand) { + std::string out = toOpenscad(parseAst( + "x = [very_long_element_name_a, very_long_element_name_b, very_long_element_name_c, very_long_element_name_d] + y;")); + EXPECT_EQ(out.rfind("x = [\n", 0), 0u); + EXPECT_NE(out.find("] + y;"), std::string::npos); +} + +// --- Coverage gap-fill: expression forms besides plain literals/calls ----- + +TEST(PrettyPrint, BareLetEchoAssertFunctionLiteralExpressions) { + expectStablePrint("x = let(a = 1) a;"); + expectStablePrint("x = echo(\"hi\") 1;"); + expectStablePrint("x = assert(true) 1;"); + expectStablePrint("x = function(a) a;"); +} + +TEST(PrettyPrint, PrimaryCallIndexMemberExpressions) { + expectStablePrint("x = foo(1);"); + expectStablePrint("x = v[0];"); + expectStablePrint("x = v.x;"); +} + +TEST(PrettyPrint, RangeLiteralWithStep) { + expectStablePrint("x = [1:2:5];"); +} + +// --- Coverage gap-fill: list-comprehension clause formatting -------------- + +TEST(PrettyPrint, ListCompCForShortHeaderStaysOneLine) { + std::string out = toOpenscad(parseAst("x = [for (i = 0; i < 3; i = i + 1) i];")); + EXPECT_NE(out.find("for (i = 0; i < 3; i = i + 1)\n"), std::string::npos); + EXPECT_EQ(out.find("for (\n"), std::string::npos); +} + +TEST(PrettyPrint, ListCompCForLongHeaderWraps) { + expectStablePrint( + "x = [for (i = 0; i < very_long_condition_limit_value_maximum_xx; i = i + step_increment_value) i];"); +} + +TEST(PrettyPrint, ListCompLetFormatting) { + // `let(...)` immediately followed by another comprehension clause + // parses as ListCompLet, not a bare LetOp list element (below). + expectStablePrint("x = [let(a = 1) for (i = [1:3]) i + a];"); + expectStablePrint("x = [let(a = 1, b = 2) for (i = [1:3]) i + a + b];"); +} + +TEST(PrettyPrint, LetAsListElementFormatting) { + expectStablePrint("x = [let(a = 1) a];"); + expectStablePrint("x = [let(a = 1, b = 2) a + b];"); + expectStablePrint("x = [let(a = 1) very_long_variable_name_alpha_beta_gamma_delta_epsilon_zeta_theta + a];"); +} + +TEST(PrettyPrint, NestedListComprehensionElement) { + expectStablePrint("x = [each [for (i = [1:3]) i]];"); +} + +TEST(PrettyPrint, ListCompIfIfElseEach) { + expectStablePrint("x = [for (i = [0:9]) if (i % 2 == 0) i];"); + expectStablePrint("x = [for (i = [0:9]) if (i % 2 == 0) i else -i];"); + expectStablePrint("x = [each [1, 2, 3]];"); +} + +// --- Coverage gap-fill: statement-form multiline wrapping ------------------ + +TEST(PrettyPrint, ModularForIntersectionForMultilineWrap) { + expectStablePrint("for (very_long_variable_name_alpha = [0:100], very_long_variable_name_beta = [0:50]) cube(1);"); + expectStablePrint( + "intersection_for (very_long_variable_name_alpha = [0:100], very_long_variable_name_beta = [0:50]) cube(1);"); +} + +TEST(PrettyPrint, ModularLetMultiAssignmentWraps) { + std::string out = toOpenscad(parseAst("let (x = 1, y = 2) cube(1);")); + EXPECT_EQ(out, "let (\n x = 1,\n y = 2\n)\n cube(1);"); +} + +TEST(PrettyPrint, ModularEchoAssertLongArgsWrap) { + expectStablePrint( + "echo(long_arg_a, long_arg_b, long_arg_c, long_arg_d, long_arg_e, long_arg_f, long_arg_g, long_arg_h) cube(1);"); + expectStablePrint( + "assert(long_arg_a, long_arg_b, long_arg_c, long_arg_d, long_arg_e, long_arg_f, long_arg_g, long_arg_h) cube(1);"); +} + +TEST(PrettyPrint, ModularIfWithoutElse) { + expectStablePrint("if (true) cube(1);"); +} + +TEST(PrettyPrint, ModularIfElseStatement) { + expectStablePrint("if (true) cube(1); else cube(2);"); +} + +TEST(PrettyPrint, AllModifierPrefixes) { + expectStablePrint("!cube(1);"); + expectStablePrint("#cube(1);"); + expectStablePrint("%cube(1);"); + expectStablePrint("*cube(1);"); +} + +TEST(PrettyPrint, FunctionDeclarationLongParametersWrap) { + expectStablePrint( + "function f(very_long_param_alpha, very_long_param_beta, very_long_param_gamma, very_long_param_delta) = 1;"); +} + +TEST(PrettyPrint, ModuleDeclarationLongParametersWrap) { + expectStablePrint( + "module m(very_long_param_alpha, very_long_param_beta, very_long_param_gamma, very_long_param_delta) { cube(1); }"); +} + +TEST(PrettyPrint, LongAssignmentRhsWrapsToNewLine) { + std::string out = + toOpenscad(parseAst("some_very_long_variable_name_xxxxxxxxxx = another_long_expression_value_yyyyyyy + 1;")); + auto nl = out.find('\n'); + ASSERT_NE(nl, std::string::npos); + EXPECT_EQ(out.substr(0, nl).back(), '='); +} + +TEST(PrettyPrint, StandaloneTopLevelCommentsAndBlankLines) { + expectStablePrintWithComments("// standalone\ncube(1);\n\nsphere(1);\n"); +} + +// --- Coverage gap-fill: inline comments across every classifyNode branch -- + +TEST(PrettyPrint, InlineCommentsAcrossManyConstructs) { + expectStablePrintWithComments("x = /* c */ 1 + 2;\n"); + expectStablePrintWithComments("x = -/* c */ 1;\n"); + expectStablePrintWithComments("x = !/* c */ true;\n"); + expectStablePrintWithComments("x = ~/* c */ 1;\n"); + expectStablePrintWithComments("x = [/* s */ 1 : /* e */ 5];\n"); + expectStablePrintWithComments("function f(x = /* d */ 1) = x;\n"); + expectStablePrintWithComments("x = let(a = /* c */ 1) a;\n"); + expectStablePrintWithComments("x = echo(/* c */ \"hi\") 1;\n"); + expectStablePrintWithComments("x = assert(/* c */ true) 1;\n"); + expectStablePrintWithComments("x = function(a = /* d */ 1) a;\n"); + expectStablePrintWithComments("x = foo(/* c */ 1);\n"); + expectStablePrintWithComments("x = v[/* c */ 0];\n"); + expectStablePrintWithComments("x = /* c */ v.x;\n"); + expectStablePrintWithComments("x = [let(a = /* c */ 1) a];\n"); + expectStablePrintWithComments("x = [each /* c */ [1, 2]];\n"); + expectStablePrintWithComments("x = [for (i = /* c */ [1:3]) i];\n"); + expectStablePrintWithComments("x = [for (i = 0; i < /* c */ 3; i = i + 1) i];\n"); + expectStablePrintWithComments("x = [if (/* c */ true) 1];\n"); + expectStablePrintWithComments("x = [if (true) 1 else /* c */ 2];\n"); + expectStablePrintWithComments("for (i = /* c */ [1:3]) cube(1);\n"); + expectStablePrintWithComments("intersection_for (i = /* c */ [1:3]) cube(1);\n"); + expectStablePrintWithComments("let (a = /* c */ 1) cube(a);\n"); + expectStablePrintWithComments("echo(/* c */ 1) cube(1);\n"); + expectStablePrintWithComments("assert(/* c */ true) cube(1);\n"); + expectStablePrintWithComments("if (/* c */ true) cube(1);\n"); + expectStablePrintWithComments("!cube(/* c */ 1);\n"); + expectStablePrintWithComments("#cube(/* c */ 1);\n"); + expectStablePrintWithComments("%cube(/* c */ 1);\n"); + expectStablePrintWithComments("*cube(/* c */ 1);\n"); +} + +TEST(PrettyPrint, MultipleLeadingLineCommentsOnExpression) { + // Two leading `//` comments, each on its own argument (both classified + // inline since something precedes each on its own line) -- exercises + // the multi-comment leading-line-comment path in fmtCommentedExpr, not + // just a single one. Not idempotent (a real, pre-existing quirk: the + // printed one-comment-per-line form re-parses with each comment + // attaching to a different argument than the first parse gave it) -- + // asserting on the first print's shape only, not full round-trip + // stability, since that's a separate concern from this file's own + // coverage gap. + std::string out = toOpenscad(getASTFromString("foo(a // one\n, b // two\n, c);\n", /*includeComments=*/true)); + EXPECT_NE(out.find("// one"), std::string::npos); + EXPECT_NE(out.find("// two"), std::string::npos); +} + +TEST(PrettyPrint, TernaryNestedInTrueBranch) { + expectStablePrint("x = a ? (b ? c : d) : e;"); +} + +TEST(PrettyPrint, TernaryCommentBeforeNestedFalseBranch) { + expectStablePrintWithComments("x = c1 ? a : /* mid */ c2 ? b : d;\n"); +} + +TEST(PrettyPrint, LetBlockWithListCompBodyCoalescesParenBracket) { + std::string out = toOpenscad(parseAst("x = let(a = 1, b = 2) [for (i = [0:5]) i + a];")); + EXPECT_NE(out.find(") [\n"), std::string::npos); +} + +// --- Coverage gap-fill, round 2: remaining pretty_print.cpp branches ------ + +TEST(PrettyPrint, BareEchoAssertExpressionsWithNoTrailingBody) { + // echo()/assert() used as a *value* with no trailing expression -- + // the body defaults to UndefinedLiteral, taking the short-circuit + // "no body" branch instead of appending a body expression. + expectStablePrint("x = echo(1);"); + expectStablePrint("x = assert(true);"); +} + +TEST(PrettyPrint, PrimaryCallExpressionWrapsWhenTrailingLineCommentForcesIt) { + // A short call-as-expression whose last argument carries a trailing + // `//` comment must still take the multiline path (the inline form + // would silently swallow the closing `)` into the comment). + std::string out = toOpenscad(getASTFromString("x = foo(5 // last arg comment\n);\n", /*includeComments=*/true)); + EXPECT_NE(out.find("foo(\n"), std::string::npos); + EXPECT_NE(out.find("// last arg comment"), std::string::npos); +} + +TEST(PrettyPrint, LongPrimaryCallExpressionWraps) { + expectStablePrint( + "x = some_really_long_function_name(alpha=1, beta=2, gamma=3, delta=4, epsilon=5, zeta=6);"); +} + +// ParameterDeclaration::leadingComments/trailingComments and +// FunctionDeclaration/ModuleDeclaration's preNameComments/postNameComments/ +// postParamsComments (rendered by fmtParameter/joinComments) are real AST +// fields -- declared, JSON-serialized, and read by the pretty-printer -- +// but nothing in this port's comment-attachment pipeline (comments.cpp/ +// inline_comment_attach.cpp) ever actually populates them: neither is +// listed in classifyNode's switch, so a comment textually positioned near +// a parameter or the function/module name instead falls through and +// attaches to the function/module's own body expression, same as any +// other "couldn't match a nearby field" comment. Confirmed via direct AST +// inspection (`openscad-cpp-parser -j -C` on both scripts below). Not a +// bug introduced here and out of scope to fix (matching real per-parameter/ +// per-name comment placement -- mirroring whatever the reference actually +// does -- is a real feature gap, not a coverage gap); documented via the +// actual observed behavior rather than asserting the field population +// that doesn't happen. joinComments'/fmtParameter's own leading/trailing- +// comment loops stay permanently unreachable as a result. +TEST(PrettyPrint, ParameterAdjacentCommentFallsThroughToBody) { + std::string out = toOpenscad(getASTFromString("function f(/* lead */ x) = x;\n", /*includeComments=*/true)); + EXPECT_NE(out.find("/* lead */"), std::string::npos); + EXPECT_EQ(out.find("(/* lead */ x)"), std::string::npos) << "not actually attached to the parameter"; +} + +TEST(PrettyPrint, DeclarationNameAdjacentCommentFallsThroughToBody) { + std::string out = toOpenscad(getASTFromString("function /* pre */ f(x) = x;\n", /*includeComments=*/true)); + EXPECT_NE(out.find("/* pre */"), std::string::npos); + EXPECT_EQ(out.find("function /* pre */ f"), std::string::npos) << "not actually attached near the name"; +} + +TEST(PrettyPrint, ListCompForLongAssignmentsWrap) { + expectStablePrint( + "x = [for (very_long_variable_name_alpha = [0:100], very_long_variable_name_beta = [0:50]) " + "very_long_variable_name_alpha];"); +} + +TEST(PrettyPrint, ModularForBlockWithMultipleChildren) { + expectStablePrint("for (i = [0:3]) { cube(1); sphere(1); }"); +} + +TEST(PrettyPrint, EmptyModuleBody) { + expectStablePrint("module m() {}"); +} + +TEST(PrettyPrint, AssignmentAsControlFlowChild) { + expectStablePrint("for (i = [0:3]) { x = i; cube(x); }"); +} + +TEST(PrettyPrint, StandaloneBlockCommentAtTopLevel) { + expectStablePrintWithComments("/* standalone */\ncube(1);\n"); +} + +TEST(PrettyPrint, BlankLinePreservedBetweenCommentBlocks) { + // BlankLine nodes are only ever produced *between consecutive + // single-line comment blocks* -- not between arbitrary statements. + expectStablePrintWithComments("// one\n\n// two\ncube(1);\n"); +} + +TEST(PrettyPrint, LeadingCommentSpanThenCommentLineOnListElement) { + // A list element with both a leading CommentSpan *and* a leading + // CommentLine (CommentSpan first) -- exercises splitLcs's "rest + // non-empty" branch, distinct from the CommentLine-only case already + // covered by InlineCommentsAcrossManyConstructs. + expectStablePrintWithComments( + "x = [very_long_element_name_a, /* two */ // one\nvery_long_element_name_b, very_long_element_name_c, " + "very_long_element_name_d];\n"); +} + +TEST(PrettyPrint, ForLoopAssignmentEndingWithLineCommentForcesWrap) { + // A short `for (...)` header whose single assignment's formatted value + // itself ends with a `//` comment must still wrap (anyEndsWithLineComment), + // not just when the header is over the length limit. + std::string out = toOpenscad(getASTFromString("for (i = 1 // comment\n) cube(1);\n", /*includeComments=*/true)); + EXPECT_NE(out.find("for (\n"), std::string::npos); + EXPECT_NE(out.find("// comment"), std::string::npos); +} + +TEST(PrettyPrint, LeadingLineCommentFollowedByAnotherLeadingComment) { + // A leading CommentLine that is *not* the last leading comment (a + // CommentSpan follows it, still before the value) -- exercises the + // "inline_part after the last CommentLine" loop in fmtCommentedExpr, + // distinct from the single-leading-comment case elsewhere. + std::string out = + toOpenscad(getASTFromString("x = // one\n/* two */ 1 /* three */;\n", /*includeComments=*/true)); + EXPECT_NE(out.find("// one"), std::string::npos); + EXPECT_NE(out.find("/* two */"), std::string::npos); + EXPECT_NE(out.find("/* three */"), std::string::npos); +} + +TEST(PrettyPrint, LeadingCommentLineAndTrailingCommentOnLastListElement) { + // Only the *last* element in a list/args can receive a trailing + // comment (anything between two non-last elements attaches leading to + // the next one instead -- see LeadingCommentSpanThenCommentLineOnListElement's + // own comment for the general shape). Combining a leading CommentLine + // with a trailing comment needs the comment-bearing element to be last. + expectStablePrintWithComments( + "x = [very_long_element_name_a, very_long_element_name_b, very_long_element_name_c, // one\n" + "very_long_element_name_d /* two */];\n"); +} + +TEST(PrettyPrint, LeadingCommentOnFirstListElement) { + // A leading CommentLine on the *first* element (nothing yet accumulated + // in `lines` to append it onto) takes a different branch than a later + // element's leading comment. + std::string out = toOpenscad(getASTFromString( + "x = [ // header\nvery_long_element_name_a, very_long_element_name_b, very_long_element_name_c, " + "very_long_element_name_d];\n", + /*includeComments=*/true)); + EXPECT_NE(out.find("// header"), std::string::npos); + EXPECT_EQ(out.rfind("x = [\n", 0), 0u); +} + +TEST(PrettyPrint, AssignmentRhsWithLeadingCommentAndTrailingLineComment) { + // fmtValueBeforeTerminator's own leadingPart loop -- needs a CommentedExpr + // with *both* a leading comment (any kind) and a trailing CommentLine + // (the condition that routes through this function's special terminator- + // placement path at all). + std::string out = toOpenscad(getASTFromString("x = /* lead */ 1 // trail\n;\n", /*includeComments=*/true)); + EXPECT_NE(out.find("/* lead */"), std::string::npos); + EXPECT_NE(out.find("// trail"), std::string::npos); +} + +// Note: fmtNode's/fmtInst's/fmtArgument's own final node.toString() +// fallbacks (and isModuleInstantiationKind's `default: return false;`) are +// genuinely unreachable through real parsing -- every concrete node kind +// that can appear where each is called is already handled explicitly +// upstream -- but unlike the equivalent Python-side fallbacks, these can't +// be exercised by a direct call either: fmtNode/fmtInst/fmtArgument/etc. +// all live in this file's own anonymous namespace (internal linkage), so +// no other translation unit -- including this test file -- can name them. +// Left as a known, permanent coverage ceiling rather than weakening the +// encapsulation just to reach 100%. From 89114d98c3da90b8d2865f2971011c7ae1a9b22b Mon Sep 17 00:00:00 2001 From: Revar Desmera Date: Fri, 24 Jul 2026 10:01:41 -0700 Subject: [PATCH 3/3] Fix comment round-trip bug and populate declaration signature comment fields fmtMultilineArgsGeneric now moves a leading `//` comment onto the previous line instead of giving it a fresh one, so re-parsing no longer misclassifies it as standalone and loses its attachment (was affecting all 6 call sites that share this function: call args, echo/assert args, function/module params). Also implements claimDeclSignatureComments/attachDeclarationComments, which populate FunctionDeclaration/ModuleDeclaration's preNameComments/ postNameComments/postParamsComments and ParameterDeclaration's leadingComments/trailingComments -- fields that were declared, serialized, and rendered but never actually populated by the comment-attachment pipeline. Recovers the parameter list's '(' / ')' positions (not captured by the grammar) via a raw-source scan that skips already-extracted comment spans. Co-Authored-By: Claude Sonnet 5 --- src/comment_attach_internal.hpp | 19 +++++ src/comments.cpp | 4 + src/inline_comment_attach.cpp | 127 ++++++++++++++++++++++++++++++++ src/pretty_print.cpp | 31 +++++++- tests/test_inline_comments.cpp | 92 +++++++++++++++++++++++ tests/test_pretty_print.cpp | 83 +++++++++++++++++---- 6 files changed, 341 insertions(+), 15 deletions(-) diff --git a/src/comment_attach_internal.hpp b/src/comment_attach_internal.hpp index dfdc2a2..9dcf9b4 100644 --- a/src/comment_attach_internal.hpp +++ b/src/comment_attach_internal.hpp @@ -3,6 +3,7 @@ #include "openscad_cpp_parser/ast.hpp" #include +#include #include namespace oscad { @@ -16,4 +17,22 @@ namespace oscad { void attachInlineComments(std::vector>& astNodes, std::vector> 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>& astNodes, const std::string& code, + std::vector>& comments); + } // namespace oscad diff --git a/src/comments.cpp b/src/comments.cpp index 56ecc7d..4e0e810 100644 --- a/src/comments.cpp +++ b/src/comments.cpp @@ -184,9 +184,13 @@ std::vector> injectComments(std::vector> attachComments(std::vector> astNodes, const std::string& code, const std::string& origin) { auto comments = extractComments(code, origin); + attachDeclarationComments(astNodes, code, comments); std::vector> inlineComments; std::vector> standalone; for (auto& c : comments) { + if (!c) { + continue; + } if (isInlineComment(*c, code)) { inlineComments.push_back(std::move(c)); } else { diff --git a/src/inline_comment_attach.cpp b/src/inline_comment_attach.cpp index 891aab0..3955017 100644 --- a/src/inline_comment_attach.cpp +++ b/src/inline_comment_attach.cpp @@ -374,6 +374,96 @@ void classifyNode(ASTNode& node, std::vector& 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>& comments) { + size_t i = static_cast(from); + while (i < code.size()) { + bool skipped = false; + for (const auto& c : comments) { + if (c && startOffsetOf(*c) == static_cast(i)) { + i = static_cast(endOffsetOf(*c)); + skipped = true; + break; + } + } + if (skipped) { + continue; + } + if (code[i] == ch) { + return static_cast(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>& out, + std::vector>& 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(static_cast(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& name, + std::vector>& parameters, int bodyStart, + std::vector>& preName, + std::vector>& postName, + std::vector>& postParams, const std::string& code, + std::vector>& 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 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>& comments) { + if (node.kind() == NodeKind::FunctionDeclaration) { + auto& n = static_cast(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(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 exprFields; + std::vector 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>& astNodes, const std::string& code, + std::vector>& comments) { + for (auto& node : astNodes) { + if (node) { + walkAttachDeclComments(*node, code, comments); + } + } +} + void attachInlineComments(std::vector>& astNodes, std::vector> inlineComments) { if (inlineComments.empty()) { diff --git a/src/pretty_print.cpp b/src/pretty_print.cpp index 971d310..4cdec0f 100644 --- a/src/pretty_print.cpp +++ b/src/pretty_print.cpp @@ -250,7 +250,36 @@ std::string fmtMultilineArgsGeneric(const std::string& head, const std::vector 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 + ")"; } diff --git a/tests/test_inline_comments.cpp b/tests/test_inline_comments.cpp index b7c63a8..3c2f54c 100644 --- a/tests/test_inline_comments.cpp +++ b/tests/test_inline_comments.cpp @@ -160,6 +160,98 @@ TEST(InlineComments, TrailingCommentAfterUseStatementFallsThroughWithoutCrashing EXPECT_NE(dynamic_cast(ast[1].get()), nullptr); } +// --- Declaration signature comments (preNameComments/postNameComments/ +// postParamsComments/ParameterDeclaration.leadingComments/trailingComments) +// -- previously declared, serialized, and rendered, but never populated by +// the comment-attachment pipeline. See claimDeclSignatureComments +// (inline_comment_attach.cpp) for the fix. + +TEST(InlineComments, ParameterWithNoDefaultGetsLeadingComment) { + // The case the old generic exprField mechanism could never reach at + // all: a parameter with no default value is never a wrappable + // Expression slot (addParameterExprList only adds parameters that HAVE + // a defaultValue), so this comment used to fall through onto the + // function body instead. + auto ast = getASTFromString("function f(/* lead */ x) = x;\n", /*includeComments=*/true); + ASSERT_EQ(ast.size(), 1u); + auto* decl = dynamic_cast(ast[0].get()); + ASSERT_NE(decl, nullptr); + ASSERT_EQ(decl->parameters.size(), 1u); + ASSERT_EQ(decl->parameters[0]->leadingComments.size(), 1u); + EXPECT_EQ(decl->expr->kind(), NodeKind::Identifier) << "must NOT have fallen through to the body"; +} + +TEST(InlineComments, PreNameCommentPopulatesField) { + auto ast = getASTFromString("function /* pre */ f(x) = x;\n", /*includeComments=*/true); + ASSERT_EQ(ast.size(), 1u); + auto* decl = dynamic_cast(ast[0].get()); + ASSERT_NE(decl, nullptr); + ASSERT_EQ(decl->preNameComments.size(), 1u); +} + +TEST(InlineComments, PostNameCommentPopulatesFieldViaParenScan) { + // Requires locating the real '(' token by scanning raw source (the + // grammar captures no location for it) -- exercises that scan directly, + // not just via the rendered/round-tripped output. + auto ast = getASTFromString("function f /* post-name */ (x) = x;\n", /*includeComments=*/true); + ASSERT_EQ(ast.size(), 1u); + auto* decl = dynamic_cast(ast[0].get()); + ASSERT_NE(decl, nullptr); + ASSERT_EQ(decl->postNameComments.size(), 1u); +} + +TEST(InlineComments, PostParamsCommentPopulatesField) { + auto ast = getASTFromString("module m(x) /* post-params */ { cube(x); }\n", /*includeComments=*/true); + ASSERT_EQ(ast.size(), 1u); + auto* decl = dynamic_cast(ast[0].get()); + ASSERT_NE(decl, nullptr); + ASSERT_EQ(decl->postParamsComments.size(), 1u); +} + +TEST(InlineComments, TrailingCommentOnLastParameterPopulatesField) { + auto ast = getASTFromString("module m(x /* trail */) { cube(x); }\n", /*includeComments=*/true); + ASSERT_EQ(ast.size(), 1u); + auto* decl = dynamic_cast(ast[0].get()); + ASSERT_NE(decl, nullptr); + ASSERT_EQ(decl->parameters.size(), 1u); + ASSERT_EQ(decl->parameters[0]->trailingComments.size(), 1u); +} + +TEST(InlineComments, EmptyParameterListCommentFallsIntoPostParams) { + // ponytail case: no parameter exists to own a comment inside `()`, so + // it's folded into postParamsComments rather than a dedicated field. + auto ast = getASTFromString("module m(/* empty */) { cube(1); }\n", /*includeComments=*/true); + ASSERT_EQ(ast.size(), 1u); + auto* decl = dynamic_cast(ast[0].get()); + ASSERT_NE(decl, nullptr); + EXPECT_TRUE(decl->parameters.empty()); + ASSERT_EQ(decl->postParamsComments.size(), 1u); +} + +TEST(InlineComments, CommentBetweenTwoParametersAttachesToSecondsLeading) { + auto ast = getASTFromString("module m(x, /* mid */ y) { cube(x + y); }\n", /*includeComments=*/true); + ASSERT_EQ(ast.size(), 1u); + auto* decl = dynamic_cast(ast[0].get()); + ASSERT_NE(decl, nullptr); + ASSERT_EQ(decl->parameters.size(), 2u); + EXPECT_TRUE(decl->parameters[0]->trailingComments.empty()); + ASSERT_EQ(decl->parameters[1]->leadingComments.size(), 1u); +} + +TEST(InlineComments, NestedModuleDeclarationInsideModuleBodyStillGetsOwnComments) { + // Confirms walkAttachDeclComments' recursion reaches a declaration + // nested inside another declaration's body, not just top-level ones. + auto ast = getASTFromString("module outer() { module inner(/* lead */ x) { cube(x); } }\n", /*includeComments=*/true); + ASSERT_EQ(ast.size(), 1u); + auto* outer = dynamic_cast(ast[0].get()); + ASSERT_NE(outer, nullptr); + ASSERT_EQ(outer->children.size(), 1u); + auto* inner = dynamic_cast(outer->children[0].get()); + ASSERT_NE(inner, nullptr); + ASSERT_EQ(inner->parameters.size(), 1u); + ASSERT_EQ(inner->parameters[0]->leadingComments.size(), 1u); +} + TEST(InlineComments, TwoStatementsEachWithOwnCommentSkipsAlreadyConsumedSlot) { // walkAttach's own per-comment classification loop re-scans the *whole* // shared comments vector at every node it visits -- so once the first diff --git a/tests/test_pretty_print.cpp b/tests/test_pretty_print.cpp index 3cfdedb..7caa093 100644 --- a/tests/test_pretty_print.cpp +++ b/tests/test_pretty_print.cpp @@ -281,15 +281,22 @@ TEST(PrettyPrint, MultipleLeadingLineCommentsOnExpression) { // Two leading `//` comments, each on its own argument (both classified // inline since something precedes each on its own line) -- exercises // the multi-comment leading-line-comment path in fmtCommentedExpr, not - // just a single one. Not idempotent (a real, pre-existing quirk: the - // printed one-comment-per-line form re-parses with each comment - // attaching to a different argument than the first parse gave it) -- - // asserting on the first print's shape only, not full round-trip - // stability, since that's a separate concern from this file's own - // coverage gap. + // just a single one. + // + // This used to NOT be idempotent: fmtMultilineArgsGeneric printed each + // leading line-comment on its own fresh line ("foo(\n a,\n // + // one\n b,\n..."), which re-parses wrong -- a `//` comment with + // nothing before it on its own line is classified *standalone*, not + // inline, so re-parsing split it off into its own top-level node + // instead of keeping it attached to `b`. Fixed by moving a leading + // "//..." line onto the end of the *previous* argument's own line + // instead (mirrors how fmtListComprehension already handles the same + // situation for list elements) -- see fmtMultilineArgsGeneric's own + // comment for the full story. + expectStablePrintWithComments("foo(a // one\n, b // two\n, c);\n"); std::string out = toOpenscad(getASTFromString("foo(a // one\n, b // two\n, c);\n", /*includeComments=*/true)); - EXPECT_NE(out.find("// one"), std::string::npos); - EXPECT_NE(out.find("// two"), std::string::npos); + EXPECT_NE(out.find("a, // one"), std::string::npos); + EXPECT_NE(out.find("b, // two"), std::string::npos); } TEST(PrettyPrint, TernaryNestedInTrueBranch) { @@ -346,16 +353,64 @@ TEST(PrettyPrint, LongPrimaryCallExpressionWraps) { // actual observed behavior rather than asserting the field population // that doesn't happen. joinComments'/fmtParameter's own leading/trailing- // comment loops stay permanently unreachable as a result. -TEST(PrettyPrint, ParameterAdjacentCommentFallsThroughToBody) { +TEST(PrettyPrint, ParameterAdjacentCommentAttachesToParameter) { + // Used to fall through to the function BODY's own leading comment + // (ParameterDeclaration.leadingComments was declared, serialized, and + // rendered, but never populated by the comment-attachment pipeline -- + // addParameterExprList only treats a parameter as a wrappable + // Expression slot when it HAS a default value, so a bare `x` was never + // even a candidate). Fixed by claimDeclSignatureComments + // (inline_comment_attach.cpp), which claims signature-gap comments + // before the generic exprField-based mechanism ever sees them. + expectStablePrintWithComments("function f(/* lead */ x) = x;\n"); std::string out = toOpenscad(getASTFromString("function f(/* lead */ x) = x;\n", /*includeComments=*/true)); - EXPECT_NE(out.find("/* lead */"), std::string::npos); - EXPECT_EQ(out.find("(/* lead */ x)"), std::string::npos) << "not actually attached to the parameter"; + EXPECT_NE(out.find("(/* lead */ x)"), std::string::npos); +} + +TEST(PrettyPrint, DeclarationNameAdjacentCommentAttachesNearName) { + // Used to fall through to the function body for the same reason as + // above -- preNameComments/postNameComments/postParamsComments were + // also declared-but-never-populated. This exercises postNameComments + // specifically (a comment between the name and '(' -- claimed via a + // raw-text scan for the '(' token, since the grammar doesn't capture + // its own position). + expectStablePrintWithComments("function f /* post-name */ (x) = x;\n"); + std::string out = toOpenscad(getASTFromString("function f /* post-name */ (x) = x;\n", /*includeComments=*/true)); + EXPECT_NE(out.find("function f /* post-name */("), std::string::npos); } -TEST(PrettyPrint, DeclarationNameAdjacentCommentFallsThroughToBody) { +TEST(PrettyPrint, PreNameCommentAttachesBeforeName) { + expectStablePrintWithComments("function /* pre */ f(x) = x;\n"); std::string out = toOpenscad(getASTFromString("function /* pre */ f(x) = x;\n", /*includeComments=*/true)); - EXPECT_NE(out.find("/* pre */"), std::string::npos); - EXPECT_EQ(out.find("function /* pre */ f"), std::string::npos) << "not actually attached near the name"; + EXPECT_NE(out.find("function /* pre */ f"), std::string::npos); +} + +TEST(PrettyPrint, PostParamsCommentAttachesAfterCloseParen) { + expectStablePrintWithComments("module m(x) /* post-params */ { cube(x); }\n"); + std::string out = toOpenscad(getASTFromString("module m(x) /* post-params */ { cube(x); }\n", /*includeComments=*/true)); + EXPECT_NE(out.find(") /* post-params */"), std::string::npos); +} + +TEST(PrettyPrint, TrailingCommentOnLastParameterAttachesToParameter) { + expectStablePrintWithComments("module m(x /* trail */) { cube(x); }\n"); + std::string out = toOpenscad(getASTFromString("module m(x /* trail */) { cube(x); }\n", /*includeComments=*/true)); + EXPECT_NE(out.find("x /* trail */)"), std::string::npos); +} + +TEST(PrettyPrint, CommentInEmptyParameterListAttachesToPostParams) { + // No parameter exists to own this comment, so it's folded into + // postParamsComments (rendered after the closing paren) rather than + // some dedicated "inside empty parens" slot -- see + // claimDeclSignatureComments' own ponytail comment. + expectStablePrintWithComments("module m(/* empty */) { cube(1); }\n"); + std::string out = toOpenscad(getASTFromString("module m(/* empty */) { cube(1); }\n", /*includeComments=*/true)); + EXPECT_NE(out.find("m() /* empty */ {"), std::string::npos); +} + +TEST(PrettyPrint, CommentBetweenTwoParametersAttachesToSecond) { + expectStablePrintWithComments("module m(x, /* mid */ y) { cube(x + y); }\n"); + std::string out = toOpenscad(getASTFromString("module m(x, /* mid */ y) { cube(x + y); }\n", /*includeComments=*/true)); + EXPECT_NE(out.find(", /* mid */ y"), std::string::npos); } TEST(PrettyPrint, ListCompForLongAssignmentsWrap) {