Skip to content

Commit f78bfe2

Browse files
committed
fix: address review feedback on external-import-shadow guard
- specifier_is_relative() now also recognizes Windows drive-letter (C:\ / C:/) and UNC (\\server\share) specifiers as in-tree, not external packages. pr-smoke runs this pipeline on Windows, and the previous POSIX-only check ('.' / '/') classified those specifiers as external, which could suppress a real edge on that platform only. Added external_import_shadow_windows_relative_specifier_kept (tests/test_registry.c): red before this fix, green after. - old_workers / old_single in the #1355 pipeline test are now const char*, matching getenv()'s read-only contract. - The pad_files literal (50) is now a named EXTERNAL_IMPORT_SHADOW_PARALLEL_PAD constant with a comment tying it to MIN_FILES_FOR_PARALLEL (a private #define in pipeline.c, not reachable for a static_assert from tests), so a future bump to that threshold can't silently drop the fixture back onto the sequential-only path. Signed-off-by: Yyunozor <yyunozor@icloud.com>
1 parent a3b1db1 commit f78bfe2

3 files changed

Lines changed: 73 additions & 6 deletions

File tree

‎src/pipeline/registry.c‎

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ enum { REG_MAX_CANDIDATES = 256 };
3131
#include "foundation/dyn_array.h"
3232
#include "foundation/platform.h"
3333

34+
#include <ctype.h>
3435
#include <math.h>
3536
#include <stdio.h>
3637
#include <stdlib.h>
@@ -465,10 +466,29 @@ bool cbm_suppress_weak_member_match(bool enabled, bool is_method, const char *st
465466
}
466467

