Skip to content

v3: ownership fixes - #28046

Open
medvednikov wants to merge 1 commit into
masterfrom
ownership-fixes
Open

v3: ownership fixes#28046
medvednikov wants to merge 1 commit into
masterfrom
ownership-fixes

Conversation

@medvednikov

Copy link
Copy Markdown
Member

Fixes to the V3 compiler's ownership system.

  • Add ownership_type_has_clone_method to the always-built checker surface, detecting types that declare a handwritten clone method (including generic-struct methods). Ownership transform support is compiled into v even when v itself is built without ownership, so this lookup cannot live behind $if ownership.
  • Distinguish marker-only IClone receivers (no declared clone method) from types that provide a real clone, so generic clone lowering selects the correct path.
  • Track mut-pointer parameters in the flat C codegen (cur_mut_pointer_params, current_param_is_mut_pointer, selector_base_is_local_value) so explicitly pointer-typed mut x &T parameters retain pointer semantics instead of being treated as value mut params.
  • Owned array/map clone and move lowering fixes.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@medvednikov

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@medvednikov

Copy link
Copy Markdown
Member Author

Reviewed PR #28046, “v3: ownership fixes,” at head c3727a25668709fdb4bc64ce4381e81e7f78f54a. I deliberately excluded CI and build-status signals. I would request changes for two code issues.

[P1] Preserve pending siblings in the iterative annotation walk

Location: vlib/v3/types/checker.v:6067–6190

annotate_node was changed from recursive traversal to a single loop with a pending stack, but the special-case branches retained their existing return statements:

mut pending := [id]
for pending.len > 0 {
    current_id := pending.pop()
    ...
    match node.kind {
        .decl_assign {
            ...
            return
        }
        .for_in_stmt {
            ...
            return
        }
        .call {
            ...
            return
        }
        ...
    }
}

Under the old recursive implementation, those returns only completed annotation of the current child; the caller’s child loop then continued. In the new implementation, they terminate the entire annotate_node invocation and discard every sibling already stored in pending. The same problem applies to the negative-node check near the top of the loop.

For example, when annotating a block like:

fn example() {
    value := create_value()
    consume(value)
    println(value)
}

the block pushes all three statements. Processing the first .decl_assign reaches return, so the remaining calls are never visited by that traversal. Equivalent truncation can occur when the first pending child is a call, for in, function literal, special selector, or one of the other early-return cases.

The stack-loop-local exits should be continue, not return. Another safe approach is to move the special-case processing into a one-node helper whose return naturally corresponds to the old recursive invocation.

[P2] Do not bypass expected-type conversion for ordinary mut &T arguments

Location: vlib/v3/gen/c/fn.v:12492–12502

This new fast path emits the pointer stored in a mutable pointer slot and immediately skips the rest of argument generation:

if arg_node.kind == .ident && !arg_node.is_mut
    && g.current_param_is_mut_pointer(arg_node.value) {
    g.write('*')
    g.gen_expr(arg_id)
    continue
}

The slot dereference itself is necessary—mut p &T now has T** storage—but the unconditional continue occurs before expected-parameter conversions later in gen_call_args. It therefore bypasses optional/result wrapping, interface boxing, sum conversion, pointer/value adaptation, embedded-interface handling, and ultimately gen_expr_with_expected_type.

A pointer-backed interface argument demonstrates the problem:

interface Reader {
    read() int
}

struct Item {}

fn (_ &Item) read() int {
    return 1
}

fn consume(reader Reader) int {
    return reader.read()
}

fn forward(mut item &Item) int {
    return consume(item)
}

Inside forward, the new path emits *item, producing the semantic Item*, but then skips the normal conversion that boxes that concrete pointer as a Reader. The existing expected-type generator explicitly handles concrete-pointer-to-interface boxing, so bypassing it can leave the generated call with the wrong C representation. The parser marks is_mut only for explicit mut argument syntax, meaning ordinary consume(item) does take this path.

The slot read should feed into the ordinary expected-type conversion pipeline rather than terminate argument handling. At minimum, the raw fast path must be restricted to cases where the expected parameter is directly pointer-compatible; a more complete fix would expose *slot as the semantic expression and still run the standard conversion logic.

Verdict

Request changes. The first finding can truncate the general type-annotation traversal after an ordinary declaration or call. The second leaves the new mutable-pointer ABI incomplete whenever the destination parameter requires more than an exact &T value.

This was a static source review; CI and check results were not considered, as requested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant