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

Simplify match blocks into ? operator using SSR #992

Merged
merged 10 commits into from
Nov 16, 2020

Conversation

MarijnS95
Copy link
Contributor

Extracted from #991

While addressing minor code inconsistency I stumbled upon a commit from Heftig using rerast for structural refactoring, and it turns out Rust-analyzer supports SSR as well. This made it possible to easily do a bulk of conversions to use ? instead of manual match blocks. See the relevant commits for the rules used.

One minor deficiency is the inability to repeat labels: it is impossible to specify Some($a) => $a, meaning that a manual check is necessary to see if the right hand side wasn't doing any extra conversion. These had to be addressed by hand, but weren't many.

"impl fmt::Display for {0} {{
\tfn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {{
\t\twrite!(f, \"{0}::{{}}\", match *self {{
",
Copy link
Member

Choose a reason for hiding this comment

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

Why? It looks nicer to have it aligned properly and the output is the same

Copy link
Contributor Author

@MarijnS95 MarijnS95 Nov 16, 2020

Choose a reason for hiding this comment

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

I don't find \n\ particularly readable, but then again that allows to have leading spaces as indentation.

Would this work? (Notice the "\):

            "\
impl fmt::Display for {0} {{
\tfn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {{
\t\twrite!(f, \"{0}::{{}}\", match *self {{
",

That also allows \t to be substituted with four spaces, the native indentation.

Copy link
Member

Choose a reason for hiding this comment

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

That would also work for me (also with four leading spaces).

Just it would be nice to have the code in the string consistently indented, otherwise it's confusing to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also with four leading spaces

That would resurface the need for trailing \n\ again. I'll remove these changed from the PR as they're merely extra noise.

Copy link
Member

Choose a reason for hiding this comment

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

Ok :) Maybe let's handle that separately then, @GuillaumeGomez probably also has opinions on this topic

Copy link
Member

Choose a reason for hiding this comment

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

Not much, what matters is that it's readable inside the gir code. The autogenerated code itself will be cleaned up anyway, as you say.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that'd improve a lot with quote! and the way it can expand arrays or iterators, and in general colorize target code in the IDE. But it appears the need for // comments with nested /**/ completely breaks this unfortunately :(.

Copy link
Member

Choose a reason for hiding this comment

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

No, quote! didn't improve the readability at all.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I tried to use quote here but it only made things worse in practice. Maybe I just used it wrongly though...

Copy link
Contributor Author

@MarijnS95 MarijnS95 Nov 16, 2020

Choose a reason for hiding this comment

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

I'd argue:

        let type_name = format_ident!("{}", enum_.name);

        let members = members.iter().map(|m| {
            let name = &m.name;
            let ident = format_ident!("{}", name);
            // TODO: version_condition_tokens(env, m.version, false, 3) -> Option<TokenStream>
            quote!(Self::#ident => #name)
        });

        quote! {
            impl fmt::Display for #type_name {
                fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
                    match *self {
                        #( #members, )*
                        _ => "Unknown",
                    }
                }
            }
        }

Is way easier to read than:

        writeln!(
            w,
            "impl fmt::Display for {0} {{\n\
             \tfn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {{\n\
             \t\twrite!(f, \"{0}::{{}}\", match *self {{",
            enum_.name
        )?;
        for member in &members {
            version_condition(w, env, member.version, false, 3)?;
            writeln!(w, "\t\t\t{0}::{1} => \"{1}\",", enum_.name, member.name)?;
        }
        writeln!(
            w,
            "\t\t\t_ => \"Unknown\",\n\
             \t\t}})\n\
             \t}}\n\
             }}\n"
        )?;

But then there are quite a few things that are not ergonomic:

  • Not able to expand members in-line (ie. #(m.name));
  • Not able to convert strings to identifiers within the macro.

Not to mention some of the issues you were finding in #919, which "simply" needs some extra tooling around.

These in turn make it impossible to have something like (pseudo-syntax) #( Self::#(/*turn into identifier*/ #(members).name) => #(#(members).name), )* directly in place of the map.

Other than that the ability to expand iterators and deal with Option<> in expansion is pretty neat.

Copy link
Member

@sdroege sdroege left a comment

Choose a reason for hiding this comment

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

Looks good to me otherwise, thanks!

@MarijnS95 MarijnS95 force-pushed the simplification-refactor branch from 562246d to cf16130 Compare November 16, 2020 10:11
Copy link
Member

@sdroege sdroege left a comment

Choose a reason for hiding this comment

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

The PR looks good for me now, let's worry about those string formatting details some other time.

@GuillaumeGomez ?

@sdroege sdroege merged commit 9f4a439 into gtk-rs:master Nov 16, 2020
@GuillaumeGomez
Copy link
Member

Thanks!

@MarijnS95 MarijnS95 deleted the simplification-refactor branch November 16, 2020 11:06
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.

3 participants