467468
/* A module specifier that names a path inside the indexed tree ("./x", "../x",
468-
* "/abs/x") rather than an external package. Package specifiers are everything
469-
* else — "drizzle-orm", "rxjs/operators", "@scope/pkg". */
469+
* "/abs/x", and on Windows "C:\x", "C:/x", "\\server\share\x") rather than an
470+
* external package. Package specifiers are everything else — "drizzle-orm",
471+
* "rxjs/operators", "@scope/pkg". Windows paths must count as in-tree here:
472+
* pr-smoke runs this pipeline on Windows, and misclassifying a drive-letter or
473+
* UNC specifier as "external" is exactly the false-external edge this guard
474+
* exists to prevent (#1355). */
470475
static bool specifier_is_relative(const char *module_path) {
471-
return module_path && (module_path[0] == '.' || module_path[0] == '/');
476+
if (!module_path || !module_path[0]) {
477+
return false;
478+
}
479+
if (module_path[0] == '.' || module_path[0] == '/') {
480+
return true;
481+
}
482+
/* UNC share: \\server\share\... */
483+
if (module_path[0] == '\\' && module_path[1] == '\\') {
484+
return true;
485+
}
486+
/* Drive-letter absolute path: C:\repo\... or C:/repo/... */
487+
if (isalpha((unsigned char)module_path[0]) && module_path[1] == ':' &&
488+
(module_path[2] == '\\' || module_path[2] == '/')) {
489+
return true;
490+
}
491+
return false;
472492
}
473493

474494
static bool import_map_binds(const char **import_map_keys, int import_map_count,

‎tests/test_pipeline.c‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4956,6 +4956,17 @@ static int count_nodes_named(cbm_store_t *s, const char *project, const char *na
49564956
* (axios.get, api.patch on a renamed-axios instance, supertest request(app).get).
49574957
* The regex false edge must stay suppressed in parallel too. CBM_WORKERS forces
49584958
* >1 worker so the parallel path is taken regardless of the host core count. */
4959+
/* #1355: padding count for write_external_import_shadow_fixture, deliberately
4960+
* well past MIN_FILES_FOR_PARALLEL (=50, a private #define in
4961+
* src/pipeline/pipeline.c — not exposed to tests, so this cannot be a
4962+
* static_assert against it) rather than sitting right at the threshold. The
4963+
* margin, not the exact value, is what the test depends on: a modest bump to
4964+
* the real threshold must not silently drop this fixture back onto the
4965+
* sequential-only path and leave the parallel resolver unexercised. Same
4966+
* pattern as ET_PARALLEL_PAD / CP_PARALLEL_PAD in test_edge_types_probe.c /
4967+
* test_convergence_probe.c. */
4968+
enum { EXTERNAL_IMPORT_SHADOW_PARALLEL_PAD = 64 };
4969+
49594970
/* #1355: write the shared external-import-shadow fixture into `dir`.
49604971
* `pad_files` filler modules push the run over MIN_FILES_FOR_PARALLEL so the
49614972
* same tree can be indexed by both resolvers. */
@@ -5046,11 +5057,13 @@ TEST(pipeline_external_import_shadow_not_bound_to_local_homonym_issue1355) {
50465057
/* Enough files that CBM_WORKERS can take the fused-parallel path; the same
50475058
* tree is then indexed by each resolver in turn, because the guard lives at
50485059
* two independent emit sites (pass_calls.c and pass_parallel.c). */
5049-
write_external_import_shadow_fixture(tmp, 50);
5060+
write_external_import_shadow_fixture(tmp, EXTERNAL_IMPORT_SHADOW_PARALLEL_PAD);
50505061

5051-
char *old_workers = getenv("CBM_WORKERS");
5062+
/* getenv() returns a pointer into the process environment that must be
5063+
* treated as read-only; strdup() below only ever reads through it. */
5064+
const char *old_workers = getenv("CBM_WORKERS");
50525065
char *saved_workers = old_workers ? strdup(old_workers) : NULL;
5053-
char *old_single = getenv("CBM_INDEX_SINGLE_THREAD");
5066+
const char *old_single = getenv("CBM_INDEX_SINGLE_THREAD");
50545067
char *saved_single = old_single ? strdup(old_single) : NULL;
50555068

50565069
cbm_setenv("CBM_INDEX_SINGLE_THREAD", "1", 1);

‎tests/test_registry.c‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -990,6 +990,39 @@ TEST(external_import_shadow_relative_binding_wins_the_tie) {
990990
PASS();
991991
}
992992

993+
TEST(external_import_shadow_windows_relative_specifier_kept) {
994+
/* #1355 follow-up: a Windows-style specifier —
995+
* drive-letter absolute (`C:\...` / `C:/...`) or UNC share (`\\server\...`)
996+
* — names a path inside the indexed tree exactly like a POSIX "./x" or
997+
* "/x". pr-smoke runs this pipeline on Windows; before this fix these
998+
* specifiers fell through to "external package" and the same-name guess
999+
* they'd otherwise validate got suppressed on that platform only. */
1000+
CBMImport imports[] = {
1001+
{.local_name = "eq", .module_path = "C:\\repo\\src\\text-utils.ts"},
1002+
{.local_name = "sql", .module_path = "C:/repo/src/text-utils.ts"},
1003+
{.local_name = "normalize", .module_path = "\\\\server\\share\\text-utils.ts"},
1004+
};
1005+
CBMImportArray arr = {.items = imports, .count = 3, .cap = 3};
1006+
1007+
/* None of these materialized an import-map key (as if resolution missed
1008+
* them, mirroring the reported scenario) — the specifier alone must still
1009+
* keep the edge because it is in-tree, not external. RED before the fix
1010+
* (specifier_is_relative saw '.' / '/' only, so all three were classified
1011+
* external and suppressed); GREEN after. */
1012+
ASSERT_FALSE(cbm_suppress_external_import_shadow("eq", "unique_name", &arr, NULL, 0));
1013+
ASSERT_FALSE(cbm_suppress_external_import_shadow("sql", "unique_name", &arr, NULL, 0));
1014+
ASSERT_FALSE(cbm_suppress_external_import_shadow("normalize", "unique_name", &arr, NULL, 0));
1015+
/* A bare package specifier without any drive letter or leading separator
1016+
* (e.g. "drizzle-orm") must still be external — this predicate must not
1017+
* become "anything with a colon or backslash". */
1018+
CBMImport pkg_only[] = {
1019+
{.local_name = "eq", .module_path = "drizzle-orm"},
1020+
};
1021+
CBMImportArray parr = {.items = pkg_only, .count = 1, .cap = 1};
1022+
ASSERT_TRUE(cbm_suppress_external_import_shadow("eq", "unique_name", &parr, NULL, 0));
1023+
PASS();
1024+
}
1025+
9931026
/* ── Suite ─────────────────────────────────────────────────────── */
9941027

9951028
/* Method call THROUGH an imported symbol that is itself an indexed node
@@ -1089,4 +1122,5 @@ SUITE(registry) {
10891122
RUN_TEST(external_import_shadow_drops_package_bound_bare_call);
10901123
RUN_TEST(external_import_shadow_keeps_everything_else);
10911124
RUN_TEST(external_import_shadow_relative_binding_wins_the_tie);
1125+
RUN_TEST(external_import_shadow_windows_relative_specifier_kept);
10921126
}

0 commit comments

Comments
 (0)