Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

non-local impl definition, they should be avoided as they go against expectation #4094

Closed
waynexia opened this issue Apr 18, 2024 · 3 comments
Labels

Comments

@waynexia
Copy link

waynexia commented Apr 18, 2024

Bug Description

In our code https://github.com/GreptimeTeam/greptimedb/blob/main/src/script/src/python/pyo3/vector_impl.rs#L94 like

#[pymethods]
impl PyVector {
    /// convert from numpy array to [`PyVector`]
    #[classmethod]
    fn from_numpy(cls: &PyType, py: Python<'_>, obj: PyObject) -> PyResult<PyObject> {
        let pa = py.import("pyarrow")?;
        let obj = pa.call_method1("array", (obj,))?;
        let zelf = Self::from_pyarrow(cls, py, obj.into())?;
        Ok(zelf.into_py(py))
    }
    ...
}

Compile with toolchain nightly-2024-04-18 would get the following error:

error: non-local `impl` definition, they should be avoided as they go against expectation
  --> src/script/src/python/pyo3/vector_impl.rs:94:1
   |
94 | #[pymethods]
   | ^^^^^^^^^^^^
   |
   = help: move this `impl` block outside the of the current method `__or__` and up 2 bodies
   = note: an `impl` definition is non-local if it is nested inside an item and may impact type checking outside of that item. This can be the case if neither the trait or the self type are at the same nesting level as the `impl`
   = note: one exception to the rule are anon-const (`const _: () = { ... }`) at top-level module and anon-const at the same nesting as the trait or type
   = note: this lint may become deny-by-default in the edition 2024 and higher, see the tracking issue <https://github.com/rust-lang/rust/issues/120363>
   = note: the attribute macro `pymethods` may come from an old version of the `pyo3_macros` crate, try updating your dependency with `cargo update -p pyo3_macros`
   = note: this error originates in the attribute macro `pymethods` (in Nightly builds, run with -Z macro-backtrace for more info)

Steps to Reproduce

git clone https://github.com/waynexia/greptimedb.git
cd greptimedb
git checkout update-toolchain-3
make clippy

Source

Backtrace

N/A

Your operating system and version

Arch Linux x86_64 6.8.6-arch1-1

Your Python version (python --version)

Python 3.11.8

Your Rust version (rustc --version)

rustc 1.79.0-nightly (becebb315 2024-04-17)

Your PyO3 version

0.20

How did you install python? Did you use a virtualenv?

pacman

Additional Info

To workaround it, add this to Cargo.toml

rust.non_local_definitions = "allow"
@waynexia waynexia added the bug label Apr 18, 2024
@adamreichold
Copy link
Member

Please update to the current version 0.21.2 which resolves this.

@tisonkun
Copy link
Contributor

tisonkun commented Apr 18, 2024

@adamreichold Thanks for your information!

In GreptimeDB we can't upgrade pyo3 now due to our deps (arrow, datafusion) are using pyo3 0.20:

But once these deps upgrade, we'll try this out.

Cross ref -

@davidhewitt
Copy link
Member

Closing as resolved in latest PyO3 versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants