fix(resolve): prefer a base CLASS for Python implements edges (#172)#180
fix(resolve): prefer a base CLASS for Python implements edges (#172)#180skakri wants to merge 1 commit into
Conversation
Python base-class inheritance emits an `implements` edge to the base, which is a CLASS (Python has no traits/interfaces). The resolver's `implements` preference was `[trait, interface]`, so a Python base never matched the preference and could mis-resolve to a same-named non-class or fall back ambiguously. `preferred_matches` is now language-aware: for Python, `implements` prefers `[class, object]`; Kotlin/TS/Rust are UNCHANGED (`implements`/`: I` targets an interface, and a same-named class must not be preferred over it). Added a resolve test (the base class is preferred over a decoy same-named function).
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
SCIP oracle — resolution reportHeuristic→compiler edge resolution per corpus. Δ compares resolved-after to the
resolved = |
![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: acd8d90f99
ℹ️ 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".
| // traits/interfaces — so prefer class-like kinds for Python (#172). Kept language-scoped so | ||
| // Kotlin/TS are UNCHANGED: there `implements`/`: I` targets an interface, and a same-named | ||
| // class must not be preferred over it. | ||
| "implements" if source_language == Some(Language::Python.as_str()) => &["class", "object"], |
There was a problem hiding this comment.
In mixed-language indexes this branch prefers any class/object symbol by kind only; preferred_matches does not check symbol.language, and the name buckets are repo-wide. When a Python base is external or otherwise not indexed as a Python class, but a TS/Kotlin/C++ class or Kotlin object with the same short name exists alongside another same-named non-class candidate, this new preferred set can resolve class Sub(Base) to the foreign symbol instead of leaving it unresolved. Please restrict the Python implements preference to Python symbols (or otherwise suppress cross-language candidates) when applying this branch.
Useful? React with 👍 / 👎.
| // traits/interfaces — so prefer class-like kinds for Python (#172). Kept language-scoped so | ||
| // Kotlin/TS are UNCHANGED: there `implements`/`: I` targets an interface, and a same-named | ||
| // class must not be preferred over it. | ||
| "implements" if source_language == Some(Language::Python.as_str()) => &["class", "object"], |
There was a problem hiding this comment.
This preference also applies to every Python implements edge even when the superclass expression is a call, because python_edges emits implements for each positional superclass and python_type_head leaves a call expression to last_identifier_text (for example class Sub(Base()):). In that scenario, if the repo has both def Base(...) and a decoy class Base, the new preferred set resolves the inheritance edge to the class, whereas the actual base expression depends on the callable and the old resolver would leave the class/function collision ambiguous. Please avoid applying the class preference to call-shaped bases, or encode the expression shape so these dynamic bases do not bind to an unrelated class.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: acd8d90f99
ℹ️ 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".
| // traits/interfaces — so prefer class-like kinds for Python (#172). Kept language-scoped so | ||
| // Kotlin/TS are UNCHANGED: there `implements`/`: I` targets an interface, and a same-named | ||
| // class must not be preferred over it. | ||
| "implements" if source_language == Some(Language::Python.as_str()) => &["class", "object"], |
There was a problem hiding this comment.
In mixed-language indexes this branch prefers any class/object symbol by kind only; preferred_matches does not check symbol.language, and the name buckets are repo-wide. When a Python base is external or otherwise not indexed as a Python class, but a TS/Kotlin/C++ class or Kotlin object with the same short name exists alongside another same-named non-class candidate, this new preferred set can resolve class Sub(Base) to the foreign symbol instead of leaving it unresolved. Please restrict the Python implements preference to Python symbols (or otherwise suppress cross-language candidates) when applying this branch.
Useful? React with 👍 / 👎.
| // traits/interfaces — so prefer class-like kinds for Python (#172). Kept language-scoped so | ||
| // Kotlin/TS are UNCHANGED: there `implements`/`: I` targets an interface, and a same-named | ||
| // class must not be preferred over it. | ||
| "implements" if source_language == Some(Language::Python.as_str()) => &["class", "object"], |
There was a problem hiding this comment.
This preference also applies to every Python implements edge even when the superclass expression is a call, because python_edges emits implements for each positional superclass and python_type_head leaves a call expression to last_identifier_text (for example class Sub(Base()):). In that scenario, if the repo has both def Base(...) and a decoy class Base, the new preferred set resolves the inheritance edge to the class, whereas the actual base expression depends on the callable and the old resolver would leave the class/function collision ambiguous. Please avoid applying the class preference to call-shaped bases, or encode the expression shape so these dynamic bases do not bind to an unrelated class.
Useful? React with 👍 / 👎.
|
Bencher Report| Branch | fix/python-implements-pref |
| Testbed | ubuntu-latest |
⚠️ WARNING: No Threshold found!Without a Threshold, no Alerts will ever be generated.
- Instructions (instructions)
- RAM Hits (hits)
- L1 Hits (hits)
- Estimated Cycles (cycles)
- Total read+write (reads/writes)
- LL Hits (hits)
Click here to create a new Threshold
For more information, see the Threshold documentation.
To only post results if a Threshold exists, set the--ci-only-thresholdsflag.
Click to view all benchmark results
| Benchmark | Estimated Cycles | cycles x 1e6 | Instructions | instructions x 1e6 | L1 Hits | hits x 1e6 | LL Hits | hits x 1e6 | RAM Hits | hits x 1e3 | Total read+write | reads/writes x 1e6 |
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| rag_pipeline::pipeline::index cargo_resolver:resolver_config() | 📈 view plot | 1,214.64 x 1e6 | 📈 view plot | 801.18 x 1e6 | 📈 view plot | 1,116.67 x 1e6 | 📈 view plot | 17.42 x 1e6 | 📈 view plot | 310.87 x 1e3 | 📈 view plot | 1,134.40 x 1e6 |
| rag_pipeline::pipeline::query_cold cargo_resolver:resolver_built_config() | 📈 view plot | 241.14 x 1e6 | 📈 view plot | 154.71 x 1e6 | 📈 view plot | 231.07 x 1e6 | 📈 view plot | 1.94 x 1e6 | 📈 view plot | 10.83 x 1e3 | 📈 view plot | 233.02 x 1e6 |
| rag_pipeline::pipeline::query_warm cargo_resolver:resolver_index() | 📈 view plot | 232.81 x 1e6 | 📈 view plot | 149.12 x 1e6 | 📈 view plot | 223.16 x 1e6 | 📈 view plot | 1.86 x 1e6 | 📈 view plot | 10.10 x 1e3 | 📈 view plot | 225.03 x 1e6 |
|
| Branch | fix/python-implements-pref |
| Testbed | ubuntu-latest |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result seconds (s) (Result Δ%) | Upper Boundary seconds (s) (Limit %) |
|---|---|---|---|
| index_time/full_rebuild_cargo | 📈 view plot 🚷 view threshold | 5.03 s(+34.17%)Baseline: 3.75 s | 5.79 s (86.91%) |
1 participant
Fixes #172. Python base-class inheritance (
class Sub(Base)) emits animplementsedge toBase, which is a class — Python has no traits/interfaces. The resolver'simplementspreference was["trait", "interface"], so a Python base never matched and the edge could mis-resolve to a same-named non-class or fall back ambiguously.Fix
preferred_matchesis now language-aware: for Python,implementsprefers["class", "object"]; Kotlin / TS / Rust are unchanged (["trait", "interface"]) — thereimplements/: Itargets an interface, and a same-named class must not be preferred over it. The source language is already threaded intoresolve_symbol(request.source_language), so this is a one-line branch in the existing preference table.Verification
python_implements_prefers_a_base_class_over_a_non_class: with a baseclass Baseand a decoy same-named module-levelfunction Base, the Pythonimplementsedge binds to the class.class Sub(Base)resolvesimplements → base.py::Base (class)at Syntactic confidence (was unresolved/ambiguous before).rag-rat-coresuite (482) green; fmt + clippy clean. Kotlin/TSimplementstests unaffected (the branch is gated to Python).Sibling of #174 (alias resolution, PR #179) — both Python resolver refinements deferred from #167. The remaining
import x as mmodule-alias case is still out of scope (noted in #174).