#[new] doesn't play nice with cfg_attr

[c410-f3r]

Compiles:

use pyo3::prelude::*;

#[pyclass]
#[derive(Debug)]
pub struct Foo {
  field: i32
}

#[pymethods]
impl Foo {
  #[new]
  pub fn new(field: i32) -> Self {
    Self { field }
  }
}

Doesn't compile with Invalid type as custom self and cannot find attribute 'new' in this scope errors:

#[cfg(feature = "with_pyo3")]
use pyo3::prelude::*;

#[cfg_attr(feature = "with_pyo3", pyclass)]
#[derive(Debug)]
pub struct Foo {
  field: i32
}

#[cfg_attr(feature = "with_pyo3", pymethods)]
impl Foo {
  // Change `#[cfg_attr(feature = "with_pyo3", new)]` for `#[new]` and `cargo build --feature with_py03` to see a successful compilation.
  #[cfg_attr(feature = "with_pyo3", new)]
  pub fn new(field: i32) -> Self {
    Self { field }
  }
}

[davidhewitt]

According to gitter this is also an issue for #[pyo3(get)] etc.

Probably needs a thorough review of cfg_attr compatibility.

[c410-f3r]

Version 0.10 worsens conditional compilation. Now #[cfg_attr(feature = "with_pyo3", pymethods)] and #[cfg_attr(feature = "with_pyo3", pyclass)] doesn't compile either.

[davidhewitt]

Thanks for the update. I'll try to give this some investigation soon.

[birkenfeld]

I can't reproduce the latest report; the example given above compiles fine with 0.10 if I make the #[new] unconditional, but the other attributes remain conditional.

It does not compile without #[new], but that has nothing to do with conditional attributes.

[birkenfeld]

I'm not sure how to resolve this. We can of course look into cfg_attr and ignore it while processing attributes, but that is only correct when the condition is as reported here, switching PyO3 off. Something like switching between two different #[new] implementations depending on flags would still fall down.

Is there a way at all to resolve cfg_attr at macro evaluation time?

[davidhewitt]

I don't know if we can "resolve" cfg_attr at macro evaluation time, but I guess we can check if the cfg_attr on the method is the same as the one on the whole pymethods block - and then if we're running the pymethods attribute macro we know that the cfg_attr must be applicable.

I suggest we start building a list of examples in this thread. For each example, we can either:

• figure out a way to support it, and add a PR and tests.
• add documentation saying why we can't support it.

My hope is that we can support "typical" use cases in this way. Or we're pleasantly surprised and find an elegant solution for everything!

@c410-f3r it would be really helpful if you could start us off by providing the new examples which you reported today as not working with 0.10.

[birkenfeld]

I don't know if we can "resolve" cfg_attr at macro evaluation time, but I guess we can check if the cfg_attr on the method is the same as the one on the whole pymethods block

Can we, though? When the pymethods macro is called its cfg_attr is already gone...

[davidhewitt]

Can we, though? When the pymethods macro is called its cfg_attr is already gone...

Oh, darn. But the other ones still present?

[programmerjake]

maybe something like the cfg_expr crate could be used to evaluate cfg() conditions? This really should be part of the proc_macro library.

[birkenfeld]

Oh, darn. But the other ones still present?

Yes, that's the problem. I quickly checked with a serde derive(Deserialize) and an inner serde attribute, and that worked. So either serde is doing some work here, or handling for derive macros is different and all attributes are resolved before the derive macro is called. (I suspect the latter.)

EDIT: confirmed, it's the latter.

@programmerjake as I understand, that's one half of the puzzle. (Already very helpful!) The other would be to get the current build config and use it evaluating these expressions. Does the proc macro have access to it?

[programmerjake]

@programmerjake as I understand, that's one half of the puzzle. (Already very helpful!) The other would be to get the current build config and use it evaluating these expressions. Does the proc macro have access to it?

I think it might through env vars from cargo.

Created thread on rust internals: https://internals.rust-lang.org/t/add-api-for-evaluating-cfg-conditions-from-proc-macros/12459?u=programmerjake

[c410-f3r]

@davidhewitt There are no errors with pymethods or pyclass because I forgot to enable the macros feature flag. I am sorry for the fuzz, should have paid more attention to the changelog.

[gcoakes]

Is there a work around for this besides just manually implementing the getter/setter in a #[pymethods] impl block? I have multiple structs with many fields each which I would like to expose in a python class, but I don't want it enabled unless a feature flag is enabled.

[davidhewitt]

At the moment, I'm not aware of any "nice" workaround.

