Hygiene: offer a way to set path to pyo3 crate

[birkenfeld]

Unfinished: see // TODO comments. Need to be able to pass #[pyo3(pyo3_path)] attributes to the FromPyObject derive macro, pyclass-enums, and pymethods.

Implementation note: Since I didn't want to add pyo3_path args to dozens of functions, I used scopes whenever possible within which I can do use #pyo3_path as _pyo3; and generate _pyo3:: references everywhere else. This meant adding some const () for impl blocks.

#pyproto is a bit harder to support since it modifies function arguments of the existing impl and I didn't want to move that impl into a const block. We can just deprecate pyproto anyway and document that this feature doesn't work with it.

[davidhewitt]

Thanks! Will try to give this a full review tomorrow sometime.

We can just deprecate pyproto anyway and document that this feature doesn't work with it.

That's totally reasonable to me; I think that's the plan for 0.16.

[birkenfeld]

When you review, you can exclude the first commit, which is just replacing ::pyo3:: by _pyo3:: everywhere in the proc-macro crate. Should be much easer to see the important stuff then.

[birkenfeld]

This is now feature complete, but I'm unsure how to test it: ::pyo3 is always available in the test suite...

[birkenfeld]

Ah yes, and there is still the issue of generating unique but reproducible const names for pymethods if we're using inventory.

[adamreichold]

This is now feature complete, but I'm unsure how to test it: ::pyo3 is always available in the test suite...

I think adding a separate crate like those in the examples folder as a kind of build test that is explicitly invoked by the CI could work?

[mejrs]

This is now feature complete, but I'm unsure how to test it: ::pyo3 is always available in the test suite...

If we do away with the self-dependency:

# features needed to run the PyO3 test suite
pyo3 = { path = ".", default-features = false, features = ["macros", "auto-initialize"] }

::pyo3 will no longer be available within pyo3 itself, and we can use unit tests for this (I think):

#[test]
fn test(){
    #[crate::pyclass(pyo3_path=crate)]
    struct Foo;
}

[birkenfeld]

#1817 seems to be blocked on some other things though.

[mejrs]

It should still raise a compilation error if types/functions/traits from crate and ::pyo3 get mixed up.

For example, this test doesn't compile:

pub struct Foo;

pub fn take_foo(_: Foo){}

#[cfg(test)]
#[test]
fn bar(){
    let foo = crate::Foo;
    ::pyo3::take_foo(foo);
}

[birkenfeld]

@davidhewitt do you have an idea how to generate unique constant names for the inventory pymethods?

[davidhewitt]

Sorry was hoping to give this a full read soon! Can we get away with just doing const _ instead of giving it a name?

[birkenfeld]

Oh wow, that works? facepalm it does!

[davidhewitt]

If we do away with the self-dependency:

Seeing this very clever idea convinced me to take another look at that - see #2039

[davidhewitt]

It might be nice to document this alongside each macro too. I really like serde's reference on attributes e.g. https://serde.rs/container-attrs.html

I've tried to do this a bit for #[pyfunction] in guide/function.md, but it's sorely lacking. There's also some docs in pyo3-macros/lib.rs.

[birkenfeld]

Maybe I'll tackle this (also for the other attributes) in a follow-up? This attribute is certainly the least-needed one :)

[davidhewitt]

Yep I think that's fine!

[davidhewitt]

How do you feel about it being #[pyo3(crate = "...")] rather than pyo3_path, so that we follow exactly the convention set by serde?

[birkenfeld]

That was my first try, but unfortunately incompatible with the way we parse keywords (you can't pass crate to syn::custom_keyword, and r#crate is invalid for some reason).

[danielhenrymantilla]

I believe it's Token![crate]. FWIW, I personally prefer the crate = "…" syntax as well 🙂

[birkenfeld]

Perfect! (Also thanks a lot for your other PR, I've transplanted it onto this branch to see if the tests pass.)

[birkenfeld]

So, rebased against main and moved the test into lib, as suggested.

However, now I get a curious error which I can't make sense of:

error: macro-expanded `macro_export` macros from the current crate cannot be referred to by absolute paths
   --> src/test_hygiene/pymethods.rs:12:1
    |
12  | #[crate::pymethods]
    | ^^^^^^^^^^^^^^^^^^^
    |
    = note: `#[deny(macro_expanded_macro_exports_accessed_by_absolute_paths)]` on by default
    = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
    = note: for more information, see issue #52234 <https://github.com/rust-lang/rust/issues/52234>
note: the macro is defined here
   --> src/class/impl_.rs:161:9
    |
161 | /         macro_rules! $generate_macro {
162 | |             ($cls:ty) => {{
163 | |                 unsafe extern "C" fn __wrap(
164 | |                     _slf: *mut $crate::ffi::PyObject,
...   |
184 | |             }};
185 | |         }
    | |_________^
...
189 | / define_pyclass_setattr_slot! {
190 | |     PyClass__setattr__SlotFragment,
191 | |     PyClass__delattr__SlotFragment,
192 | |     __setattr__,
...   |
198 | |     setattrofunc,
199 | | }
    | |_- in this macro invocation
    = note: this error originates in the attribute macro `crate::pymethods` (in Nightly builds, run with -Z macro-backtrace for more info)

Any ideas? Does the generate_macro need to be macro_exported?

[davidhewitt]

Coverage failure is rust-lang/rust#91689 - I guess we'll just have to live with it broken for a day or two.

[davidhewitt]

Thanks, this looks great to me! I think it's a nice piece of flexibility this adds for users.

Just one thought about variable names, otherwise please merge!

[davidhewitt]

Now that we have crate = I'd be nice to call this PyO3CrateAttribute or CrateAttribute, similarly some variables called pyo3_path might want to be called pyo3_crate (or krate?! - then expanding it inside proc-macros as #krate is no longer than macro_rules $crate)

[birkenfeld]

Agreed, done!

[davidhewitt]

Perfect! Thanks @birkenfeld and @danielhenrymantilla!