Skip to content

Conversation

Tpt
Copy link
Contributor

@Tpt Tpt commented Mar 31, 2025

No annotations or explicit default values yet

Fixes an issue related to function identifier paths

@Tpt Tpt added the CI-skip-changelog Skip checking changelog entry label Mar 31, 2025
@Tpt Tpt force-pushed the tpt/sub-fn-signature branch 2 times, most recently from 91f84e9 to cf48e96 Compare March 31, 2025 11:13
@Tpt Tpt self-assigned this Apr 4, 2025
@Tpt Tpt requested a review from davidhewitt April 4, 2025 07:29
#[derive(Debug, Eq, PartialEq, Clone, Hash)]
pub struct Argument {
pub name: String,
pub kind: ParameterKind,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if we should use this kind enum or a set of different list of arguments (posonlyargs, args, vararg, kwonlyargs, kw_defaults, kwarg) like the Python AST module.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The advantage of having different lists of arguments would be that it guarantees the structure of parameters is ok. Otherwise the parameters might come in a confusing order. Maybe something like:

positional_args: Vec<String>,
positional_only_args: usize, // within range 0..positional_args.len()
positional_defaults: Vec<String>, // at most has positional_args.len() elements
vararg: Option<String>,
keyword_only_parameters: Vec<(String, Option<String>)>, // keyword, maybe with default
kwarg: Option<String>

This would have reasonable alignment with the FunctionDescription type internal to PyO3's argument parsing, which might make it easier to generate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I have done it in a slightly different way, still using an Argument object (useful to store both the default value and the type annotation). Happy to tweak it again if you want.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looking nice! Some suggestions...

#[derive(Debug, Eq, PartialEq, Clone, Hash)]
pub struct Argument {
pub name: String,
pub kind: ParameterKind,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The advantage of having different lists of arguments would be that it guarantees the structure of parameters is ok. Otherwise the parameters might come in a confusing order. Maybe something like:

positional_args: Vec<String>,
positional_only_args: usize, // within range 0..positional_args.len()
positional_defaults: Vec<String>, // at most has positional_args.len() elements
vararg: Option<String>,
keyword_only_parameters: Vec<(String, Option<String>)>, // keyword, maybe with default
kwarg: Option<String>

This would have reasonable alignment with the FunctionDescription type internal to PyO3's argument parsing, which might make it easier to generate.

Comment on lines 168 to 169
// TODO: generate a nice default values for literals (None, false, 0, ""...)
params.insert("default_value", IntrospectionNode::String("..."));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should re-use the machinery we have for text signature docstrings, see default_value_for_parameter() in signature.rs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I missed it. Done.

@Tpt Tpt force-pushed the tpt/sub-fn-signature branch 2 times, most recently from 95b51d0 to 5b1f54a Compare April 4, 2025 15:20
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, overall this looks good to me with just a few questions of finishing touches 👍

@@ -48,5 +48,36 @@ fn class_stubs(class: &Class) -> String {
}

fn function_stubs(function: &Function) -> String {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be nice to check that vararg and kwarg have no default as part of this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added a new VariableLengthArgument type without the default field, this prevents the need for this check.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be worth adding unit tests to this file for various cases to show they format as we want.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! (note we already got good coverage with the integration tests)

}

#[derive(Debug, Eq, PartialEq, Clone, Hash)]
pub struct Arguments {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am mostly happy with this, though I do somewhat wish that this encoded the fact that vararg and kwarg cannot be defaulted (and also that required positional arguments cannot follow positional arguments with defaults).

In practice this is probably good enough, as long as we add suitable checks elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added a new VariableLengthArgument type without the default field.

.emit(pyo3_crate_path)
}

fn arguments_introspection_data<'a>(signature: &'a FunctionSignature<'a>) -> IntrospectionNode<'a> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like the logic here might be somewhat duplicated with other code in the macros which sets up the argument parsing. Do you think there's opportunity to refactor and reuse?

Copy link
Contributor Author

@Tpt Tpt Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe if we introduce an other intermediate data structure that splits the argument by kind. But it will add quite a bit of boilerplate (new structs to represent the arguments). What do you prefer?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep this as-is for now, maybe we refactor later if there seems to be opportunity.

m.add_function(wrap_pyfunction!(simple_args_kwargs, m)?)?;
m.add_function(wrap_pyfunction!(args_kwargs, m)?)?;
Ok(())
#[pyfunction(signature = (a, /, b))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be nice to add tests / benchmarks to test_pyfunctions.py to match the rest of the functions in this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@Tpt Tpt force-pushed the tpt/sub-fn-signature branch from 5b1f54a to 57ef77c Compare April 8, 2025 18:16
@Tpt Tpt force-pushed the tpt/sub-fn-signature branch from 28ee5ab to 9d63c36 Compare April 9, 2025 06:56
@abrisco
Copy link
Contributor

abrisco commented Apr 15, 2025

Anything more needed for this change?

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks good to me!

.emit(pyo3_crate_path)
}

fn arguments_introspection_data<'a>(signature: &'a FunctionSignature<'a>) -> IntrospectionNode<'a> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep this as-is for now, maybe we refactor later if there seems to be opportunity.

@davidhewitt davidhewitt added this pull request to the merge queue Apr 21, 2025
Merged via the queue into PyO3:main with commit b2ee6b6 Apr 21, 2025
52 checks passed
ngoldbaum pushed a commit to clin1234/pyo3 that referenced this pull request Apr 25, 2025
* Introspection: add function signatures

No annotations or explicit default values yet

Fixes an issue related to object identifiers path

* Better default value

* Refine arguments struct

* Introduce VariableLengthArgument

* Adds pyfunctions tests

* Adds some serialization tests
@ngoldbaum
Copy link
Contributor

It looks like this along with #5090 is leading to new 3.7-specific CI issues:

https://github.com/PyO3/pyo3/actions/runs/14670527221/job/41175810604?pr=4811#step:28:991

