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

Add from_py_with attribute #1411

Merged
merged 1 commit into from
Feb 20, 2021
Merged

Conversation

daniil-konovalenko
Copy link
Contributor

@daniil-konovalenko daniil-konovalenko commented Feb 1, 2021

This PR addresses #1239.

It adds a #[pyo3(from_py_with = "function")] attribute that allows to customize conversion between Python-native and Rust-native types.
At the moment only argument attributes are supported. I plan on adding support for derive in the near future.

I've made a couple of design decisions along the way, that can be changed if needed:

  • I chose from_py_with instead of from_python_with from the original proposal because it was shorter to type. Also it goes well with the trait name FromPyObject.
  • The path to the conversion function is quoted instead of being a raw identifier, mainly because serde does it. If there are any reasons it shouldn't be quoted, please let me know.

There's still a bunch of stuff left to do:

@nw0
Copy link
Contributor

nw0 commented Feb 2, 2021 via email

@davidhewitt
Copy link
Member

Thanks very much for this PR. I'm very excited to see it!

I'm very busy at the moment though hope to get a moment to read it by the weekend and share my full thoughts and feedback then.

Copy link
Member

@kngwyu kngwyu left a comment

Choose a reason for hiding this comment

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

Thanks!
I think the API is fine, but there's some room for improvements in the implementation (can we reduce mut)?

pyo3-macros-backend/src/pyfunction.rs Outdated Show resolved Hide resolved
pyo3-macros-backend/src/pyfunction.rs Outdated Show resolved Hide resolved
@daniil-konovalenko
Copy link
Contributor Author

daniil-konovalenko commented Feb 8, 2021

Just bikeshedding: would you consider 'from_py'?

I think it's worth considering, but I personally prefer from_py_with more because it feels more familiar to me.

In any case, I feel that the final naming decision should be made by the maintainers, as they have much better understanding of pyo3's API as a whole than I do

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 again for this, and sorry for the delay from me getting round to reading it!

The implementation looks good and has some decent test coverage; I added a few comments on places in the code where I have some questions and ideas to improve it a bit.

Regarding name - I think you're correct that from_py_with is better than from_python_with.

I also think that from_py_with is better than from_py, as we're keeping in line with the precedent made by serde.

Would be nice to hear if @kngwyu has any strong preference on name also.

tests/test_class_basics.rs Outdated Show resolved Hide resolved
pyo3-macros-backend/src/method.rs Outdated Show resolved Hide resolved
pyo3-macros-backend/src/from_pyobject.rs Outdated Show resolved Hide resolved
pyo3-macros-backend/src/method.rs Outdated Show resolved Hide resolved
pyo3-macros-backend/src/module.rs Outdated Show resolved Hide resolved
pyo3-macros-backend/src/pyfunction.rs Outdated Show resolved Hide resolved
pyo3-macros-backend/src/pyfunction.rs Outdated Show resolved Hide resolved
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 for the continued work on this, this is looking increasingly solid.

I took another read just now and had a few more ideas...

pyo3-macros-backend/src/attrs.rs Outdated Show resolved Hide resolved
pyo3-macros-backend/src/attrs.rs Outdated Show resolved Hide resolved
pyo3-macros-backend/src/pyfunction.rs Outdated Show resolved Hide resolved
pyo3-macros-backend/src/pymethod.rs Outdated Show resolved Hide resolved
pyo3-macros-backend/src/pyproto.rs Outdated Show resolved Hide resolved
tests/test_pyfunction.rs Show resolved Hide resolved
@daniil-konovalenko
Copy link
Contributor Author

daniil-konovalenko commented Feb 11, 2021

CI just got updated to Rust 1.50 and it broke formatting, plus there's a couple of new Clippy rules...

@davidhewitt
Copy link
Member

👍 I'll fix in a bit and you can rebase this PR on it after

@daniil-konovalenko
Copy link
Contributor Author

Awesome, thank you!

@davidhewitt
Copy link
Member

See #1422 - just waiting for a second pair of eyes on it...

@davidhewitt
Copy link
Member

#1422 is merged, so if you rebase things should work now!

@daniil-konovalenko
Copy link
Contributor Author

Rebased, I hope it wouldn't mess up the comments too much

@davidhewitt davidhewitt linked an issue Feb 14, 2021 that may be closed by this pull request
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, the implementation looks great to me!

I'm happy with the design here, I think this is good to merge. @kngwyu are you also happy with this?

I see that the TODO list still includes CHANGELOG entry and docs.

I think docs should go in the guide. TBH the #[name = ...] attribute is also undocumented; I was going to move it to #[pyo3(name = ...)] and add a guide section to document attributes better at the same time. If you like you can just open an issue to document #[pyo3(from_py_with)] and I'll do it at the same time.

CHANGELOG entry - I suggest something like this:

- Add #[pyo3(from_py_with = "...")]` attribute for function arguments and struct fields to override the default from-Python conversion. [#1411](https://github.com/PyO3/pyo3/pull/1411)

tests/test_class_basics.rs Show resolved Hide resolved
some_object: String,
}

#[test]
Copy link
Member

Choose a reason for hiding this comment

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

