fix: avoid #[pymethods] name collisions between regular methods and #[getter]/#[setter]/#[deleter]#6135
Open
mokashang wants to merge 4 commits into
Open
Conversation
…[deleter] (fixes PyO3#5974) Previously, `#[pymethods]` generated the same internal associated function name for two distinct kinds of method: - a getter for `x` (`#[getter] fn x`) becomes `__pymethod_get_x__` (the `get_` prefix is from `impl_py_getter_def`). - a regular method called `get_x` becomes `__pymethod_get_x__` (the whole name comes from `impl_py_method_def`, which uses `__pymethod_{python_name}__`). These clash with `error[E0592]: duplicate definitions with name `__pymethod_get_x__``. The same pattern hits `set_X` against `#[setter]` and `delete_X` against `#[deleter]`, and it survives `#[pyo3(name = "get_y")]` because the regular-method wrapper is keyed off `spec.python_name`. This forces user-facing Python APIs to choose between common idioms that should coexist — a `value` property *and* a separate `get_value` method for parameterised lookup, which is a common pattern when adapting legacy code. Fix the regular-method wrapper to use a `__pymethod_method_*__` infix, which can no longer collide with `__pymethod_get_*__`, `__pymethod_set_*__`, or `__pymethod_delete_*__` regardless of the chosen Python name. The maintainer-suggested direction in PyO3#5974 was to add a distinct prefix that is not a substring of the property prefixes; `method` reads more naturally than `standard` and has the same effect. Add a regression test in `tests/test_getter_setter.rs` covering all three collision shapes (regular method, `#[pyo3(name = ...)]` rename, and the setter variant). Update the existing UI test that was asserting on the old wrapper name.
ngoldbaum
reviewed
Jun 15, 2026
| let wrapper_ident = format_ident!("__pymethod_{}__", spec.python_name); | ||
| // The `method_` infix keeps the generated identifier from colliding with | ||
| // the wrappers for `#[getter]`/`#[setter]`/`#[deleter]`, which use the | ||
| // `get_`/`set_`/`delete_` prefixes (see #5974). |
Contributor
There was a problem hiding this comment.
IMO this kind of comment isn't really helpful. The one in the test is sufficient.
Author
There was a problem hiding this comment.
Agreed — dropped the comment in b07540f. The test docstring already covers it.
The regression test in test_getter_setter.rs already documents the PyO3#5974 collision pattern in detail, so the in-source comment was duplicating that explanation.
The wrapper identifier for regular #[pymethods] methods was renamed
to __pymethod_method_{name}__ to avoid colliding with getter/setter
wrappers (this PR's fix for PyO3#5974). One UI test snapshot still
referenced the old __pymethod_{name}__ symbol both in its //~ ERROR
annotation and in the .stderr file, so the trybuild check failed
on the python3.14t job.
Update the inline directive and snapshot to match the new symbol.
Author
|
Pushed 1bbdc5f to fix the CI failure on The renamed wrapper identifier broke Verified locally: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #5974.
Bug
The
#[pymethods]macro generates an internal associated function forevery entry in the
implblock. Up to now those names came from twodifferent conventions that overlap:
#[getter] fn x__pymethod_get_x__#[setter] fn x__pymethod_set_x__#[deleter] fn x__pymethod_delete_x__fn x__pymethod_x__So defining both a property and a same-named accessor method —
#[getter] fn x+fn get_x— collides on__pymethod_get_x__witherror[E0592]: duplicate definitions. The same trap exists forset_Xagainst#[setter]anddelete_Xagainst#[deleter]. It also survives#[pyo3(name = "get_y")]renames, because the regular-method wrapper is keyed offspec.python_name, not the Rust identifier:This blocks a common Python pattern: shipping a cheap
valueproperty alongside a parameterisedget_value(...)method for callers that need the underlying computation. The original report (#5974) is a real adaptation case where backwards-compatible naming required exactly this shape.Fix
Switch the wrapper name for regular methods from
__pymethod_{python_name}__to__pymethod_method_{python_name}__inimpl_py_method_def. Themethodinfix can no longer be a substring ofget_,set_, ordelete_, so the prefix space is now disjoint regardless of which Python name the user chooses. This matches the direction agreed in #5974 (comment) — pick a distinct prefix for "ordinary" methods — andmethodreads more naturally thanstandardwhile having the same effect.The change is purely internal: these
__pymethod_*__symbols are private associated items consumed by other macro-generated code in the same expansion. No public API moves.Scope
I deliberately kept this narrow:
pyo3-macros-backend/src/pymethod.rsis changed; the#[classattr]andgen_py_constsites also generate__pymethod_{name}__, but their collision space (uppercase-snake constants, class-level Python names) is much harder to hit in real code thanget_X/set_X/delete_X. Happy to expand if reviewers prefer.__pymethod___new____,__pymethod___richcmp____,__pymethod_traverse__,__pymethod_constructor__,__pymethod_variant_cls_*etc. are unchanged — they don't share the property-prefix shape.Tests
property_and_regular_method_can_share_name_prefixintests/test_getter_setter.rs. It exercises all three collision shapes — regular method (fn get_x),#[pyo3(name = "get_y")] fn y_getrename, and the setter variantfn set_zagainst#[setter] fn z— and verifies that each callable reaches the right Rust impl from Python (instance.x == 10vsinstance.get_x() == 11, etc.). Without the fix this test fails to compile withE0592.tests/ui/invalid_pymethods_duplicates.rs(+.stderrsnapshot): the existing "two methods both renamed tofunc" duplicate-detection still trips, just on__pymethod_method_func__now.cargo test -p pyo3 --testsis green across the test suite I can run locally on macOS (class basics, class attributes, getter/setter, methods, proto methods, multiple-pymethods feature, inheritance, declarative module, etc.). UI tests that fail on my machine all fail identically onmain— they're rustc-version-sensitive snapshots and unrelated to this change.cargo fmt --checkclean;cargo clippy --tests --features=macrosclean.