  E     File "/home/runner/work/pyo3/pyo3/pytests/tests/test_pyfunctions.py", line 90
  E       def positional_only_py(a, /, b):
  E                                 ^
  E   SyntaxError: invalid syntax
  =========================== short test summary info ============================
  ERROR tests/test_pyfunctions.py

Probably just needs to have version guards or whatever to avoid running this code on 3.7. @Tpt would you mind putting in a fix?

github-merge-queue bot pushed a commit that referenced this pull request Apr 26, 2025
* Add news item

* Move news file

* Fix version limit check in noxfile.py

* Bump Python version for testing debug builds

* 3.14 is available from GH's setup-python action

* Bump maximum supported CPython version in pyo3-ffi

* Rework PyASCIIObject and PyUnicodeObject to be compatible with 3.14

Due to python/cpython#128196, data types within `PyASCIIObject.state` have changed, resulting in test failures when building against 3.14.

* Run `cargo fmt --all`

* Actually add Py_3_14 as a legitimate macro

When `rustc` is invoked, the macro is included with the `--check-cfg`
flag, but not with the `--cfg` flag. This caused errors about duplicate
definitions to spew out when building with stable Rust toolchains.

* Revert "Actually add Py_3_14 as a legitimate macro"

This reverts commit 5da57af.

* Fix version macro placement for 3.14-specific getters and setters

* Import 'c_ushort' only if compiling against CPython 3.14 or later

* Add wrapper functions for the statically_allocated field

* Remove unused libc::c_ushort

* Add (hopefully) final version-specific macros

* Port 3.14-specific 64-bit code of Py_INCREF

* Don't expose PyDictObject.ma_version_tag when building against 3.14 or later

* fix ffi-check on the GIL-enabled ABI

* fix older pythons

* fix ffi-check on older pythons

* WIP: update for 3.14t

* fix ffi-check on the free-threaded build

* fix clippy

* fix clippy on older python versions

* fix cargo check on the MSRV

* fix ffi-check on 3.13t

* fix CI which is using 3.13.1

* fix copy/paste error in noxfile

* update ffi bindings for the latest changes in 3.14

* update layout of refcnt field on gil-enabled build

* delete unused HangThread struct

* fix ffi-check on GIL-enabled build

* Revert "delete unused HangThread struct"

This reverts commit 3dd439d.

* config-out HangThread

* fix 3.13 ffi-check

* fix debug python build error

* fix graalpy build

* Ignore DeprecationWarnings from the  pytest_asyncio module in tests

* Add abi3-py314

* fix free-threading issue in `test_coroutine` (#5069)

* Introspection: add function signatures (#5025)

* Introspection: add function signatures

No annotations or explicit default values yet

Fixes an issue related to object identifiers path

* Better default value

* Refine arguments struct

* Introduce VariableLengthArgument

* Adds pyfunctions tests

* Adds some serialization tests

* respond to david's code review

* add comment and fix test failure

* fix check-feature-powerset

* fix clippy

* fix wasip1 clippy

* fix 32 bit python 3.14 bug

* mark test-py step continue-on-error for dev python builds

* use github issue URL

* run ffi-check before running tests

* fix ffi-check for 3.14.0a7

---------

Co-authored-by: Nathan Goldbaum <[email protected]>
Co-authored-by: David Hewitt <[email protected]>
Co-authored-by: Thomas Tanon <[email protected]>
newcomertv pushed a commit to newcomertv/pyo3 that referenced this pull request Apr 28, 2025
* Introspection: add function signatures

No annotations or explicit default values yet

Fixes an issue related to object identifiers path

* Better default value

* Refine arguments struct

* Introduce VariableLengthArgument

* Adds pyfunctions tests

* Adds some serialization tests
newcomertv pushed a commit to newcomertv/pyo3 that referenced this pull request Apr 28, 2025
* Add news item

* Move news file

* Fix version limit check in noxfile.py

* Bump Python version for testing debug builds

* 3.14 is available from GH's setup-python action

* Bump maximum supported CPython version in pyo3-ffi

* Rework PyASCIIObject and PyUnicodeObject to be compatible with 3.14

Due to python/cpython#128196, data types within `PyASCIIObject.state` have changed, resulting in test failures when building against 3.14.

* Run `cargo fmt --all`

* Actually add Py_3_14 as a legitimate macro

When `rustc` is invoked, the macro is included with the `--check-cfg`
flag, but not with the `--cfg` flag. This caused errors about duplicate
definitions to spew out when building with stable Rust toolchains.

* Revert "Actually add Py_3_14 as a legitimate macro"

This reverts commit 5da57af.

* Fix version macro placement for 3.14-specific getters and setters

* Import 'c_ushort' only if compiling against CPython 3.14 or later

* Add wrapper functions for the statically_allocated field

* Remove unused libc::c_ushort

* Add (hopefully) final version-specific macros

* Port 3.14-specific 64-bit code of Py_INCREF

* Don't expose PyDictObject.ma_version_tag when building against 3.14 or later

* fix ffi-check on the GIL-enabled ABI

* fix older pythons

* fix ffi-check on older pythons

* WIP: update for 3.14t

* fix ffi-check on the free-threaded build

* fix clippy

* fix clippy on older python versions

* fix cargo check on the MSRV

* fix ffi-check on 3.13t

* fix CI which is using 3.13.1

* fix copy/paste error in noxfile

* update ffi bindings for the latest changes in 3.14

* update layout of refcnt field on gil-enabled build

* delete unused HangThread struct

* fix ffi-check on GIL-enabled build

* Revert "delete unused HangThread struct"

This reverts commit 3dd439d.

* config-out HangThread

* fix 3.13 ffi-check

* fix debug python build error

* fix graalpy build

* Ignore DeprecationWarnings from the  pytest_asyncio module in tests

* Add abi3-py314

* fix free-threading issue in `test_coroutine` (PyO3#5069)

* Introspection: add function signatures (PyO3#5025)

* Introspection: add function signatures

No annotations or explicit default values yet

Fixes an issue related to object identifiers path

* Better default value

* Refine arguments struct

* Introduce VariableLengthArgument

* Adds pyfunctions tests

* Adds some serialization tests

* respond to david's code review

* add comment and fix test failure

* fix check-feature-powerset

* fix clippy

* fix wasip1 clippy

* fix 32 bit python 3.14 bug

* mark test-py step continue-on-error for dev python builds

* use github issue URL

* run ffi-check before running tests

* fix ffi-check for 3.14.0a7

---------

Co-authored-by: Nathan Goldbaum <[email protected]>
Co-authored-by: David Hewitt <[email protected]>
Co-authored-by: Thomas Tanon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-skip-changelog Skip checking changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants