Repository navigation
cl: generate logical classes for MethodCheck with AsXxx conversion (#948) - #949
Conversation
When MethodCheck resolves a C function to a logical type (e.g. PyList_GetItem -> List) that differs from its physical receiver (Object), emit the method on a new logical class that embeds the base class, and generate an As<Class> conversion method on the base (e.g. (*Object).AsList() *List). The conversion only reinterprets the pointer. Logical classes are generated lazily - only when at least one function resolves to them. When the logical type is the physical type itself (PyObject_IsTrue -> Object) the method stays on Object with no AsObject; functions with an empty logical type (Py_IsTrue) remain plain package functions. An existing package-level type of the same name is reused rather than regenerated, and an As<Class> that collides with an existing base method reports a diagnostic. tool test: TestPython now covers List, Dict (PyDict_GetItem collision case), and the Object/package-function cases. Fixes #948
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #949 +/- ##
==========================================
- Coverage 88.02% 87.95% -0.07%
==========================================
Files 22 23 +1
Lines 2622 2665 +43
==========================================
+ Hits 2308 2344 +36
- Misses 314 321 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: logical classes (issue #948)
This is a clean, well-documented addition. The lazy generation is correctly guarded (cls != "" && cls != typName), the logicals map is properly initialized, emission order is deterministic (driven by function compile order, not map iteration), and the golden output matches the described behavior (PyObject_IsTrue correctly stays on Object with no AsObject). The package-level doc comment explaining the why (method-name conflicts on the base) is excellent.
A few findings below, mostly around the doc/behavior mismatch on the collision path and two latent edge cases on the type-reuse path. None are blocking.
| // As<Class> conversion; genAsMethod itself keeps any pre-existing method of | ||
| // that name and reports a diagnostic. |
There was a problem hiding this comment.
[P2] Doc comment calls a hard compilation error a soft "diagnostic"
The comment says "genAsMethod itself keeps any pre-existing method of that name and reports a diagnostic" (also lines 66-67 and 111-112). In practice genAsMethod calls p.errorf(...) on a name collision, which increments p.errCnt; compile.go:287 then turns any errCnt > 0 into fmt.Errorf("compilation failed with %d errors", ...), and under FailFast checkFailFast can os.Exit(1). So the outcome is a compilation error that fails generation (the As<Class> method is simply skipped), not a tolerant warning. Only the narrow "existing method is kept" sub-claim is true. Suggest rewording to state the collision is reported as a compilation error and the conversion is skipped.
| // headers or provided through TypeAlias), it is reused instead of generating a | ||
| // second one, and the As<Class> conversion is still added when it is missing. | ||
| func (p *pkgCtx) logicalClassOf(decl clang.Cursor, goName string, base *types.Named) *types.Named { | ||
| if lc, ok := p.logicals[goName]; ok { |
There was a problem hiding this comment.
[P2] Logical-class cache ignores base; differing receiver silently absorbed
logicalClassOf caches by goName only (p.logicals[goName]). If two C functions resolve via MethodCheck to the same logical class name but have different physical receiver types, the second reuses the first's logical class (which embeds the first function's base) with no diagnostic. At the call site in func.go the receiver is then unconditionally rewritten to that cached logical type, so the emitted method would embed/convert through the wrong base relative to its actual C receiver. Latent given typical MethodCheck patterns, but there is no guard — consider keying on (goName, base) or reporting a diagnostic when p.logicals[goName].base != base.
| // existing package-level type when one is present and otherwise emitting a fresh | ||
| // "type <goName> struct { <base> }". | ||
| func (p *pkgCtx) newLogicalType(decl clang.Cursor, goName string, base *types.Named) *types.Named { | ||
| if o := p.pkg.Types.Scope().Lookup(goName); o != nil { |
There was a problem hiding this comment.
[P2] Reused pre-existing type not verified to embed base at offset 0
newLogicalType reuses any package-level type found by Scope().Lookup(goName) (e.g. one from headers or TypeAlias) without checking it is a struct whose first field is the embedded base. genAsMethod then unconditionally emits return (*List)(unsafe.Pointer(o)). That reinterpret cast is only memory-safe when the reused type actually begins with the base layout; the freshly generated path guarantees this (single embedded field at offset 0), but the reuse path does not. A same-named type with a different layout would produce a silently incorrect, memory-unsafe conversion. Recommend validating the reused type's first field is base (skip/diagnose otherwise) before adding the conversion, and documenting the layout assumption.
| sig := types.NewSignatureType(recv, nil, nil, nil, results, false) | ||
|
|
||
| f, err := pkg.NewFuncWith(goNodePos(p, decl), name, sig, nil) | ||
| if err != nil { |
There was a problem hiding this comment.
[P3] genAsMethod aborts whole run via panicf instead of errorf
On a NewFuncWith error, genAsMethod calls p.panicf (→ log.Panicf), tearing down the entire generation run. Note findMember only checks the base's own methods/direct fields, not names promoted through the base's embedded types, so a promoted-name collision would fall through to NewFuncWith and hit this panic rather than the graceful errorf path above it. For consistency with the rest of the file's error handling, consider errorf + return so one malformed/colliding symbol doesn't abort the whole package.
1 participant
MethodCheck— Logical-Type Method Detection in llcppg #945Requested by @xushiwei
Fixes #948.
MethodCheckcan tell which logical type a C function belongs to (e.g.PyList_GetItembelongs toList), but every such method was still attached to the physical base classObject, polluting the base API and inviting name conflicts (PyList_GetItemandPyDict_GetItemboth wanted to beItem).What changed
cl/logical.go(new): logical-class machinery.logicalClassOflazily emitstype <Class> struct { <Base> }and anAs<Class>conversion method on the base class (func (o *Object) AsList() *List { return (*List)(unsafe.Pointer(o)) }). It reuses an existing package-level type of the same name, and reports a diagnostic ifAs<Class>collides with an existing base method.cl/func.go: when a C global function resolves (viaMethodCheck) to a logical class distinct from its physical receiver type, the method is emitted on the logical class instead of the base.cl/ctx.go/cl/compile.go: added and initialized thelogicalsmap onpkgCtx.tool/_testc/python-3.14.8: addedPyDict_GetItemto the header (name-collision case) and updated the expectedpythread.go.Behavior (matches the acceptance criteria)
PyList_GetItem→(*List).Item;ObjectgetsAsList()only becauseListhas a method.PyDict_GetItem→(*Dict).Item+(*Object).AsDict()— no conflict withList.Item.PyObject_IsTruestays(*Object).IsTruewith noAsObject;Py_IsTruestays a package functionIsTrue.Verification
go build ./cl/andgo vet ./cl/... ./tool/...type-check clean. The expectedpythread.gois gofmt-canonical.go test) link against libclang/llgo, which isn't available in this sandbox, so the end-to-endTestPythonrun (darwin-only in the harness) was not executed here. The generator logic and the updated golden file were verified by code analysis against gogen's emission/formatting behavior.