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

refactor pyproto internals #1270

Merged
merged 1 commit into from
Nov 13, 2020
Merged

Conversation

dvermd
Copy link
Contributor

@dvermd dvermd commented Nov 10, 2020

This PR aims to refactor MethodProto.

make test passed OK

Still to do:

  • an entry in CHANGELOG.md
  • Free methods

Closes: #1117

use std::fmt::Display;

pub fn print_err(msg: String, t: TokenStream) {
Copy link
Member

Choose a reason for hiding this comment

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

Good catch 👍

@kngwyu
Copy link
Member

kngwyu commented Nov 11, 2020

Quite a nice refactoring, thanks.
I think your approach to Free methods is correct. They don't have type Result = ....

How about adding some constructors to MethodProto? For example, with_result: false is only used for GC methods, so we can use a 'short-cut' constructor for the common case.

@kngwyu
Copy link
Member

kngwyu commented Nov 11, 2020

BTW, I came up with a shorter modify_arg_ty:

fn modify_arg_ty(
    sig: &mut syn::Signature,
    idx: usize,
    decl1: &syn::FnArg,
    decl2: &syn::FnArg,
) -> syn::Result<()> {
    let arg = sig.inputs[idx].clone();
    match arg {
        syn::FnArg::Typed(ref cap) if crate::utils::option_type_argument(&*cap.ty).is_some() => {
            sig.inputs[idx] = fix_name(&cap.pat, &decl2.inputs[idx])?;
        }
        syn::FnArg::Typed(ref cap) => {
            sig.inputs[idx] = fix_name(&cap.pat, &decl1.inputs[idx])?;
        }
        _ => return Err(syn::Error::new_spanned(arg, "not supported")),
    }

    Ok(())
}

@dvermd
Copy link
Contributor Author

dvermd commented Nov 11, 2020

How about adding some constructors to MethodProto? For example, with_result: false is only used for GC methods, so we can use a 'short-cut' constructor for the common case.

I thought and tried some constructors but then the declarations in defs.rs won't be that explicit without constructors' arguments names. If you're OK with that I can create them.

Else, there's the builder pattern that can be a solution builder(name, proto).args(&[...]).with_self().with_result().

Which one do you like better ?

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 awesome to me! Lovely to see a great reduction in LOC and overall it's much easier to understand the affected files.

The .has_self() builder method is really nice as it'll make it super easy to refactor the self-types later as discussed on the issue.

Regarding CHANGELOG entry, I don't think this affects users so imo it shouldn't have an entry.

Comment on lines 35 to 32
pub const fn args(self, args: &'static [&'static str]) -> MethodProtoBuilder {
let mut with_args = self;
with_args.args = args;
with_args
}
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 you can probably take mut self in each of these builder methods, e.g.

Suggested change
pub const fn args(self, args: &'static [&'static str]) -> MethodProtoBuilder {
let mut with_args = self;
with_args.args = args;
with_args
}
pub const fn args(mut self, args: &'static [&'static str]) -> MethodProtoBuilder {
self.args = args;
self
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I didn't think mut self was possible but the code is much nicer this way.

@@ -66,6 +66,7 @@ fn impl_proto_impl(
for iimpl in impls.iter_mut() {
if let syn::ImplItem::Method(ref mut met) = iimpl {
// impl Py~Protocol<'p> { type = ... }
// TODO: MethodProto.result = meth.sig.output
Copy link
Member

Choose a reason for hiding this comment

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

Not sure what this TODO is asking to happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This TODO was forgotten there. It's removed.

@davidhewitt
Copy link
Member

Regarding MSRV failure - I think there's now maybe enough reasons for us to bump to rust 1.45, so I'm probably going to open a PR for that later.

@davidhewitt davidhewitt mentioned this pull request Nov 12, 2020
@davidhewitt
Copy link
Member

Looks great, thanks! If you squash and rebase on master I think you should find that MSRV CI starts working.

I notice this PR is still in draft, but I think other than a rebase it's now hopefully ready to be merged?

@dvermd dvermd force-pushed the refactor_MethodProto branch 2 times, most recently from f88029d to 9b9ca2c Compare November 12, 2020 20:54
@dvermd dvermd marked this pull request as ready for review November 12, 2020 20:54
@dvermd
Copy link
Contributor Author

dvermd commented Nov 12, 2020

Squashed, rebased, removed a use comment in defs.rs and un-drafted.

Still some problems with MSRV and 1.45. The right commit made it to 1.46

@davidhewitt
Copy link
Member

davidhewitt commented Nov 13, 2020

Ugh.

Based on this comment

This PR stabilizes these casts (which are already stable in const outside const fn)

I wonder if this works as a workaround:

// TODO: workaround for no unsized casts in const fn on Rust 1.45 (stable in 1.46)
const EMPTY_ARGS: &[&str] = &[];

impl MethodProtoBuilder {
    pub const fn new(name: &'static str, proto: &'static str) -> Self {
        MethodProtoBuilder {
            name,
            proto,
            args: EMPTY_ARGS,
            with_self: false,
            with_result: true,
        }
    }
}

If you don't have a chance before I do, I might push a commit to this branch tonight to try that.

@davidhewitt
Copy link
Member

... or I guess if that doesn't work, maybe could just always take args as an input to the const fn new.

@kngwyu
Copy link
Member

kngwyu commented Nov 13, 2020

Just a nit, but your EMPTY_ARGS can be an associated constant, like:

impl MethodProtoBuilder {
    const EMPTY_ARGS: &[&str] = &[];
...
}

@kngwyu
Copy link
Member

kngwyu commented Nov 13, 2020

Why use build?
It looks like that MethodProtoBuilder and MethodProto are the same and I don't see any reason for defining the builder as another struct.

@dvermd
Copy link
Contributor Author

dvermd commented Nov 13, 2020

impl MethodProtoBuilder {
    const EMPTY_ARGS: &[&str] = &[];
...
}

This workaround works !

Why use build?
It looks like that MethodProtoBuilder and MethodProto are the same and I don't see any reason for defining the builder as another struct.

I merged the MethodProtoBuilder into the MethodProto and removed the build() method.

@davidhewitt
Copy link
Member

It looks like that MethodProtoBuilder and MethodProto are the same and I don't see any reason for defining the builder as another struct.

👍 good spot

I merged the MethodProtoBuilder into the MethodProto and removed the build() method.

🎉 this refactoring is really great; it's reduced LOC by just over 50% 2 for this part of the codebase. Thanks so much for your work on this!

@davidhewitt davidhewitt merged commit 8b61830 into PyO3:master Nov 13, 2020
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.

Refactor pyproto internals
3 participants