ast, cgen: fix receiver methods on embedded interfaces (fix #19550) - #27476
ast, cgen: fix receiver methods on embedded interfaces (fix #19550)#27476Macho0x wants to merge 30 commits into
Conversation
- Only rewrite self-referential parameters for interface method declarations in new_method_with_receiver_type; keep concrete receiver method parameters unchanged. - Generate an interface-to-interface conversion for the receiver when calling a method inherited from an embedded interface, instead of reinterpreting the interface struct pointer. - Add regression test.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
@codex review |
![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: 439aa2bb35
ℹ️ 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".
| embed_value_type) | ||
| g.writeln(';') | ||
| g.write(stmt_str) | ||
| g.write('&${tmp}') |
There was a problem hiding this comment.
When this embedded-interface method call is on a non-lvalue receiver (for example make_element().append_child(...)) or an interface smartcast, the receiver setup above has already emitted an ADDR(rec_cc_type, ... wrapper instead of a literal &. This branch only strips &, then writes &tmp, so the later cast_n close produces C like ADDR(EmbeddedIface, &tmp), wrapping a pointer where the macro expects an interface value and causing otherwise valid calls to fail to compile.
Useful? React with 👍 / 👎.
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 439aa2bb35
ℹ️ 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".
| // (no_body == true). For concrete receiver methods defined outside the interface, | ||
| // the parameters keep their original types, so methods like | ||
| // `fn (mut n Node) add(child &Node)` stay valid when Node is embedded. | ||
| if f.no_body { |
There was a problem hiding this comment.
When an external interface receiver method is loaded from a generated .vh under -usecache/build-module, it also has no_body == true (the Fn.no_body field explicitly covers .vh declarations). Consequently, this branch still rewrites a concrete method parameter such as child &Node to &Element, recreating the argument-type failure this commit fixes for source builds. Distinguish methods declared inside an interface from bodyless declarations of concrete receiver methods rather than using no_body alone.
Useful? React with 👍 / 👎.
|
Addressed both review findings in 61d8015:
Validation: targeted interface tests pass; checker tests pass; compiler error suite passes (1,582 passed, 1 skipped). The C-output suite was attempted but this environment lacks |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61d8015a8a
ℹ️ 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".
| // Mutating receiver: create a temporary converted interface value, | ||
| // whose field pointers refer to the same underlying object, and pass | ||
| // its address. Modifications through those pointers are visible. | ||
| stmt_str := g.go_before_last_stmt() |
There was a problem hiding this comment.
When a mutable embedded-interface method call appears in a lazy context, such as false && make_element().check(), this go_before_last_stmt() hoists evaluation of make_element() ahead of the entire statement. need_tmp_var_in_expr does not classify these calls as requiring the short-circuit-safe path, so receiver side effects occur even when the RHS should be skipped; similarly, a receiver expression in a loop condition is converted only once rather than on every condition evaluation. Keep the conversion at the call's evaluation point or integrate this case with the existing lazy-expression temporary handling.
Useful? React with 👍 / 👎.
|
Addressed the lazy-evaluation review finding in 8c7ef0b:
Validation: targeted interface and |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c7ef0b42d
ℹ️ 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".
| g.write('ADDR(${g.styp(embed_value_type)}, ') | ||
| g.gen_interface_to_interface_conversion(ast.Expr(node.left), left_type, | ||
| embed_value_type) |
There was a problem hiding this comment.
For a mutable extension method that replaces its receiver, such as fn (mut n Node) replace(next Node) { n = next }, calling it through an Element that embeds Node now passes the address of a temporary converted interface. The assignment updates only that temporary, so the caller's Element remains unchanged after element.replace(next), even when next also implements Element; the conversion must propagate receiver replacement back to the original interface or reject this call rather than silently discarding a valid mutable-receiver operation.
Useful? React with 👍 / 👎.
|
Fixed the current-head review finding by rejecting whole-receiver replacement methods when called through an embedded interface, while preserving direct calls and ordinary mutable embedded-interface methods. Added a checker regression fixture. Validation: compiler_errors_test (1583 passed, 1 skipped), parser suite (8 passed, 1 skipped), checker suite, focused interface C-output fixtures (3 output + 4 must-have), and both embedded-interface receiver regression tests all pass with -gc none. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc91fa438d
ℹ️ 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".
| for child in node.children() { | ||
| if node_reassigns_ident(child, name) { | ||
| return true |
There was a problem hiding this comment.
When the receiver is replaced in a C-style loop initializer or increment, such as for node = next; false; {}, this recursive scan misses it because ast.Node.children() exposes only ForCStmt.stmts, not its init or inc. The method therefore remains marked as not reassigning its receiver, so calling it through an embedded interface is allowed and the replacement is silently applied only to the temporary converted interface. Fresh evidence beyond the prior receiver-replacement report is this unscanned loop-clause path; inspect ForCStmt.init and ForCStmt.inc explicitly.
Useful? React with 👍 / 👎.
| if is_method_from_embed && left_sym.kind == .interface && rec_sym.kind == .interface | ||
| && method.receiver_reassigned { |
There was a problem hiding this comment.
When the defining module is consumed through a generated .vh/cached module, a bodyless method declaration cannot reconstruct receiver_reassigned, so this condition is false even if the compiled method body assigns its receiver. The call is then accepted and passes the address of a converted temporary, causing the replacement to be discarded at runtime. Fresh evidence beyond the prior report is that the newly added .vh test serializes only the method signature and exercises no reassignment; the metadata must be preserved in headers or such cached calls must be rejected conservatively.
Useful? React with 👍 / 👎.
|
Addressed both dc91fa4 review findings. Receiver-reassignment scanning now covers C-style loop initializers and increments, with independent regression calls. Mutable extension methods loaded from .vh are conservatively rejected because their bodies cannot be inspected, while immutable .vh extension methods and mutable methods declared by embedded interfaces remain supported. Validation: compiler_errors_test (1583 passed, 1 skipped), parser suite (8 passed, 1 skipped), checker suite, focused interface C-output fixtures (3 output + 4 must-have), embedded receiver tests, .vh tests, and interface_embedding_test all pass with -gc none. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1133d1e55d
ℹ️ 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 node is ast.AssignStmt { | ||
| for left in node.left { | ||
| reduced := left.remove_par() | ||
| if reduced is ast.Ident && reduced.name == name { | ||
| return true |
There was a problem hiding this comment.
When a method delegates replacement to another mutable receiver method, such as fn (mut n Node) reset(next Node) { n.replace(next) } where replace assigns n = next, this direct AssignStmt scan leaves reset.receiver_reassigned false. Calling element.reset(next) through an embedded interface is therefore accepted, but cgen passes a converted temporary as the receiver and the delegated replacement updates only that temporary, leaving element unchanged. Fresh evidence beyond the prior receiver-replacement reports is this transitive mutable-call path; propagate reassignment metadata through calls or reject receiver escapes through mutable calls.
Useful? React with 👍 / 👎.
|
Fixed the 1133d1e transitive receiver-replacement finding. Mutable receiver methods now record delegated receiver calls; the checker follows that call graph (cycle-safe) and rejects embedded-interface calls only when delegation can replace the wrapper. Explicit mut-receiver escapes are conservatively rejected, and loop/branch conditions are scanned too. Added regressions for delegated method/function replacement plus a positive delegated field-mutation case. Validation: compiler_errors_test (1583 passed, 1 skipped), parser suite (8 passed, 1 skipped), checker suite, focused interface C-output fixtures (3 output + 4 must-have), embedded receiver tests, .vh tests, and interface_embedding_test all pass with -gc none. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 704f3fcc26
ℹ️ 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".
| reduced := left.remove_par() | ||
| if reduced is ast.Ident && reduced.name == name { | ||
| info.directly_reassigned = true |
There was a problem hiding this comment.
When a mutable receiver is replaced through its address, for example mut p := &node; unsafe { *p = next }, the assignment target is a PrefixExpr, so this check never sets receiver_reassigned; taking &node is not recorded as an escape either. Calling that method through an interface embedding Node is therefore accepted, but cgen passes the address of a converted temporary and the replacement is silently discarded. Fresh evidence beyond the prior receiver-replacement reports is this untracked address-alias path; conservatively mark address escapes or dereferenced assignments derived from the receiver.
Useful? React with 👍 / 👎.
|
Fixed the 704f3fc pointer-alias finding by conservatively recording any address-taking of a mutable interface receiver and rejecting its invocation through an embedded interface, where a converted temporary would otherwise hide replacement. Added an address-escape diagnostic regression. Validation: compiler_errors_test (1583 passed, 1 skipped), parser suite (8 passed, 1 skipped), checker suite, focused interface C-output fixtures (3 output + 4 must-have), embedded receiver tests, .vh tests, and interface_embedding_test all pass with -gc none. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a078e1906b
ℹ️ 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".
| ast.SpawnExpr { | ||
| scan_receiver_reassignment(node.call_expr, name, mut info) |
There was a problem hiding this comment.
When a pointer-receiver method launches a non-mutating receiver method with spawn node.read(), recursively scanning the call only adds read to receiver_method_calls; because read itself does not replace or escape its receiver, method_can_replace_receiver still considers the outer method safe. Calling that method through an embedding interface passes the spawned task a pointer to cgen's temporary converted interface, which can become dangling once the caller's block returns. Mark receiver calls launched by spawn as address escapes rather than treating them as synchronous delegation.
Useful? React with 👍 / 👎.
| if has_receiver { | ||
| info.address_taken = true |
There was a problem hiding this comment.
When a pointer receiver is stored only in a local aggregate, such as holder := Holder{node: node} followed by synchronous reads, contains_receiver_or_alias is true but the direct-alias exception above does not apply, so this unconditionally sets receiver_address_taken. An inherited call through an embedding interface is then rejected even though the pointer remains valid for the entire method invocation and never escapes; local aggregate bindings need escape tracking comparable to local pointer aliases.
Useful? React with 👍 / 👎.
|
Fixed the exact-head async-call and local-aggregate findings in 28ffcdb. Receiver tracking now distinguishes direct pointer aliases from local pointer-containing carriers, propagates carrier lifetime/escape information, and marks receiver uses launched by spawn or go as escaping. Added positive local-holder coverage and a spawned receiver diagnostic. Validation: rebuilt vnew; focused embedded-interface tests passed; compiler errors 1583 passed / 1 skipped; parser 8 passed / 1 skipped; checker 1 passed; focused interface C-output checks 7 passed. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28ffcdb1b6
ℹ️ 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 alias_idx >= 0 { | ||
| info.receiver_aliases[alias_idx] = ast.ReceiverAlias{ | ||
| ...info.receiver_aliases[alias_idx] | ||
| end_pos: left.pos().pos | ||
| } |
There was a problem hiding this comment.
Fresh evidence beyond the prior unconditional-rebinding report is a conditional rebind such as mut alias := node; if cond { alias = other }; overwrite(alias): this unconditionally closes the receiver alias while scanning the branch, even though alias still points to node when cond is false. The later helper call is therefore accepted through an embedded interface and can replace only cgen's converted temporary, leaving the outer interface unchanged; keep the original binding live across control-flow joins unless every path rebinds it.
Useful? React with 👍 / 👎.
| if !alias.is_pointer { | ||
| return false | ||
| } |
There was a problem hiding this comment.
Fresh evidence beyond the prior safe local-aggregate case is when that aggregate is subsequently mutated or passed onward: for example, mut holder := Holder{node: node}; replace_in_holder(mut holder) can let the helper assign through holder.node. The parser records holder as a non-pointer receiver carrier, but this early return prevents record_receiver_argument() from recognizing it, so an inherited pointer-receiver call is accepted and the helper replaces only the converted interface temporary. Propagate receiver containment through aggregate aliases when they are passed to code that can mutate or retain them.
Useful? React with 👍 / 👎.
|
Fixed the exact-head conditional-alias and mutable-carrier findings in cc1f5d1. Alias lifetime termination is now scope-aware, so a nested conditional rebind does not erase the receiver possibility after the control-flow join, while same-scope straight-line rebinds remain precise. Mutable/pointer arguments carrying a receiver inside a local aggregate now propagate the unsafe-call classification. Added regressions for both paths. Validation: rebuilt vnew; focused embedded-interface tests passed; compiler errors 1583 passed / 1 skipped; parser 8 passed / 1 skipped; checker 1 passed; focused interface C-output checks 7 passed. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc1f5d1a12
ℹ️ 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".
| arg_is_receiver_pointer := receiver_pointer_argument(arg.expr, receiver_name) | ||
| || aliases.any(receiver_pointer_alias_argument(arg.expr, it)) |
There was a problem hiding this comment.
When a pointer receiver is forwarded through a conditional or match expression, such as overwrite(if cond { node } else { other }, next), neither this recognizer nor the parser scan identifies the argument as containing the receiver. The inherited call through an embedded interface is therefore accepted, and when the receiver branch is selected, the helper replaces only cgen's converted temporary rather than the outer interface. Fresh evidence beyond the prior cast-argument report is that the new recognizer still has no IfExpr or MatchExpr case.
Useful? React with 👍 / 👎.
|
Fixed the current-head conditional-expression finding in 0c4d304: receiver-pointer analysis now inspects value-producing |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c4d3046b0
ℹ️ 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 info.receiver_is_ptr && node.op == .arrow | ||
| && contains_receiver_or_alias(node.right, name, info.receiver_aliases) { | ||
| info.address_taken = true |
There was a problem hiding this comment.
When a pointer-receiver method appends its receiver to an array that outlives the call—for example, building mut saved := []&Node{}, doing saved << node, and returning saved—the append is an InfixExpr with .left_shift, but this branch only marks channel sends (.arrow) as escapes. Visiting the right-hand identifier afterward does not set any flag, so the inherited call through an embedded interface is accepted and stores a pointer to cgen's temporary converted interface; the returned array then contains a dangling pointer.
Useful? React with 👍 / 👎.
|
Fixed the current-head array-append finding in dc1e413: receiver pointers appended to local arrays now make those arrays tracked carriers, while appends to caller-owned storage are immediate escapes. Added regressions for returned arrays, mutable external arrays, and safe local-only reads. Validation: rebuild passed; focused diagnostic and functional tests passed; compiler errors 1583 passed/1 skipped; checker 1 passed; parser 8 passed/1 skipped. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc1e413a9a
ℹ️ 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".
| } else if reduced is ast.PrefixExpr && reduced.op == .mul { | ||
| dereferenced := reduced.right.remove_par() | ||
| if dereferenced is ast.Ident && dereferenced.name == name { |
There was a problem hiding this comment.
When a pointer-receiver method replaces its interface through pointer indexing, for example unsafe { node[0] = next }, the assignment target is an IndexExpr, while this scan only recognizes a direct identifier or dereference prefix. The fresh path beyond direct *node replacement is pointer indexing: no receiver flag is set, so calling the method through an embedded interface is accepted and overwrites only cgen's converted temporary rather than the outer interface.
Useful? React with 👍 / 👎.
| ast.SelectorExpr { | ||
| scan_receiver_reassignment(node.or_block, name, mut info) |
There was a problem hiding this comment.
When a pointer-receiver method returns or stores a bound method value, such as return unsafe { node.read_name }, this SelectorExpr handling never marks receiver_address_taken. The fresh path beyond an explicitly captured anonymous function is the bound method closure itself: an inherited call through an embedding interface binds it to cgen's temporary converted receiver, so invoking the function after the call returns dereferences a dangling pointer.
Useful? React with 👍 / 👎.
|
Fixed both current-head findings in d3815e7. Indexed receiver replacement is now recognized (the regression uses a valid fixed-array pointer cast around the interface pointer), and bound receiver method values mark pointer receivers as escaped. Added diagnostic regressions for both. Validation: rebuild passed; focused diagnostic and interface functional tests passed; compiler errors 1583 passed/1 skipped; checker 1 passed; parser 8 passed/1 skipped. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3815e79d0
ℹ️ 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 dereferenced is ast.Ident && dereferenced.name == name { | ||
| info.directly_reassigned = true |
There was a problem hiding this comment.
When a pointer-receiver method first aliases its receiver and then assigns through that alias, such as alias := unsafe { node }; unsafe { *alias = next }, this branch only recognizes a dereferenced identifier whose name is the original receiver. Although the earlier assignment records alias in receiver_aliases, receiver_reassigned remains false, so invoking this method through an embedding interface is accepted and cgen overwrites only the converted temporary instead of the caller's outer interface. Check the dereferenced expression with is_receiver_pointer_alias as the indexed-assignment branch already does.
Useful? React with 👍 / 👎.
|
Fixed the current-head pointer-alias replacement finding in b17585e: dereference assignments now recognize tracked pointer receiver aliases, so |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b17585e802
ℹ️ 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".
| } else if aliases.any(!it.is_pointer && receiver_alias_argument(arg.expr, it)) { | ||
| if param.is_mut && arg.is_mut { | ||
| receiver_sym.methods[method_idx].receiver_passed_mut = true | ||
| } else if param.typ.is_any_kind_of_pointer() | ||
| || c.table.unaliased_type(param.typ).is_any_kind_of_pointer() { |
There was a problem hiding this comment.
When a pointer receiver is stored in a local carrier and that carrier is passed by value—for example, holder := PointerHolder{node: node}; replace_in_holder(holder) where the helper assigns through holder.node—this branch does nothing because it only marks non-pointer aliases for mutable arguments or pointer-typed parameters. Copying the carrier does not copy the pointed-to converted interface, so the helper can still overwrite or retain cgen's temporary while the embedded-interface call is accepted. Fresh evidence beyond the earlier mutable-holder case is this non-mut, value-parameter path; receiver-bearing aggregate arguments must be treated as escapes unless the callee is proven not to propagate or mutate the contained pointer.
Useful? React with 👍 / 👎.
| else { | ||
| false |
There was a problem hiding this comment.
When a local aggregate contains the pointer receiver and its field is forwarded directly, such as holder := PointerHolder{node: node}; replace_pointer_node(holder.node, next), receiver_alias_argument() returns false because it has no SelectorExpr or IndexExpr case. The initial aggregate assignment is deliberately recorded as a local carrier rather than an escape, so neither the parser nor this checker marks the later projected pointer; calling the method through an embedded interface then lets the helper replace only cgen's conversion temporary. Extend carrier recognition through projections that preserve the contained receiver pointer.
Useful? React with 👍 / 👎.
|
Fixed both current-head receiver-carrier findings in c5b230f. Carrier recognition now follows field/index projections, and by-value receiver-bearing aggregates are conservatively treated as escapes; mutable carrier parameters retain the existing mutable-call classification. Added regressions for by-value holder mutation and projected pointer forwarding. Validation: rebuild passed; focused diagnostic and interface functional tests passed; compiler errors 1583 passed/1 skipped; checker 1 passed; parser 8 passed/1 skipped. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5b230f8c2
ℹ️ 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".
| } else if c.table.cur_fn.receiver.typ.is_ptr() && (param.typ.is_any_kind_of_pointer() | ||
| || c.table.unaliased_type(param.typ).is_any_kind_of_pointer()) { | ||
| receiver_sym.methods[method_idx].receiver_address_taken = true |
There was a problem hiding this comment.
When a pointer-receiver method delegates a read to an ordinary helper, such as fn (node &Node) name() string { return read_name(node) }, this unconditionally marks the receiver address as escaping merely because the helper parameter is pointer-typed. Even when the helper body is available and only reads the value, invoking name() through an interface embedding Node is consequently rejected. Track whether the helper can retain or modify that parameter instead of treating every pointer argument as an escape.
Useful? React with 👍 / 👎.
| if info.receiver_is_ptr && node is ast.Return | ||
| && node.exprs.any(contains_receiver_or_alias(it, name, info.receiver_aliases)) { | ||
| info.address_taken = true |
There was a problem hiding this comment.
For a pointer-receiver method returning an option fallback such as return maybe_node() or { node }, this return check misses the receiver because contains_receiver_or_alias() has no CallExpr case and therefore never examines the call's or_block. The method is then allowed through an embedding interface, and the fallback returns a pointer to cgen's converted interface temporary, which can dangle after the caller's scope exits. Include or-block result statements when determining whether a returned expression contains the receiver.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5b230f8c2
ℹ️ 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 info.receiver_is_ptr && node is ast.Return | ||
| && node.exprs.any(contains_receiver_or_alias(it, name, info.receiver_aliases)) { | ||
| info.address_taken = true |
There was a problem hiding this comment.
When a pointer receiver is first stored in a local carrier and a field is returned, such as holder := PointerHolder{node: node}; return holder.node, this return check calls contains_receiver_or_alias, but its underlying expression walker has no SelectorExpr or IndexExpr case. Unlike the already reported helper-argument projection, this returned projection therefore leaves receiver_address_taken false, so a call through an embedded interface is accepted and can return a pointer to cgen's converted interface temporary, which dangles once the caller's block exits.
Useful? React with 👍 / 👎.
2 participants
This PR fixes the compiler error and incorrect runtime behavior when calling a receiver method (defined outside an interface) on an interface that embeds another interface.
Fixes #19550.
Problem
Given:
Calling element.append_child(&Node(&Text{...})) on a value of type &Element failed with:
error: cannot implement interface
Elementwith a different interface&NodeEven when the checker error was bypassed, the generated code reinterpreted the Element* interface struct as a Node*, leading to incorrect field offsets and broken runtime behavior.
Root cause
When an interface embeds another interface, new_method_with_receiver_type was rewriting self-referential parameters of concrete receiver methods to match the outer interface type. That made signatures such as fn (mut n Node) add(child &Node) appear to require &Element, which is wrong.
In cgen, calling a method inherited from an embedded interface used a raw pointer cast from the outer interface struct to the embedded interface struct. The two structs have different C layouts, so the cast read/wrote the wrong fields.
Fix
Test
Added vlib/v/tests/interfaces/interface_receiver_method_on_embedded_interface_test.v, which exercises:
Verification