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 6 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
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.
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.
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.
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>
stabey
force-pushed
the
fix/cpp-receiver-type-resolution
branch
from
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.
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 👍 / 👎.
) Addresses a fourth Codex finding on colbymchenry#646. A chained scoped call kept only the segment before the accessor, so `a::Factory::create().draw()` and `b::Factory::create().draw()` both collapsed to `Factory::create` and the first captured return type won — misrouting when the same class name exists in multiple namespaces. - Encode the FULL qualifier (`a::Factory::create().draw`) so the resolver can tell the namespaces apart by return type. - cppReturnTypeOf matches most-specific first (exact qn → full-qualifier suffix → Class::accessor) and bails when matches disagree on the return type (uniqueCppReturnType) — silent beats a wrong edge. - cppReturnTypeOfMethodOnType bails on the same ambiguity; the accessor-name fallback keeps the namespace in the owner type. Adds a regression test resolving a Factory class shared across two namespaces. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Addressed the namespace finding (4th Codex comment). The chained scoped-call encoding kept only the segment before the accessor, so |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a6fc078a2
ℹ️ 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 inner = lastCppSegment(smart[1]!); | ||
| return inner && !CPP_PRIMITIVE_TYPES.has(inner) ? inner : null; | ||
| } | ||
| const base = normalizeCppTypeName(raw); | ||
| if (!base || CPP_PRIMITIVE_TYPES.has(base)) return null; |
There was a problem hiding this comment.
When a scoped factory returns a namespaced type whose basename is shared, this normalization drops the namespace before resolveMethodOnType runs. For example, a::Factory::create() returning a::Widget and b::Factory::create() returning b::Widget both become Widget, so a::Factory::create().draw() can be linked to whichever ::Widget::draw was indexed first rather than the a::Widget method; keep the qualified return type through the chained-call resolver or otherwise use it to disambiguate the target class.
Useful? React with 👍 / 👎.
…ces (colbymchenry#645) Addresses a fifth Codex finding on colbymchenry#646. The return type is normalized to a bare basename, so `a::Factory::create()` returning `a::Widget` and another `b::Widget` collapse to `Widget`, and the final method could link to whichever `::Widget::draw` was indexed first. The return-type-driven chained resolutions now go through resolveUniqueMethodOnType, which bails when the type's basename has more than one owner across namespaces (overloads on the same owner still resolve) — silent beats a wrong edge. Adds a regression test (verified red before the guard). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Addressed the 5th finding (ambiguous return-type basename across namespaces). The return type is normalized to a bare basename, so |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4dd64080ae
ℹ️ 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 paramText = params ? getNodeText(params, source) : '()'; | ||
| if (typeField) { | ||
| return `${paramText} -> ${getNodeText(typeField, source).trim()}`; |
There was a problem hiding this comment.
auto
When a C++ factory/accessor is written with a trailing return type, e.g. auto Factory::create() -> Widget, the function type field is just auto and the real Widget return type lives on the function declarator's trailing return node. Storing the signature as () -> auto makes parseCppReturnType reject it, so the new return-type resolver cannot resolve Factory::create().draw() or auto w = Factory::create(); w.draw() even though the return type is syntactically available.
Useful? React with 👍 / 👎.
| */ | ||
| function normalizeCppReturnType(raw: string): string | null { | ||
| if (!raw) return null; | ||
| const smart = raw.match(/\b(?:shared_ptr|unique_ptr|weak_ptr|auto_ptr)\s*<\s*([A-Za-z_][\w:]*)/); |
There was a problem hiding this comment.
For smart-pointer return types with a leading cv-qualified pointee such as std::shared_ptr<const Widget>, this regex captures const as the pointee name instead of Widget. The chained-call path then looks for methods on a type named const and leaves calls like Factory::create()->draw() unresolved, despite the new resolver explicitly supporting smart-pointer factory returns.
Useful? React with 👍 / 👎.
… pointees (colbymchenry#645) Addresses two Codex findings on colbymchenry#646, both common modern-C++ syntax: - Trailing return types: `auto Factory::create() -> Widget` has `auto` as the type field and the real type on the declarator's trailing-return node. extractCppSignature now reads that, so `Factory::create().draw()` and the `auto` forms resolve instead of seeing `() -> auto`. - cv-qualified smart-pointer pointees: `std::shared_ptr<const Widget>` matched `const` as the type. The smart-pointer/make_*/cast patterns now skip a leading const/volatile and capture `Widget`. Adds unit + end-to-end regression tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Addressed both findings from the latest review — both common modern-C++ syntax, so good catches:
Added unit tests ( |
1 participant
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