From 343d7dd5e47b817999098605370f0eab706675f0 Mon Sep 17 00:00:00 2001 From: Martin Vogel Date: Sun, 20 Sep 2026 19:45:31 +0200 Subject: [PATCH] fix(extract): a C-family function's return type keeps its pointer and qualifiers Distilled from #1245 by Andrew Hundt (c51e5cb0, the extract_defs.c half; the mcp.c and token-reduction parts are not taken). For C-family functions return_type was the text of the declaration's `type` node alone. In these grammars the `*`, `&` and `&&` live on the declarator and cv-qualifiers are sibling nodes, so const char *get_name(void) published char robj **lookupKeys(...) published robj Text &operator=(const Text &) published Text and no C or C++ function in any index had a pointer return type at all. The C/C++ resolver's return-type parser already strips a leading const/volatile and handles trailing `*`, `&`, `&&` -- branches that were unreachable for functions, so every `Foo *f()` was registered as returning Foo by value. c_declared_return_type() renders the declared type at both sites (functions and methods) for C, C++, CUDA, GLSL, HLSL, ISPC, Slang and Objective-C -- the list resolve_func_name_c_family uses: so `const char *`, `char **`, `Text &`, `Text &&`, `Text *&`, `volatile unsigned long *`; a pointer-level qualifier follows its star (`char *const *`); east-const `char const *` normalises to `const char *`. With no qualifier and no marker the old text is returned unchanged. It measures, then writes into one exact-size arena allocation; the declarator walk is a plain child chain, no recursion. def.return_types is untouched. Two deliberate differences from the upstream hunk. Upstream copied every type_qualifier node, which in these grammars includes constexpr, _Noreturn, mutable and __extension__ -- `constexpr int f()` would have become `constexpr int`. Only const, volatile, restrict, __restrict, __restrict__ and _Atomic are kept, and both cases are asserted unchanged. And the walk stops at the first declarator that is neither pointer nor reference, so `int (*f(void))(int)` stays `int` rather than becoming a wrong `int *`. RED, production reverted and the final tests kept: c_function_return_type_preserves_pointer_and_qualifier FAIL tests/test_extraction.c:1005: "char" != "const char *" cpp_method_return_type_preserves_pointer_and_qualifier FAIL tests/test_extraction.c:1072: "char" != "const char *" 351 passed, 2 failed c_function_return_type_plain_unchanged is the control: it passes before and after. GREEN: extraction 353, c_lsp 762, lang_contract 41, pipeline 281. Effect on real code -- Redis, production binaries, old against new. 39,132 nodes in both. 1,442 Function nodes change, in return_type only: void -> void * (270), char -> const char * (101), unsigned char -> unsigned char * (91), char -> char * (81), robj -> robj * (57), ... Functions with a `*` in return_type: 0 -> 1,440. With one worker the complete edge set apart from SEMANTICALLY_RELATED is byte-identical old against new across six runs (three each) -- CALLS, USAGE, WRITES, IMPORTS and the rest do not move. SEMANTICALLY_RELATED goes 350 -> 325 because that pass tokenises the return-type string into its type vector. One thing measured rather than assumed. With several workers, which `dict.h` Redis' src/dict.c imports (src/ or deps/hiredis/) depends on worker merge order. That is main's defect, not this change's: unmodified main picked src/dict.h in 2 of 10 runs and the hiredis header in 8; with one worker both binaries always pick src/dict.h. This change picked src/dict.h in 6 of 10. At ten runs a side that gap is not separable from chance (Fisher exact, two-sided p = 0.17), and a shifted rate would be unsurprising for a timing-dependent pick; it is recorded here rather than explained. The include-target fix is a separate change (#2227). Known limits, all pre-existing and out of scope: struct and class FIELDS have the same defect (`const char *data;` publishes char); C prototypes and declaration-only C++ methods are not extracted as definitions, so they are not reached; where a macro is misparsed as the type (`LUA_API const char *f()`), the base was already wrong and now reads `const LUA_API *`; the resolver's parser does not yet understand a pointer-level qualifier or a leading _Atomic/restrict and falls back to a named-type lookup for those, as it effectively did for every pointer return before. Co-authored-by: Andrew Hundt Signed-off-by: Martin Vogel --- internal/cbm/extract_defs.c | 157 +++++++++++++++++++++++++++++++++++- tests/test_extraction.c | 98 ++++++++++++++++++++++ 2 files changed, 253 insertions(+), 2 deletions(-) diff --git a/internal/cbm/extract_defs.c b/internal/cbm/extract_defs.c index 93da084be..916f1136f 100644 --- a/internal/cbm/extract_defs.c +++ b/internal/cbm/extract_defs.c @@ -3477,6 +3477,159 @@ static TSNode find_function_params(TSNode func_node, CBMLanguage lang) { return params; } +/* ── C-family declared return type ────────────────────────────────── + * The C-family grammars split a declared return type three ways: the `type` + * field (`char`), sibling type_qualifier nodes (`const`), and the + * pointer/reference declarators wrapping the function declarator (`*`). Taking + * only the `type` field published `const char *get_name(void)` as "char". + * + * Canonical spelling: leading cv-qualifiers in source order, the base type text + * verbatim, then one space and the declarator markers outermost-first with no + * space between them — `const char *`, `char **`, `Text &`, `Text *&`. A + * qualifier on a pointer level follows its `*` and is separated from the next + * marker by a space: `char *const *`. A qualifier written after the base type + * (`char const *`) is normalized to the leading position. */ + +/* Output sink: measures when buf is NULL, writes otherwise. Rendering twice + * sizes the arena allocation exactly without a second copy of the logic. */ +typedef struct { + char *buf; + size_t len; +} c_rt_out_t; + +static void c_rt_put(c_rt_out_t *out, const char *text, size_t n) { + if (out->buf) { + memcpy(out->buf + out->len, text, n); + } + out->len += n; +} + +static void c_rt_put_str(c_rt_out_t *out, const char *text) { + c_rt_put(out, text, strlen(text)); +} + +static void c_rt_put_node(c_rt_out_t *out, TSNode node, const char *source) { + uint32_t start = ts_node_start_byte(node); + uint32_t end = ts_node_end_byte(node); + if (end > start) { + c_rt_put(out, source + start, (size_t)(end - start)); + } +} + +/* type_qualifier also covers keywords that are not part of the type + * (`constexpr`, `_Noreturn`, `mutable`, `__extension__`, …). Keep only the ones + * that are, so `constexpr int f()` still returns "int". */ +static bool is_c_return_cv_qualifier(TSNode node, const char *source) { + static const char *const kept[] = {"const", "volatile", "restrict", "__restrict", + "__restrict__", "_Atomic", NULL}; + if (strcmp(ts_node_type(node), "type_qualifier") != 0) { + return false; + } + uint32_t start = ts_node_start_byte(node); + uint32_t end = ts_node_end_byte(node); + size_t len = end > start ? (size_t)(end - start) : 0; + for (const char *const *k = kept; *k; k++) { + if (strlen(*k) == len && memcmp(source + start, *k, len) == 0) { + return true; + } + } + return false; +} + +static bool is_c_declarator_lang(CBMLanguage lang) { + return lang == CBM_LANG_C || lang == CBM_LANG_CPP || lang == CBM_LANG_CUDA || + lang == CBM_LANG_GLSL || lang == CBM_LANG_HLSL || lang == CBM_LANG_ISPC || + lang == CBM_LANG_SLANG || lang == CBM_LANG_OBJC; +} + +/* Render the canonical return type into `out`; returns how many qualifiers and + * markers were added around the base type (0 = the base text alone is already + * the whole type). The declarator walk is one strict child chain, so it is + * O(depth) with no recursion and needs no depth cap. It stops at the first node + * that is neither a pointer nor a reference declarator: for a function returning + * a function pointer (`int (*f(void))(int)`) that is the outer + * function_declarator, which leaves the base type as it was. */ +static size_t c_rt_render(c_rt_out_t *out, TSNode func_node, TSNode type_node, TSNode declarator, + const char *source) { + size_t added = 0; + uint32_t decl_start = ts_node_start_byte(declarator); + uint32_t nc = ts_node_named_child_count(func_node); + for (uint32_t i = 0; i < nc; i++) { + TSNode ch = ts_node_named_child(func_node, i); + if (ts_node_start_byte(ch) >= decl_start) { + break; + } + if (is_c_return_cv_qualifier(ch, source)) { + c_rt_put_node(out, ch, source); + c_rt_put_str(out, " "); + added++; + } + } + c_rt_put_node(out, type_node, source); + + bool need_space = true; + TSNode decl = declarator; + while (!ts_node_is_null(decl)) { + const char *dk = ts_node_type(decl); + bool is_ref = strcmp(dk, "reference_declarator") == 0; + if (!is_ref && strcmp(dk, "pointer_declarator") != 0) { + break; + } + if (need_space) { + c_rt_put_str(out, " "); + need_space = false; + } + /* A reference_declarator opens with its `&` / `&&` token. */ + TSNode marker = ts_node_child(decl, 0); + if (is_ref && !ts_node_is_null(marker) && !ts_node_is_named(marker)) { + c_rt_put_node(out, marker, source); + } else { + c_rt_put_str(out, is_ref ? "&" : "*"); + } + added++; + uint32_t dn = ts_node_named_child_count(decl); + for (uint32_t i = 0; i < dn; i++) { + TSNode q = ts_node_named_child(decl, i); + if (is_c_return_cv_qualifier(q, source)) { + c_rt_put_node(out, q, source); + need_space = true; + added++; + } + } + /* tree-sitter-cpp/-cuda give a reference_declarator's inner declarator no + * `declarator` field (see find_c_params); it is the one named child. */ + TSNode inner = ts_node_child_by_field_name(decl, TS_FIELD("declarator")); + if (ts_node_is_null(inner) && is_ref && dn > 0) { + inner = ts_node_named_child(decl, 0); + } + decl = inner; + } + return added; +} + +/* Declared return type of a C-family function/method node whose `type` field is + * `type_node`. Any other language, and any type with nothing around its base + * type, gets the base type text exactly as before. */ +static char *c_declared_return_type(CBMExtractCtx *ctx, TSNode func_node, TSNode type_node) { + CBMArena *a = ctx->arena; + TSNode declarator = ts_node_child_by_field_name(func_node, TS_FIELD("declarator")); + if (!is_c_declarator_lang(ctx->language) || ts_node_is_null(declarator)) { + return cbm_node_text(a, type_node, ctx->source); + } + c_rt_out_t out = {NULL, 0}; + if (c_rt_render(&out, func_node, type_node, declarator, ctx->source) == 0) { + return cbm_node_text(a, type_node, ctx->source); + } + out.buf = (char *)cbm_arena_alloc(a, out.len + NULL_TERM); + if (!out.buf) { + return cbm_node_text(a, type_node, ctx->source); + } + out.len = 0; + (void)c_rt_render(&out, func_node, type_node, declarator, ctx->source); + out.buf[out.len] = '\0'; + return out.buf; +} + // C++: resolve trailing return type (auto f() -> Type) on a declarator node. // Updates def->return_type and def->return_types if trailing type found. static void resolve_cpp_trailing_return(CBMArena *a, TSNode func_node, const char *source, @@ -3712,7 +3865,7 @@ static void extract_func_def(CBMExtractCtx *ctx, TSNode node, const CBMLangSpec for (const char **f = rt_fields; *f; f++) { TSNode rt = ts_node_child_by_field_name(func_node, *f, (uint32_t)strlen(*f)); if (!ts_node_is_null(rt)) { - def.return_type = cbm_node_text(a, rt, ctx->source); + def.return_type = c_declared_return_type(ctx, func_node, rt); def.return_types = extract_return_types(a, rt, ctx->source, ctx->language); break; } @@ -4964,7 +5117,7 @@ static void push_method_def(CBMExtractCtx *ctx, TSNode child, TSNode class_node, for (const char **f = rt_fields; *f; f++) { TSNode rt = ts_node_child_by_field_name(child, *f, (uint32_t)strlen(*f)); if (!ts_node_is_null(rt)) { - def.return_type = cbm_node_text(a, rt, ctx->source); + def.return_type = c_declared_return_type(ctx, child, rt); break; } } diff --git a/tests/test_extraction.c b/tests/test_extraction.c index 8d9bf742b..331aa54f3 100644 --- a/tests/test_extraction.c +++ b/tests/test_extraction.c @@ -977,6 +977,65 @@ TEST(c_struct) { PASS(); } +/* return_type of the first definition named `name`; NULL when there is no such + * definition or it carries no return type. */ +static const char *def_return_type(CBMFileResult *r, const char *name) { + for (int i = 0; i < r->defs.count; i++) { + if (strcmp(r->defs.items[i].name, name) == 0) { + return r->defs.items[i].return_type; + } + } + return NULL; +} + +/* PR #1245: the C grammar splits a declared return type across the `type` node, + * sibling type_qualifier nodes and the pointer_declarator chain wrapping the + * function declarator. Taking only the `type` node published `const char *` as + * "char". Canonical spelling: qualifiers, base type, one space, then the + * declarator markers unspaced (`const char *`, `char **`). */ +TEST(c_function_return_type_preserves_pointer_and_qualifier) { + CBMFileResult *r = extract("static const char *text_end(const char *text) { return text; }\n" + "char **table(void) { return 0; }\n" + "char const *east_const(void) { return 0; }\n" + "const struct Point *find_point(void) { return 0; }\n" + "volatile unsigned long *reg(void) { return 0; }\n" + "char *const *frozen(void) { return 0; }\n", + CBM_LANG_C, "t", "returns.c"); + ASSERT_NOT_NULL(r); + ASSERT_FALSE(r->has_error); + ASSERT_STR_EQ(def_return_type(r, "text_end"), "const char *"); + ASSERT_STR_EQ(def_return_type(r, "table"), "char **"); + ASSERT_STR_EQ(def_return_type(r, "east_const"), "const char *"); + ASSERT_STR_EQ(def_return_type(r, "find_point"), "const struct Point *"); + ASSERT_STR_EQ(def_return_type(r, "reg"), "volatile unsigned long *"); + ASSERT_STR_EQ(def_return_type(r, "frozen"), "char *const *"); + cbm_free_result(r); + PASS(); +} + +/* Guard for the fix above: a return type with no qualifier and no declarator + * marker is already correct and must come out byte-identical. */ +TEST(c_function_return_type_plain_unchanged) { + CBMFileResult *r = extract("int scalar(void) { return 0; }\n" + "void nothing(void) {}\n" + "unsigned long wide(void) { return 0; }\n" + "struct Point make_point(void) { struct Point p; return p; }\n" + "static inline size_t count(void) { return 0; }\n" + "_Noreturn void die(void) { for (;;) {} }\n", + CBM_LANG_C, "t", "plain.c"); + ASSERT_NOT_NULL(r); + ASSERT_FALSE(r->has_error); + ASSERT_STR_EQ(def_return_type(r, "scalar"), "int"); + ASSERT_STR_EQ(def_return_type(r, "nothing"), "void"); + ASSERT_STR_EQ(def_return_type(r, "wide"), "unsigned long"); + ASSERT_STR_EQ(def_return_type(r, "make_point"), "struct Point"); + ASSERT_STR_EQ(def_return_type(r, "count"), "size_t"); + /* _Noreturn parses as a type_qualifier but is not part of the type. */ + ASSERT_STR_EQ(def_return_type(r, "die"), "void"); + cbm_free_result(r); + PASS(); +} + /* --- C++ --- */ TEST(cpp_class) { CBMFileResult *r = extract( @@ -990,6 +1049,42 @@ TEST(cpp_class) { PASS(); } +/* PR #1245, C++ side: in-class methods go through a separate extraction path + * from free functions, and C++ adds reference markers (`&`, `&&`) whose + * reference_declarator carries no `declarator` field. */ +TEST(cpp_method_return_type_preserves_pointer_and_qualifier) { + CBMFileResult *r = extract("class Text {\n" + "public:\n" + " const char *end() { return nullptr; }\n" + " char **table() { return nullptr; }\n" + " Text &self() { return *this; }\n" + " const Text &cself() const { return *this; }\n" + " Text *&slot() { return next_; }\n" + " Text *next_;\n" + " int width() const { return 0; }\n" + " constexpr int square(int x) const { return x * x; }\n" + "};\n" + "const Text &shared() { static Text t; return t; }\n" + "Text &&moved(Text &t) { return static_cast(t); }\n" + "const char *Text::c_str() const { return nullptr; }\n", + CBM_LANG_CPP, "t", "text.cpp"); + ASSERT_NOT_NULL(r); + ASSERT_FALSE(r->has_error); + ASSERT_STR_EQ(def_return_type(r, "end"), "const char *"); + ASSERT_STR_EQ(def_return_type(r, "table"), "char **"); + ASSERT_STR_EQ(def_return_type(r, "self"), "Text &"); + ASSERT_STR_EQ(def_return_type(r, "cself"), "const Text &"); + ASSERT_STR_EQ(def_return_type(r, "slot"), "Text *&"); + ASSERT_STR_EQ(def_return_type(r, "width"), "int"); + /* constexpr parses as a type_qualifier but is not part of the type. */ + ASSERT_STR_EQ(def_return_type(r, "square"), "int"); + ASSERT_STR_EQ(def_return_type(r, "shared"), "const Text &"); + ASSERT_STR_EQ(def_return_type(r, "moved"), "Text &&"); + ASSERT_STR_EQ(def_return_type(r, "c_str"), "const char *"); + cbm_free_result(r); + PASS(); +} + /* ═══════════════════════════════════════════════════════════════════ * Group C: Scripting / Dynamic Languages * ═══════════════════════════════════════════════════════════════════ */ @@ -8206,8 +8301,11 @@ SUITE(extraction) { RUN_TEST(go_interface); RUN_TEST(zig_function); RUN_TEST(c_function); + RUN_TEST(c_function_return_type_preserves_pointer_and_qualifier); + RUN_TEST(c_function_return_type_plain_unchanged); RUN_TEST(c_struct); RUN_TEST(cpp_class); + RUN_TEST(cpp_method_return_type_preserves_pointer_and_qualifier); /* Scripting */ RUN_TEST(python_function);