fix(extract): receiver-aware self-recursion detection
The recursion detector flagged any call whose short name matched the enclosing def as self-recursion, so super().save() inside save, axios.get() inside get, and console.error() inside error were all false positives feeding is_recursive, recursion_in_loop and unguarded_recursion. Add is_self_receiver() and AND it into the short-name comparison: a qualified callee only counts when its whole receiver chain (everything before the last '.') names the same object — self/this/cls/@self, or the enclosing def's own receiver identifier parsed from CBMDefinition.receiver (Go: s in 'func (s *Store) save()'). Bare names keep the prior behavior. Matching the full chain keeps self.obj.recur() (a field's same-named method) out, and the dynamic receiver whitelist keeps Go s.save() detected. Distilled from PR #699 with two corrections: the receiver chain is matched via the last dot instead of the first segment (self.obj.recur was still a false positive), and the enclosing-receiver whitelist so Go method self-recursion is not lost. Closes #599 Co-authored-by: Gen Li <lg320531124@users.noreply.github.com> Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
This commit is contained in:
+77
-2
@@ -439,6 +439,77 @@ static bool is_alloc_name(const char *n) {
|
||||
return name_in_set(n, set);
|
||||
}
|
||||
|
||||
// Extract the receiver identifier from a def's receiver text — Go's
|
||||
// "(s *Store)" / "(s Store)" → "s". Stores the identifier start in *out and
|
||||
// returns its length; returns 0 for unnamed receivers ("(*Store)", "(Store)"),
|
||||
// where no second token follows the identifier (a lone token is the TYPE, not
|
||||
// a name — such methods have no receiver variable to call through anyway).
|
||||
static size_t receiver_ident(const char *recv_text, const char **out) {
|
||||
const char *p = recv_text;
|
||||
if (*p == '(') {
|
||||
p++;
|
||||
}
|
||||
while (*p == ' ' || *p == '\t') {
|
||||
p++;
|
||||
}
|
||||
const char *start = p;
|
||||
while ((*p >= 'a' && *p <= 'z') || (*p >= 'A' && *p <= 'Z') || (*p >= '0' && *p <= '9') ||
|
||||
*p == '_') {
|
||||
p++;
|
||||
}
|
||||
size_t len = (size_t)(p - start);
|
||||
if (len == 0) {
|
||||
return 0; // "(*Store)": leading '*', no identifier
|
||||
}
|
||||
while (*p == ' ' || *p == '\t') {
|
||||
p++;
|
||||
}
|
||||
if (*p == ')' || *p == '\0') {
|
||||
return 0; // "(Store)": single token is the type, receiver unnamed
|
||||
}
|
||||
*out = start;
|
||||
return len;
|
||||
}
|
||||
|
||||
// Whether a callee expression targets the same instance/class as the enclosing
|
||||
// def, i.e. counts as genuine self-recursion rather than a same-named call on a
|
||||
// different receiver. callee_name may be bare ("recur") or qualified
|
||||
// ("self.recur", "this.recur", "super().save", "axios.get", "self.obj.recur").
|
||||
//
|
||||
// Bare names have no receiver → assume self-call (free function calling itself
|
||||
// by bare name; preserves prior behavior). Qualified names: the receiver chain
|
||||
// is everything before the LAST '.', and the WHOLE chain must name the same
|
||||
// object — self/this/cls/@self, or the enclosing def's own receiver identifier
|
||||
// (Go: `s` in `func (s *Store) save()`, from CBMDefinition.receiver). Matching
|
||||
// the whole chain (not its first segment) keeps self.obj.recur() out: it
|
||||
// targets self's FIELD obj, a different object. super() is the parent class and
|
||||
// any other receiver (axios, console, ...) a different target. See #599.
|
||||
static bool is_self_receiver(const char *callee_name, const char *def_receiver) {
|
||||
if (!callee_name || !callee_name[0]) {
|
||||
return false;
|
||||
}
|
||||
const char *dot = strrchr(callee_name, '.');
|
||||
if (!dot) {
|
||||
return true; // bare name → self-recursion candidate
|
||||
}
|
||||
size_t rlen = (size_t)(dot - callee_name);
|
||||
static const char *const self_receivers[] = {"self", "this", "cls", "@self", NULL};
|
||||
for (int i = 0; self_receivers[i]; i++) {
|
||||
size_t sl = strlen(self_receivers[i]);
|
||||
if (rlen == sl && strncmp(callee_name, self_receivers[i], sl) == 0) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
if (def_receiver) {
|
||||
const char *rid = NULL;
|
||||
size_t ril = receiver_ident(def_receiver, &rid);
|
||||
if (ril > 0 && ril == rlen && strncmp(callee_name, rid, ril) == 0) {
|
||||
return true; // call through the enclosing method's own receiver
|
||||
}
|
||||
}
|
||||
return false; // super() / axios / console / self.obj / any other receiver
|
||||
}
|
||||
|
||||
// Count parameters from a signature string like "(int a, Foo* b, cb (*)(int,int))".
|
||||
// Fallback for languages where param_names isn't populated (e.g. C keeps only the
|
||||
// signature text). Counts commas at the top paren level; treats "()"/"(void)" as 0.
|
||||
@@ -772,12 +843,16 @@ CBMFileResult *cbm_extract_file(const char *source, int source_len, CBMLanguage
|
||||
continue;
|
||||
}
|
||||
CBMDefinition *d = &result->defs.items[best];
|
||||
// callee_name may be bare ("recur") or qualified ("pkg.recur", "self.recur")
|
||||
// callee_name may be bare ("recur") or qualified ("self.recur",
|
||||
// "super().save", "axios.get"). A short-name match alone is not
|
||||
// self-recursion: the callee must also target the same object
|
||||
// (is_self_receiver), or super().save() inside save and axios.get
|
||||
// inside get are false positives (#599).
|
||||
const char *dot = strrchr(c->callee_name, '.');
|
||||
const char *callee_short = dot ? dot + 1 : c->callee_name;
|
||||
bool in_loop = c->loop_depth > 0;
|
||||
|
||||
if (strcmp(callee_short, d->name) == 0) {
|
||||
if (strcmp(callee_short, d->name) == 0 && is_self_receiver(c->callee_name, d->receiver)) {
|
||||
// Direct self-recursion. The call graph omits self-edges (pass_calls
|
||||
// skips source==target), so detect it here; seeds "recursive".
|
||||
d->is_recursive = true;
|
||||
|
||||
@@ -2987,6 +2987,132 @@ TEST(complexity_guarded_recursion) {
|
||||
PASS();
|
||||
}
|
||||
|
||||
/* #599: super().save() inside a method named save is a parent-class call —
|
||||
* the receiver is super(), never self — so it must NOT flag self-recursion.
|
||||
* super()-ONLY fixture: no self.save() alongside, so the assertion cannot pass
|
||||
* vacuously off a genuine self-call. */
|
||||
TEST(complexity_super_only_not_recursive) {
|
||||
CBMFileResult *r = extract("class B(A):\n"
|
||||
" def save(self):\n"
|
||||
" super().save()\n",
|
||||
CBM_LANG_PYTHON, "t", "super_only.py");
|
||||
ASSERT_NOT_NULL(r);
|
||||
ASSERT_FALSE(r->has_error);
|
||||
const CBMDefinition *d = find_def(r, "save");
|
||||
ASSERT_NOT_NULL(d);
|
||||
ASSERT_FALSE(d->is_recursive); /* parent-class call, not self-recursion */
|
||||
ASSERT_FALSE(d->unguarded_recursion);
|
||||
cbm_free_result(r);
|
||||
PASS();
|
||||
}
|
||||
|
||||
/* #599: a same-named call on an unrelated receiver (axios.get inside a
|
||||
* function also named get) is delegation, not self-recursion. */
|
||||
TEST(complexity_same_name_other_receiver_not_recursive) {
|
||||
CBMFileResult *r = extract("function get(url) {\n"
|
||||
" return axios.get(url);\n"
|
||||
"}\n",
|
||||
CBM_LANG_JAVASCRIPT, "t", "axios_get.js");
|
||||
ASSERT_NOT_NULL(r);
|
||||
ASSERT_FALSE(r->has_error);
|
||||
const CBMDefinition *d = find_def(r, "get");
|
||||
ASSERT_NOT_NULL(d);
|
||||
ASSERT_FALSE(d->is_recursive); /* axios.get targets axios, not this fn */
|
||||
ASSERT_FALSE(d->unguarded_recursion);
|
||||
cbm_free_result(r);
|
||||
PASS();
|
||||
}
|
||||
|
||||
/* Guard: genuine self-recursion through self/this receivers still trips the
|
||||
* detector after the receiver-aware narrowing (#599). */
|
||||
TEST(complexity_self_receiver_still_recursive) {
|
||||
/* Python: self.recur() — same object. */
|
||||
CBMFileResult *r = extract("class C:\n"
|
||||
" def recur(self, n):\n"
|
||||
" if n > 0:\n"
|
||||
" self.recur(n - 1)\n",
|
||||
CBM_LANG_PYTHON, "t", "self_recur.py");
|
||||
ASSERT_NOT_NULL(r);
|
||||
ASSERT_FALSE(r->has_error);
|
||||
const CBMDefinition *d = find_def(r, "recur");
|
||||
ASSERT_NOT_NULL(d);
|
||||
ASSERT_TRUE(d->is_recursive);
|
||||
ASSERT_FALSE(d->unguarded_recursion); /* guarded by `if n > 0` */
|
||||
cbm_free_result(r);
|
||||
|
||||
/* JS: this.step() — same object. */
|
||||
r = extract("class C {\n"
|
||||
" step(n) {\n"
|
||||
" if (n > 0) { this.step(n - 1); }\n"
|
||||
" }\n"
|
||||
"}\n",
|
||||
CBM_LANG_JAVASCRIPT, "t", "this_step.js");
|
||||
ASSERT_NOT_NULL(r);
|
||||
ASSERT_FALSE(r->has_error);
|
||||
d = find_def(r, "step");
|
||||
ASSERT_NOT_NULL(d);
|
||||
ASSERT_TRUE(d->is_recursive);
|
||||
ASSERT_FALSE(d->unguarded_recursion); /* guarded by `if (n > 0)` */
|
||||
cbm_free_result(r);
|
||||
PASS();
|
||||
}
|
||||
|
||||
/* #599: chained receiver — self.obj.recur() inside recur targets self's FIELD
|
||||
* obj, a different object. The whole receiver chain ("self.obj") must be
|
||||
* compared, not just its first segment ("self"). */
|
||||
TEST(complexity_chained_receiver_not_self) {
|
||||
CBMFileResult *r = extract("class C:\n"
|
||||
" def recur(self, n):\n"
|
||||
" self.obj.recur(n)\n",
|
||||
CBM_LANG_PYTHON, "t", "chained_recur.py");
|
||||
ASSERT_NOT_NULL(r);
|
||||
ASSERT_FALSE(r->has_error);
|
||||
const CBMDefinition *d = find_def(r, "recur");
|
||||
ASSERT_NOT_NULL(d);
|
||||
ASSERT_FALSE(d->is_recursive); /* self.obj is not self */
|
||||
ASSERT_FALSE(d->unguarded_recursion);
|
||||
cbm_free_result(r);
|
||||
PASS();
|
||||
}
|
||||
|
||||
/* #599: Go method receiver — the enclosing def's own receiver identifier
|
||||
* (`s` in `func (s *Store) save()`) is whitelisted dynamically from
|
||||
* CBMDefinition.receiver, so s.save() still counts as self-recursion while
|
||||
* s.backup.save() (a field's same-named method) does not. */
|
||||
TEST(complexity_go_method_receiver_self_recursion) {
|
||||
CBMFileResult *r = extract("package p\n"
|
||||
"type Store struct{}\n"
|
||||
"func (s *Store) save(n int) {\n"
|
||||
" if n > 0 {\n"
|
||||
" s.save(n - 1)\n"
|
||||
" }\n"
|
||||
"}\n",
|
||||
CBM_LANG_GO, "t", "store.go");
|
||||
ASSERT_NOT_NULL(r);
|
||||
ASSERT_FALSE(r->has_error);
|
||||
const CBMDefinition *d = find_def(r, "save");
|
||||
ASSERT_NOT_NULL(d);
|
||||
ASSERT_TRUE(d->is_recursive); /* s.save() == receiver s → self */
|
||||
ASSERT_FALSE(d->unguarded_recursion); /* guarded by `if n > 0` */
|
||||
cbm_free_result(r);
|
||||
|
||||
/* Same-named method on a field of the receiver: NOT self-recursion. */
|
||||
r = extract("package p\n"
|
||||
"type Store struct{ backup *Store }\n"
|
||||
"func (s *Store) save(n int) {\n"
|
||||
" s.backup.save(n)\n"
|
||||
"}\n",
|
||||
CBM_LANG_GO, "t", "store_backup.go");
|
||||
ASSERT_NOT_NULL(r);
|
||||
ASSERT_FALSE(r->has_error);
|
||||
d = find_def(r, "save");
|
||||
ASSERT_NOT_NULL(d);
|
||||
ASSERT_FALSE(d->is_recursive); /* s.backup is not s */
|
||||
ASSERT_FALSE(d->unguarded_recursion);
|
||||
cbm_free_result(r);
|
||||
PASS();
|
||||
}
|
||||
|
||||
/* Deep chained member access + parameter count structure smells. */
|
||||
TEST(complexity_access_depth_and_params) {
|
||||
CBMFileResult *r = extract("package p\n"
|
||||
@@ -3380,6 +3506,11 @@ SUITE(extraction) {
|
||||
RUN_TEST(complexity_linear_scan_in_loop);
|
||||
RUN_TEST(complexity_recursion_in_loop_unguarded);
|
||||
RUN_TEST(complexity_guarded_recursion);
|
||||
RUN_TEST(complexity_super_only_not_recursive);
|
||||
RUN_TEST(complexity_same_name_other_receiver_not_recursive);
|
||||
RUN_TEST(complexity_self_receiver_still_recursive);
|
||||
RUN_TEST(complexity_chained_receiver_not_self);
|
||||
RUN_TEST(complexity_go_method_receiver_self_recursion);
|
||||
RUN_TEST(complexity_access_depth_and_params);
|
||||
RUN_TEST(walk_defs_no_truncation_over_4096_issue668);
|
||||
|
||||
|
||||
Reference in New Issue
Block a user