Final thought; could we please add tests for tuple structs and enum variants? Just to make sure that all cases are covered and remain covered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, thank you for the reminder!

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've finally got to this and I have a question. At the moment we provide from_py_with for struct fields only.
Do you have in mind that from_py_with should be supported for tuple structs and enums as well?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, that's a good point, I didn't really specify those on #1239 when I proposed the idea.

I think for tuple structs with #[derive(FromPyObject)] it's quite natural to want to be able to annotate fields in the same way. Maybe let's skip on deciding what to do with enum variants for now. That'll be easy to add later anyway.

If tuple structs are going to be a lot of extra work, perhaps let's merge this PR for now and add an issue to remember to support them later?

Copy link
Contributor Author

@daniil-konovalenko daniil-konovalenko Feb 19, 2021

Choose a reason for hiding this comment

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

Thanks for clearing this out! I'll try supporting tuple structs, maybe it won't be that hard.
In any case, I think it's worth adding a compiler error (warning?) when from_py_with is used on types that are not yet supported

@daniil-konovalenko
Copy link
Contributor Author

daniil-konovalenko commented Feb 15, 2021

@davidhewitt
Thank you for the thorough review!
I like the changelog line that you proposed, I think it summarizes the changes really well.
I'll try to fix the remaining issues on the following week, probably closer to the weekend.
If you'll be able to update the guide it would be awesome! I've created an issue for this: #1432.

I'm still a little bit bummed about this:

let inputs_empty = sig.inputs.is_empty();
let sig_span = sig.span();
let inputs_span = sig.inputs.span();

I have tried a few things but I can't seem to fix it completely.
My approach was to split up FnSpec::parse into different functions, it helped to find out which parts of the parsing code use which references.
Turns out the main problem is this:

  1. an immutable reference to sig must be valid during the whole FnSpec::parse
  2. we need sig.inputs.iter() to parse fn_spec etc.
  3. we need sig.inputs.iter_mut() to parse attributes.
  4. sig.inputs.iter_mut() can't live at the same time as &sig.inputs.iter()

If we just use the mutable iterator everywhere, then we would need the additional variables listed above. Also it introduces unnecessary mutability to parse_method_receiver.

The other approach I tried is this:

  1. Use an immutable iterator to parse fn_type. Count how many elements (N) from sig.inputs we used.
  2. Obtain a mutable iterator and skip the first N elements, then parse the rest of the arguments.
    This approach is somewhat awkward. I've got it to compile, but it does not pass all the tests yet.

There's also the third approach:

  1. Parse everything immutably
  2. remove attributes from the arguments.
    see Add from_py_with attribute #1411 (comment) for the details.
    I'm not a fan of this one because it would be easy to forget to remove attributes. Also it adds an implicit dependency between the parsing code and the removal code.

At this point I'm stuck. I would love to hear any ideas that you have on this.

@kngwyu
Copy link
Member

kngwyu commented Feb 15, 2021

Would be nice to hear if @kngwyu has any strong preference on name also.

No, I'm fine with the current name.

@kngwyu
Copy link
Member

kngwyu commented Feb 15, 2021

I am now still playing in the code but will leave some suggestions in a day.

@daniil-konovalenko
Copy link
Contributor Author

daniil-konovalenko commented Feb 15, 2021

I've pushed the second approach implementation. It's awkward around skipping arguments, but otherwise I like it more than what I had before.

Copy link
Member

@kngwyu kngwyu left a comment

Choose a reason for hiding this comment

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

Thanks, your last commit nicely simplifies the code. 👍🏼
I left some comments to make the intention clearer.

pyo3-macros-backend/src/method.rs Outdated Show resolved Hide resolved
pyo3-macros-backend/src/method.rs Outdated Show resolved Hide resolved
pyo3-macros-backend/src/method.rs Outdated Show resolved Hide resolved
pyo3-macros-backend/src/method.rs Outdated Show resolved Hide resolved
@daniil-konovalenko
Copy link
Contributor Author

@kngwyu Thanks for the suggestions! It looks a lot better now

Copy link
Member

@kngwyu kngwyu left a comment

Choose a reason for hiding this comment

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

Thanks, I think now it's ready 👍🏼

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.

This all looks great to me, thanks for your hard work on this. The implementation of FnSpec::parse is a lot better now, really nice job.

Few final nits and then let's merge this. It could also be worth squashing this down to one or two commits from 22.

CHANGELOG.md Outdated Show resolved Hide resolved
pyo3-macros-backend/src/method.rs Outdated Show resolved Hide resolved
* allow from_py_with inside #[derive(FromPyObject)]
* split up FnSpec::parse
@kngwyu
Copy link
Member

kngwyu commented Feb 20, 2021

add tests for enums/tuple structs

Is this the only blocker?

@daniil-konovalenko
Copy link
Contributor Author

Is this the only blocker?

I forgot to update the TODO: I still need to implement from_py_with for tuple structs and enums, but it can be deferred to a future PR.
See our discussion with David:
#1411 (comment)

@kngwyu
Copy link
Member

kngwyu commented Feb 20, 2021

I see, thank you!

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

Successfully merging this pull request may close these issues.

Add #[pyo3(from_python_with = ...)] attribute
4 participants