fix(cpp): resolve singleton/factory/chained calls via return types#646
fix(cpp): resolve singleton/factory/chained calls via return types#646stabey wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb22d8f0e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (last) { | ||
| candidates = context | ||
| .getNodesByName(last) | ||
| .filter((n) => n.qualifiedName.endsWith(callee)); |
There was a problem hiding this comment.
Require a separator for scoped return lookups
When the extractor encodes a namespaced chained call as Foo::create().draw, this fallback also accepts owners whose names merely end with Foo, e.g. ns::OtherFoo::create satisfies endsWith('Foo::create'). If that method is indexed before the real ns::Foo::create and returns a different type, the new return-type path resolves the following call to the wrong class; the suffix check needs to require an exact match or a :: boundary.
Useful? React with 👍 / 👎.
| (n) => | ||
| n.kind === 'method' && | ||
| n.language === 'cpp' && | ||
| n.qualifiedName.endsWith(suffix), |
There was a problem hiding this comment.
Require a separator when matching owner types
For member-chain inference such as Manager m; m.view().render(), this suffix check also matches unrelated classes whose names end with the requested type (OtherManager::view ends with Manager::view). If such a method is indexed first and has a different return type, the newly added chained/auto resolver will infer that wrong type and misroute the final method call; this should use an exact match or a ::${suffix} boundary like resolveMethodOnType does.
Useful? React with 👍 / 👎.
…mchenry#645) Addresses Codex review on colbymchenry#646. The return-type lookups matched a qualified name with a plain `endsWith`, so `Foo::create` also accepted `OtherFoo::create` (and `Manager::view` accepted `OtherManager::view`). If the same-suffix class indexed first and returned a different type, the chained/auto call misrouted. Match at a `::` boundary instead (exact, or `::<suffix>`), mirroring resolveMethodOnType. Adds a regression test with an `OtherManager` decoy that sorts first — verified red before the fix, green after. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks @codex — both P2 findings were valid. The return-type lookups ( Fixed in e424e31: both now match at a |
|
To use Codex here, create an environment for this repo. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e424e3170d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .getNodesByName(callee) | ||
| .filter((n) => n.kind === 'function' || n.kind === 'method'); |
There was a problem hiding this comment.
Filter bare factory lookups to free functions
When callee has no ::, extraction represents an unqualified call like makeWidget()->draw() as makeWidget().draw, so this branch is resolving a bare callee name. Including every method with that name lets an unrelated class method supply the return type; if Decoy::makeWidget() is indexed before the real free function makeWidget(), cppReturnTypeOf returns Decoy and the final draw call is linked to Decoy::draw. Restrict bare lookups to free functions, or otherwise scope unqualified member calls to the caller's class before using a method signature as the receiver type.
Useful? React with 👍 / 👎.
…olbymchenry#645) C++ calls whose receiver was another call's result — `Foo::instance().bar()`, factories, free-function factories, and single-level member chains — lost the receiver's type during extraction and degraded to a bare method name. They then mis-resolved to the first-indexed same-named method, or went unresolved. - Capture C++ return types into method/function signatures so resolution has a type to recover (`extractCppSignature`, emitted as `(params) -> ReturnType`). - Encode chained-call receivers at extraction (`Recv().method`, `obj.getThing().method`) instead of dropping them to a bare name. - Resolve chained/auto calls by the callee's return type, with a self-returning-accessor-name fallback; infer `auto` locals from new/make_unique/make_shared/cast/construction/accessor/return-type; and single-level member chains via the chained method's return type. Every inferred type is validated against the graph before an edge is created, so a wrong guess falls through silently rather than producing a wrong edge. Deliberately out of scope: deep chains, multi-level member access, and overload/alias/template-correct selection (need a real type environment). Adds end-to-end + unit coverage; node count stable across re-index; full suite green (1110 passed). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…mchenry#645) Addresses Codex review on colbymchenry#646. The return-type lookups matched a qualified name with a plain `endsWith`, so `Foo::create` also accepted `OtherFoo::create` (and `Manager::view` accepted `OtherManager::view`). If the same-suffix class indexed first and returned a different type, the chained/auto call misrouted. Match at a `::` boundary instead (exact, or `::<suffix>`), mirroring resolveMethodOnType. Adds a regression test with an `OtherManager` decoy that sorts first — verified red before the fix, green after. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…olbymchenry#645) Addresses a third Codex finding on colbymchenry#646. cppReturnTypeOf's bare (unqualified) branch — used for `makeWidget()->draw()`-style chained calls — also matched same-named *methods*, so an unrelated `Decoy::makeWidget` indexed first could supply the return type and misroute the chained call. A bare unqualified call is a free function; restrict the lookup to kind 'function'. Adds a regression test with a same-named method that sorts first. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
e424e31 to
261ada4
Compare
|
Rebased onto latest The bare-callee branch of |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 261ada4177
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const klass = parts[parts.length - 2]!; | ||
| return `${klass}::${accessor}().${methodName}`; |
There was a problem hiding this comment.
Preserve namespaces for chained scoped calls
When the inner receiver call is explicitly namespaced, this keeps only the segment before the accessor, so a::Factory::create().draw() and b::Factory::create().draw() are both emitted as Factory::create().draw. In the chained-call resolver this is then suffix-matched against any ::Factory::create and the first captured return type is used, which can link the final method to the wrong class whenever the same factory class name exists in multiple namespaces. Encode the full qualifier before the accessor instead of only klass.
Useful? React with 👍 / 👎.
Fixes #645.
Problem
In C++, a method call whose receiver is another call's result lost the
receiver's type during extraction and degraded to a bare method name. When two
classes shared a method name, the call silently resolved to whichever class was
indexed first — or didn't resolve at all. This corrupted
callers,callees,impact, andtracefor the most common C++ idioms (singletons, factories,chained getters), and did so silently with a plausible-looking wrong edge.
Approach
Resolve the receiver by what the inner call returns. This required first
capturing C++ return types, which weren't extracted at all before.
(
extractCppSignature, emitted as(params) -> ReturnType; base type from thetypefield, smart-pointer template arg preserved). Encode chained-callreceivers as a resolvable reference (
Recv().method,obj.getThing().method)instead of dropping them to a bare name.
matchCppChainedAccessorresolves those by the callee'sreturn type, with a self-returning-accessor-name fallback when the return
type isn't indexed (e.g. an external accessor).
autolocals are inferred fromnew/std::make_unique/std::make_shared/ casts / direct construction /named accessors / the initializer call's return type. Single-level member
chains resolve the object's type, then the chained method's return type.
Covered
Foo::instance().bar(),Foo::getInstance()->bar()(any accessor name, not justinstance).WidgetFactory::create().draw()resolves on
Widget, notWidgetFactory.openSession()->run().autolocal first, plusnew/std::make_unique/std::make_shared/ casts / direct construction.manager.view().render().Deliberately out of scope
Need a real type environment (symbol tables + overload resolution by argument
types), not a heuristic — left uncovered (silent, not wrong):
a().b().c().h.mgr.view().render().typedef/usingalias resolution, templatedreturn types, inherited methods.
Safety
Every inferred type is validated against the graph (the class must actually have
the method) before an edge is created, so a wrong guess falls through silently
rather than producing a wrong edge.
Testing
frameworks-integration.test.tsfor the singleton,factory-returns-other-class, oddly-named-accessor, free-function-factory, and
member-chain cases — each with a same-named decoy class that sorts first, so a
green test proves resolution was driven by type, not indexing order.
resolution.test.tsforinferCppTypeFromInitializerandparseCppReturnType(tier boundaries, smart-pointer unwrap, primitive/voidrejection).
🤖 Generated with Claude Code