Skip to content

transform: avoid field-name collisions for bound methods - #28787

Open
medvednikov wants to merge 4 commits into
masterfrom
fix/28784-bound-method-field-collision
Open

medvednikov wants to merge 4 commits into
masterfrom
fix/28784-bound-method-field-collision

Conversation

@medvednikov

Copy link
Copy Markdown
Member

Fixes #28784.

Summary

  • Stop selector field-type inference from falling back to an unrelated struct's same-named field after the receiver has been resolved to a known struct.
  • Preserve direct and promoted-field lookup, and retain the existing fallback when receiver struct information is unavailable.
  • Add a focused transformer regression for value/pointer receivers and the unknown-receiver fallback.
  • Add language regressions covering the reported optional fn () callback assignment, a callback that reads receiver state, reassignment to a different receiver, invocation after assignment, and resetting the callback to none.

Diagnosis

In the report, Model has an on_done method but no on_done field. resolve_selector_type_uncached nevertheless falls through to lookup_unique_field_type('on_done'), which can supply the ?fn () field type from Process. A method value must not acquire the optionality of an unrelated field merely because their names match.

After receiver-specific field and promoted-field lookup fails for a known struct, leave field inference unresolved so the existing checker-backed expression-type fallback can supply the method's function type. The compiler change is four added lines, including its explanatory comment.

Validation status

Draft: the diagnosis and patch have been reviewed statically, but reproduction and runtime validation are not yet complete.

  • Verified the committed diff against 9bfa1dc3f3455d5733d5c226662b6aaba8544f2c: only the four-line compiler change and the two new regression files are present.
  • No V build, formatter, or tests were run. The execution sandbox has no V compiler, and repository/bootstrap downloads are blocked (Could not resolve host: github.com / raw.githubusercontent.com). Repository reads and branch commits were made through the connected GitHub API.
  • No CI, build configuration, or unrelated source files were changed.

Commands still requiring an environment with V available:

./v self
./v fmt -verify vlib/v/transform/type_propagation.v vlib/v/transform/selector_method_field_collision_test.v vlib/v/tests/options/option_fn_bound_method_field_name_collision_test.v
./v -silent test vlib/v/transform/selector_method_field_collision_test.v
./v -silent test vlib/v/tests/options/option_fn_bound_method_field_name_collision_test.v
./v -silent vlib/v/compiler_errors_test.v
./v -silent test vlib/v/

The focused regressions should also be checked against the parent revision to establish a failing-before/passing-after result before marking this ready for review.

@medvednikov
medvednikov marked this pull request as ready for review September 19, 2026 12:32
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 19, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-19T14:15:57.911689Z ff12b02 New commits
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cfd8f7f0fc

ℹ️ 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".

Comment on lines +771 to +774
// A selector on a known struct may be a bound method. Do not infer its
// type from a same-named field on an unrelated struct; let the checker
// supply the method's function type instead.
return ''

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve type resolution through nested embeddings

When a field is promoted through two or more embedded structs, this return incorrectly classifies it as a possible method and produces no type. embedded_field_for_promoted_field in vlib/v/transform/struct.v checks only whether the immediate embedded struct directly declares the field, so a valid selector such as Baz.x in vlib/v/tests/structs/nested_struct_embed_selector_test.v (Baz -> Bar -> Foo.x) reaches this branch; before this change, the unique-field fallback returned int. Direct callers of resolve_selector_type, including specialized array/loop inference paths, can consequently lose the field type when a checker annotation is unavailable, so the known-struct early return should occur only after a recursive promoted-field lookup.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in ff12b02.

Replaced the one-level promoted-field lookup with the existing recursive struct_field_path_for_field helper, then resolve the field type from the final owning struct. Pointer embeddings are unwrapped for the owner lookup, and the known-struct guard still prevents borrowing an unrelated same-named field's type.

Added a checker-free regression covering eight combinations of value/pointer receivers, value/pointer embeddings, and scalar/array fields, plus nearer-field and receiver-field shadowing. The original method/field collision regression remains in place.

Verified that the committed diff matches the prepared two-file patch. V compilation, formatting, and runtime tests remain unrun locally: the sandbox has no V compiler, and bootstrap downloads fail DNS resolution. Leaving the thread open for re-review.

Use the recursive embedding path before rejecting unrelated field-name
fallbacks for known receivers. Cover nested value/pointer embeddings,
scalar/array fields, and nearer-field shadowing without checker annotations.

Addresses the nested-embedding review on #28787.

This branch has not been deployed

No deployments
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.

C error with multiple structs that contain the same method/property name

1 participant