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

Improving function "signatures" #2193

Closed
davidhewitt opened this issue Feb 27, 2022 · 8 comments
Closed

Improving function "signatures" #2193

davidhewitt opened this issue Feb 27, 2022 · 8 comments
Milestone

Comments

@davidhewitt
Copy link
Member

TLDR; I propose a #[pyo3(signature)] attribute for both #[pyfunction] and #[pymethods] which:

  • automatically generates text_signature
  • leaves no room for ambiguity
  • is required to specify default arguments (including for Option<T> which implicitly has a None default in some cases at current).

Something like:

#[pyfunction]
#[pyo3(signature = (a, /, b, c = None, d = 5, **kwargs))]
fn foo(a: i32, b: i32, c: Option<i32>, d: i32, kwargs: Option<&PyDict>) {
    (a, b, c, d, kwargs)
}

I've been thinking for a while that there are a few ergonomics improvements we could make around #[pyfunction] signatures. By signatures I collectively mean:

  • The #[pyo3(text_signature)] feature
  • Default arguments, which we support implicitly through Option<T> and explicitly.
  • Other features such as positional-only arguments (/), or *args and **kwargs. These are again supported by the same syntaxes as the "explicit default arguments" above.

There's a few problems with all of this stuff:

  • They can get out of sync: the #[text_signature] repeats a lot of information already present in the rest of the function definition.
  • Implicit Option<T> having a None default makes the possibility of going out of sync worse.
  • After pyfunction: allow required positional after option #2093 we don't make an Option<T> implicitly defaulted to None if a required parameter follows it. This was to avoid a large breaking change on existing code, however it does also make the rules very confusing.
  • There are bugs with the #[args] annotation such as Argument specification by function attribute can be ambiguous #794 .
  • I think it's just way too much boilerplate.

Here's an example which exhibits some of these problems:

#[pyfunction(a, "/", d = 5, kwargs = "**")]
#[pyo3(text_signature = "(a1, b = None, /, d, **kwargs)")]
fn foo(a: i32, b: i32, c: Option<i32>, d: i32, kwargs: Option<&PyDict>) {
    (a, b, c, d, kwargs)
}

In particular:

  • This text_signature has the wrong argument names, wrong defaults, and is missing c.
  • The #[pyfunction(a, "/", d = 5, kwargs = "**")] spec is missing b and c, so it's incredibly unclear what the actual function behavior is. As it turns out, PyO3 will currently make a positional only, b, c and d positional or keyword (c default None and d default 5), and **kwargs as expected.

Fixing this

I don't think we can easily change much about the existing systems. They're widely used, so adding additional checks to the #[args()] system or changing implicit-defaut Option<T> will cause a lot of breakage. Validating text_signature seems impractical.

Instead, my ideal solution would be to adopt a new syntax, which we can migrate over to and deprecate the above. My ideal syntax:

  • Requires as little boilerplate as possible without being ambiguous.
  • Would make options which are defaulted to None an explicit opt-in choice.
  • Automatically generates the text_signature content.

Throwing ideas into the ring, I can think of two possible syntaxes to achieve this. See sketches below of the problematic foo from above rewritten in these.

1. new #[pyo3(signature)] annotation

We could introduce a new attribute option which could meet these ideas. I think this is the better choice for Rust readers, although does lead to some repetition.

#[pyfunction]
#[pyo3(signature = (a, /, b, c = None, d = 5, **kwargs))]
fn foo(a: i32, b: i32, c: Option<i32>, d: i32, kwargs: Option<&PyDict>) {
    (a, b, c, d, kwargs)
}

2. support Python-like syntax directly in the Rust function

We could support things like /, **kwargs and default values directly in the Rust function definition. It's not legal Rust syntax however the proc macro can remove the illegal syntax so that this compiles correctly.

Compared to the above #[pyo3(signature)] attribute, this leads to even less boilerplate and a very natural usage of PyO3 for users mostly writing Python code. However, it makes it much harder to understand how this function should be called from Rust.

#[pyfunction]
fn foo(a: i32, /, b: i32, c: Option<i32> = None, d: i32 = 5, **kwargs: Option<&PyDict>) {
    (a, b, c, d, kwargs)
}