I wonder if we can resolve this by changing #[pymethods] expansion to just mark all items inside the impl block with an attribute (e.g. #[pyo3::__pymethod]) which then does the actual pymethod expansion. Presumably when each item will be expanded it's own cfg_attr entries will have been resolved. I'm unsure if this will affect application startup times negatively, because an inventory submission will then be needed per-method instead of once for the whole pymethods block.

[roblabla]

The CFG can easily be acquired through environment variables. See this: https://doc.rust-lang.org/cargo/reference/environment-variables.html#environment-variables-cargo-sets-for-build-scripts

CARGO_FEATURE_<name> — For each activated feature of the package being built, this environment variable will be present where is the name of the feature uppercased and having - translated to _.

CARGO_CFG_<cfg> — For each configuration option of the package being built, this environment variable will contain the value of the configuration, where <cfg> is the name of the configuration uppercased and having - translated to _. Boolean configurations are present if they are set, and not present otherwise. Configurations with multiple values are joined to a single variable with the values delimited by ,. This includes values built-in to the compiler (which can be seen with rustc --print=cfg) and values set by build scripts and extra flags passed to rustc (such as those defined in RUSTFLAGS).

It should be possible to use those two to evaluate everything we might need.

[davidhewitt]

Looks like rust-lang/rust#83824 would resolve this on a sufficiently new rust (nightly atm).

If anyone gets a chance to verify this, then this would become just a documentation ticket.

[mejrs]

If anyone gets a chance to verify this

It looks that way.

Is there a way we can insert this attribute ourselves rather than having users use it? It would be nice to make this Just Work.

[davidhewitt]

I guess we could have

#[pyclass]
struct Foo { }

literally just adds #[cfg_eval], expanding to

#[cfg_eval]
#[pyclass(cfg_eval = false)]
struct Foo {}

where #[pyclass(cfg_eval = false)] then expands as normal.

... but if we do that, we'll suprise people because the #[cfg_eval] happens behind the scenes. I think it'll also regress compilation times because we'll be forcing two extra proc macro expansions for every pyclass. It seems better to me to leave it for users to add #[cfg_eval] themselves when needed?

We'd also only be able to do the automatic cfg-eval on Rust versions which actually support it, which again makes it seem better that users should have to manually update to a new enough compiler version and explicitly add #[cfg_eval].

[mejrs]

I worked around this by creating a proc macro that inserts #[pyo3(get)] for every field:

#[cfg_attr(feature = "pyo3", macro_utils::pyo3_get_all)]
#[cfg_attr(feature = "pyo3", pyclass)]
#[derive(Serialize, Clone, Debug, Default)]
pub struct Foo{
    pub id: u32,
    pub name: Option<String>,
}

That works out because all the (hundred+) fields are public anyway. Perhaps we can do something similar like #[pyclass(get_all, set_all)]?

[davidhewitt]

That's a really interesting idea. Very much related to the idea in #1375.

#[pyclass(dataclass)] was proposed to do a lot more (add #[new], cmp, and hashing, as well as get/set for all pub fields). I think there could be value in also having get_all and set_all as you propose.

[JCGrant]

The example @c410-f3r posted now works if you use #[cfg_eval] on your impl! 🙌

#![feature(cfg_eval)]

#[cfg(feature = "with_pyo3")]
use pyo3::prelude::*;

#[cfg_attr(feature = "with_pyo3", pyclass)]
#[derive(Debug)]
pub struct Foo {
    field: i32,
}

#[cfg_eval]
#[cfg_attr(feature = "with_pyo3", pymethods)]
impl Foo {
    #[cfg_attr(feature = "with_pyo3", new)]
    pub fn new(field: i32) -> Self {
        Self { field }
    }

    pub fn get_field(&self) -> i32 {
        self.field
    }
}

#[cfg(feature = "with_pyo3")]
#[pymodule]
fn my_module(_py: Python, m: &PyModule) -> PyResult<()> {
    m.add_class::<Foo>()?;
    Ok(())
}

from my_module import Foo

c = Foo(5)
print(c.get_field())

