From 0843106fc511963460a71f44d03d62114639f5cb Mon Sep 17 00:00:00 2001 From: harshitaajoshi Date: Sun, 27 Sep 2026 18:55:55 -0500 Subject: [PATCH] fix(extract): address review on PHP inline-HTML masking (#2000) Signed-off-by: harshitaajoshi --- internal/cbm/php_inline_html.c | 65 +++++++++++++++++++++++++--------- tests/test_extraction.c | 52 +++++++++++++++++++++++++++ tests/test_php_lsp.c | 60 +++++++++++++++++++++++++++++++ tests/test_pipeline.c | 10 +----- 4 files changed, 161 insertions(+), 26 deletions(-) diff --git a/internal/cbm/php_inline_html.c b/internal/cbm/php_inline_html.c index df5652d54..e766a1fbd 100644 --- a/internal/cbm/php_inline_html.c +++ b/internal/cbm/php_inline_html.c @@ -263,6 +263,47 @@ static int php_scan_code(const char *s, int len, int i) { return len; } +/* The rewritten buffer. It is allocated and filled from the source on the + * first write, so a file whose only `?>` bytes sit in strings or comments + * costs no copy. */ +typedef struct { + CBMArena *arena; + const char *source; + int len; + char *out; + bool failed; +} PHPMask; + +static char *php_mask_writable(PHPMask *m) { + if (!m->out && !m->failed) { + m->out = (char *)cbm_arena_alloc(m->arena, (size_t)m->len + SKIP_ONE); + if (m->out) { + memcpy(m->out, m->source, (size_t)m->len); + m->out[m->len] = '\0'; + } else { + m->failed = true; + } + } + return m->out; +} + +/* Blank [from, to) of the rewritten buffer, keeping line breaks. */ +static void php_mask_blank(PHPMask *m, int from, int to) { + char *out = php_mask_writable(m); + if (out) { + php_blank(out, from, to); + } +} + +/* Turn the `?>` at i into `; `. */ +static void php_mask_close_tag(PHPMask *m, int i) { + char *out = php_mask_writable(m); + if (out) { + out[i] = ';'; + out[i + SKIP_ONE] = ' '; + } +} + const char *cbm_php_mask_inline_html(CBMArena *arena, const char *source, int source_len) { if (!arena || !source || source_len <= 0) { return source; @@ -273,17 +314,10 @@ const char *cbm_php_mask_inline_html(CBMArena *arena, const char *source, int so return source; } - char *out = (char *)cbm_arena_alloc(arena, (size_t)source_len + SKIP_ONE); - if (!out) { - return source; - } - memcpy(out, source, (size_t)source_len); - out[source_len] = '\0'; - - bool changed = false; + PHPMask m = {.arena = arena, .source = source, .len = source_len}; bool seen_open = false; int i = 0; - while (i < source_len) { + while (i < source_len && !m.failed) { /* Inline-HTML mode: blank up to the next open tag. */ int html_start = i; int tag = 0; @@ -291,15 +325,13 @@ const char *cbm_php_mask_inline_html(CBMArena *arena, const char *source, int so i++; } if (i > html_start) { - php_blank(out, html_start, i); - changed = true; + php_mask_blank(&m, html_start, i); } if (i >= source_len) { break; } if (seen_open) { - php_blank(out, i, i + tag); - changed = true; + php_mask_blank(&m, i, i + tag); } seen_open = true; @@ -308,10 +340,9 @@ const char *cbm_php_mask_inline_html(CBMArena *arena, const char *source, int so if (close < 0 || close >= source_len) { break; } - out[close] = ';'; - out[close + SKIP_ONE] = ' '; - changed = true; + php_mask_close_tag(&m, close); i = close + PAIR_LEN; } - return changed ? out : source; + /* On allocation failure parse the file as it is: the old behaviour. */ + return (m.out && !m.failed) ? m.out : source; } diff --git a/tests/test_extraction.c b/tests/test_extraction.c index c8d47ca52..4d03423c1 100644 --- a/tests/test_extraction.c +++ b/tests/test_extraction.c @@ -938,6 +938,56 @@ TEST(php_inline_html_close_tag_in_literals_issue2000) { PASS(); } +/* Markup is blanked, never parsed: code-shaped text outside the PHP tags must + * not produce definitions or calls, wherever it sits. */ +TEST(php_inline_html_markup_never_reaches_graph_issue2000) { + CBMFileResult *r = extract("

function before() { return leakBefore(); }

\n" + "\n" + "

function fake() { return leak(); }

\n" + "\n" + "function xmlFake() { leakXml(); }\n" + "\n" + "

class Tail { function tailFake() { leakTail(); } }

\n", + CBM_LANG_PHP, "t", "markup.php"); + ASSERT_NOT_NULL(r); + ASSERT_FALSE(r->has_error); + ASSERT(has_def(r, "Function", "real")); + ASSERT(has_def(r, "Function", "after")); + ASSERT(has_call(r, "real")); + ASSERT_FALSE(has_def_any(r, "before")); + ASSERT_FALSE(has_def_any(r, "fake")); + ASSERT_FALSE(has_def_any(r, "jsFake")); + ASSERT_FALSE(has_def_any(r, "xmlFake")); + ASSERT_FALSE(has_def_any(r, "Tail")); + ASSERT_FALSE(has_def_any(r, "tailFake")); + ASSERT_FALSE(has_call(r, "leak")); + ASSERT_FALSE(r->parse_incomplete); + cbm_free_result(r); + PASS(); +} + +/* The mask hands back the caller's buffer, uncopied, when nothing needs + * rewriting, and a same-length rewrite otherwise. */ +TEST(php_inline_html_mask_copies_only_on_write_issue2000) { + CBMArena arena; + cbm_arena_init(&arena); + const char *plain = "';\n/* ?> */\n$h = <<\nEOT;\n"; + const char *mixed = "\nx\n"; + ASSERT(cbm_php_mask_inline_html(&arena, plain, (int)strlen(plain)) == plain); + ASSERT(cbm_php_mask_inline_html(&arena, quoted, (int)strlen(quoted)) == quoted); + const char *masked = cbm_php_mask_inline_html(&arena, mixed, (int)strlen(mixed)); + ASSERT(masked != mixed); + ASSERT_EQ((int)strlen(masked), (int)strlen(mixed)); + ASSERT_STR_EQ(masked, "` (common at the end of older class files) is not a partial * parse, and CRLF line endings keep their line numbers. */ TEST(php_inline_html_trailing_close_tag_crlf_issue2000) { @@ -13252,6 +13302,8 @@ SUITE(extraction) { RUN_TEST(php_inline_html_tail_reaches_graph_issue2000); RUN_TEST(php_inline_html_template_shapes_issue2000); RUN_TEST(php_inline_html_close_tag_in_literals_issue2000); + RUN_TEST(php_inline_html_markup_never_reaches_graph_issue2000); + RUN_TEST(php_inline_html_mask_copies_only_on_write_issue2000); RUN_TEST(php_inline_html_trailing_close_tag_crlf_issue2000); RUN_TEST(ruby_class); RUN_TEST(ruby_module); diff --git a/tests/test_php_lsp.c b/tests/test_php_lsp.c index 04ff1001d..7c0314a27 100644 --- a/tests/test_php_lsp.c +++ b/tests/test_php_lsp.c @@ -110,6 +110,65 @@ TEST(phplsp_local_method_via_typed_param) { PASS(); } +/* #2000: with no cached tree (a result spilled under the memory budget comes + * back without one) the cross-file pass re-parses the on-disk source itself. + * It must blank inline HTML exactly as the per-file extract does, or a file's + * tail would lose cross-file resolution only when that file spilled. */ +static int find_resolved_arr(const CBMResolvedCallArray *arr, const char *callerSub, + const char *calleeSub) { + for (int i = 0; i < arr->count; i++) { + const CBMResolvedCall *rc = &arr->items[i]; + if (rc->caller_qn && strstr(rc->caller_qn, callerSub) && rc->callee_qn && + strstr(rc->callee_qn, calleeSub)) + return i; + } + return -1; +} + +TEST(phplsp_cross_reparse_reads_past_inline_html_issue2000) { + const char *src = "\n" + "
\n" + "

plain markup after the closing tag

\n" + "
\n" + "send(); }\n"; + CBMLSPDef defs[] = { + {.qualified_name = "test.main.gamma", + .short_name = "gamma", + .label = "Function", + .def_module_qn = "test.main"}, + {.qualified_name = "test.main.delta", + .short_name = "delta", + .label = "Function", + .def_module_qn = "test.main"}, + {.qualified_name = "test.mailer.Mailer", + .short_name = "Mailer", + .label = "Class", + .def_module_qn = "test.mailer"}, + {.qualified_name = "test.mailer.Mailer.send", + .short_name = "send", + .label = "Method", + .def_module_qn = "test.mailer", + .receiver_type = "test.mailer.Mailer"}, + }; + const char *imp_names[] = {"Mailer"}; + const char *imp_qns[] = {"test.mailer.Mailer"}; + + CBMArena arena; + cbm_arena_init(&arena); + CBMResolvedCallArray out = {0}; + + cbm_run_php_lsp_cross(&arena, src, (int)strlen(src), "test.main", defs, 4, imp_names, imp_qns, + 1, NULL, &out); + + ASSERT_GTE(find_resolved_arr(&out, "main.delta", "Mailer.send"), 0); + + cbm_arena_destroy(&arena); + PASS(); +} + /* ── 3. Arrow function with typed parameter ───────────────────── */ TEST(phplsp_arrow_function_typed_param) { @@ -5841,6 +5900,7 @@ SUITE(php_lsp) { /* Phase 1 baseline regressions */ RUN_TEST(phplsp_local_method_via_new_assignment); RUN_TEST(phplsp_local_method_via_typed_param); + RUN_TEST(phplsp_cross_reparse_reads_past_inline_html_issue2000); RUN_TEST(phplsp_arrow_function_typed_param); RUN_TEST(phplsp_static_call_resolved); RUN_TEST(phplsp_self_and_parent); diff --git a/tests/test_pipeline.c b/tests/test_pipeline.c index 94a281a07..c8094efac 100644 --- a/tests/test_pipeline.c +++ b/tests/test_pipeline.c @@ -1405,15 +1405,7 @@ TEST(pipeline_php_inline_html_calls_resolve_issue2000) { "function renderCard($item) { return formatPrice($item['cents']); }\n"}, }; for (size_t i = 0; i < sizeof(files) / sizeof(files[0]); i++) { - char path[512]; - snprintf(path, sizeof(path), "%s/%s", g_tmpdir, files[i].name); - FILE *f = fopen(path, "w"); - if (!f) { - teardown_test_repo(); - FAIL("failed to write php fixture"); - } - fputs(files[i].body, f); - fclose(f); + write_temp_file(g_tmpdir, files[i].name, files[i].body); } char db[512];