Of the two ideas above, I think I prefer the #[pyo3(signature)] attribute better. However I just wanted to throw all ideas I had out into the ring, and see you folks think as well as what else you might propose.

I'm not sure I'll have a chance to work on this soon, so as the preferred design emerges, if anyone wants to implement this please feel free to claim it!

@birkenfeld
Copy link
Member

I agree that the syntax needs to be more explicit and consistent. Option 2, although attractive for DRY, is a non-starter IMO since tools other than rustc just that ignore attributes would choke on it.

Individual attributes on arguments could be an alternative; I don't remember when they were stabilized though, and I suspect it wouldn't be more readable.

@davidhewitt
Copy link
Member Author

Good points. For sake of sketching out, I guess individual attributes might look something like this:

#[pyfunction]
fn foo(
    a: i32,
    #[pyo3(/)] b: i32,
    #[pyo3(default = None)] c: Option<i32>,
    #[pyo3(default = 5)] d: i32,
    #[pyo3(**)] kwargs: Option<&PyDict>
) {
    (a, b, c, d, kwargs)
}

This would fit nicely with existing #[pyo3(from_py_with)] option we already support per-argument. Would definitely need some designing. Not sure I think it'd be clearer than a standalone #[pyo3(signature)], and it's probably longer by the time #[pyo3()] has been repeated so many times.

(Is there an argument we could allow #[py( )] rather than #[pyo3( )] for brevity if we went down this road?)

@mejrs
Copy link
Member

mejrs commented Mar 7, 2022

I don't think the current situation is so bad. I'm inclined to do nothing with default arguments, and let users handle it themselves:

fn foo(a: i32, b: i32, c: Option<i32>, d: Option<i32>, kwargs: Option<&PyDict>) {
    let d = d.unwrap_or(5);
    ...
}
  1. support Python-like syntax directly in the Rust function

This is too magical and confusing IMO. I really don't want this.

@davidhewitt
Copy link
Member Author

I'm inclined to do nothing with default arguments, and let users handle it themselves:

Using Option works fine in simple cases, and I don't think we necessarily need to change PyO3's base behaviour. I do think we need to provide a clean explicit syntax for default arguments, for two reasons:

  • In signatures where a non-optional value follows Option:

    fn foo(a: i32, b: Option<i32>, c: i32, d: Option<i32>) { ... }

    To avoid panics & breaking build changes, PyO3 0.16 changed it so that b is required rather than optional, because c is required. I do think it's a nasty edge case. To avoid ambiguity, if we had a working #[pyo3(signature = (...)] annotation then we could fail to build in this case and ask user to specify what they want:

    #[pyo3(signature = (a, b = None, c = 0, d = None)]
    fn foo_with_default_b_c(a: i32, b: Option<i32>, c: i32, d: Option<i32>) { ... }
    
    #[pyo3(signature = (a, b, c, d = None)]
    fn foo_with_required_b_c(a: i32, b: Option<i32>, c: i32, d: Option<i32>) { ... }
  • Not allowing None in defaulted arguments:

    #[pyo3(signature = (a = 0))]
    fn foo_int(a: i32) { ... }
    
    #[pyo3(signature = (a = None))]  // This could just be omitted as the default meaning of `Option`
    fn foo_optional_int(a: Option<i32>) { ... }

    The above would be equivalent to the following Python definitions:

    def foo_int(a: int = 0): ...
    
    def foo_optional_int(a: int | None = None): ...

@adamreichold
Copy link
Member

I made at least three mistakes related to text_signature and the actual signature and/or default values going out of sync last week in an internal project. So while I am also not keen on the Python-like syntax, I would be very glad about a single source of truth for the signature and the defaults. (The individual attribute variant seems to be have the least redundancies, but pyo3(signature = "...") does look nicer IMHO as long as consistency is enforced.)

@davidhewitt
Copy link
Member Author

davidhewitt commented Mar 8, 2022

Yes, I'm leaning towards #[pyo3(signature = "...")] too.

@birkenfeld
Copy link
Member

Anything left to do here?

@davidhewitt
Copy link
Member Author

I need to rebase #2703 and I have a branch nearly ready for auto text_signature, which I think captures the remaining ideas here, so yes let's close this.

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

No branches or pull requests

4 participants