This currently requires Rust nightly (so #![feature(cfg_eval)] needs to be added), but it seems that cfg_eval will be stabilised in this PR: rust-lang/rust#87221.

[c410-f3r]

Closing due to inactivity. Feel free to re-open this issue if desired.

[andyblarblar]

I worked around this by creating a proc macro that inserts #[pyo3(get)] for every field:

#[cfg_attr(feature = "pyo3", macro_utils::pyo3_get_all)]
#[cfg_attr(feature = "pyo3", pyclass)]
#[derive(Serialize, Clone, Debug, Default)]
pub struct Foo{
    pub id: u32,
    pub name: Option<String>,
}

That works out because all the (hundred+) fields are public anyway. Perhaps we can do something similar like #[pyclass(get_all, set_all)]?

Would you mind making this macro a gist (assuming you still have it)? It seems like the nightly feature used above isn't going to be stabalized anytime soon, and I've ran into the same situation of annotating large structs with getters and don't care about using a hack as this is just a hobby project.

[mejrs]

@andyblarblar see https://github.com/mejrs/rs3cache/blob/master/rs3cache_macros/src/lib.rs

Note that functionality like #[pyclass(get_all, set_all)] is still something I want to add to pyo3 :)

[pacowong]

Hello, I ran into the same issue today. It seems that the solution using cfg_eval is not going to be adopted. Are there alternatives to solve #[cfg_attr(feature = "with_pyo3", new)] problem?

[yodaldevoid]

I think this issue needs to be reopened seeing as it doesn't look like cfg_eval will ever be stabilized.

As for how to move forward, it is probably worth it look look at parsing out features and build configurations using CARGO_FEATURE_<name> and CARGO_CFG_<cfg>, respectively. It's fair to say that this would not be solely helpful to PyO3, but where it lives can be figured out once it works.

EDIT: never mind, the environment variables in question are not set when cargo runs the proc macro.

[davidhewitt]

Happy to reopen this issue, however there is still no clear solution to this - and as the comment above points out, I'm not aware of any way that we could expand cfgs ourself. So I think we basically need someone willing to help upstream rust figure out the path (if it's not cfg_eval).

[c410-f3r]

At least for this project, it seems that upstream is the only solution.

You guys might want to reach out Vadim Petrochenkov (OP of rust-lang/rust#87221) to see if any help is needed. Otherwise, it is possible that this issue will be open for years to come.

[c410-f3r]

I am house-keeping my dashboard closing issues that will probably take time to resolve and are not on my radar any more. If desired, feel free to create another issue with any updated status from #2692.

[rdelfin]

Any chance the pyo3::pyo3_get_all and a pyo3::pyo3_set_all can be integrated into this crate? This is an ongoing issue from what I can tell that's making it hard to have code that uses feature flags to enable/disable pyo3 support. Happy to look into opening a PR for this

[davidhewitt]

#[pyclass(get_all, set_all)] are already released. https://pyo3.rs/v0.19.2/class#customizing-the-class

[rdelfin]

Ah fantastic, thanks @davidhewitt!

[zuckerruebe]

My current workaround is just to define the new function twice and then conditionally compile one or the other:

#[cfg_attr(feature = "python-bindings", pymethods)]
impl Parser {
    #[cfg(feature = "python-bindings")]
    #[new]
    pub fn new() -> Parser {
        Parser {
            token_definitions: TokenDefinitions::new(),
        }
    }

    pub fn parse_variable(&self, input: &str) -> Result<Variable, Error> {
        let token_definition = &self.token_definitions.variable;

        match (&token_definition.regex).find(input) {
            Some(m) => {
                let name = String::from(m.as_str());
                Ok(Variable { name })
            }
            None => {
                let message = format!("Not a {}.", token_definition.description);
                Err(Error { message })
            }
        }
    }
}

#[cfg(not(feature = "python-bindings"))]
impl Parser {
    pub fn new() -> Parser {
        Parser {
            token_definitions: TokenDefinitions::new(),
        }
    }
}

Just for reference. I might have missed a similar approach somewhere above. If the new functions grew to something more substantial I'd probably extract it and share between both "compilations".

[ThatOneShortGuy]

@zuckerruebe To avoid replicating the new functions, you can rename the python new function something different that calls a regular new function. Then you drop the #[cfg(not(...))]

#[cfg_attr(feature = "python-bindings", pymethods)]
impl Parser {
    #[cfg(feature = "python-bindings")]
    #[new]
    pub fn new_py() -> Parser {
        Parser::new()
    }

    pub fn parse_variable(&self, input: &str) -> Result<Variable, Error> {
        let token_definition = &self.token_definitions.variable;

        match (&token_definition.regex).find(input) {
            Some(m) => {
                let name = String::from(m.as_str());
                Ok(Variable { name })
            }
            None => {
                let message = format!("Not a {}.", token_definition.description);
                Err(Error { message })
            }
        }
    }
}

impl Parser {
    pub fn new() -> Parser {
        Parser {
            token_definitions: TokenDefinitions::new(),
        }
    }
}

In my project, it made no difference to usability from any resource. Hope this helps!