Generate `impl` blocks for associated enum functions #991

MarijnS95 · 2020-11-15T23:50:13Z

Leverage existing function generation for class/record to generate associated functions on enumerations as well, instead of having to write these by hand 🎉. The MR for gstreamer-rs can be found here. These changes came up during https://gitlab.freedesktop.org/gstreamer/gstreamer-rs/-/merge_requests/581.

This relies on functions being associated with the <enumeration> in .gir files as opposed to being free (same story with class/record), see https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/-/merge_requests/805 for an example where this was not the case.

Note that we have decided against turning the first argument from <parameter> into <instance-parameter> in Gir files (to make these functions behave like fn my_func(&self) instead of fn my_func(my_enum: Self)). This is instead overridden during analysis, it's a bit ugly so perhaps we need to revisit this discussion. It all depends on other tooling, and how many other languages (potentially) support functions on enum instances like Rust.

Apologies for finding out about SSR in the history of this repo and performing some refactorings 😬. Perhaps those are better submitted in a separate, easier-to-review PR?

Related PRs/MRs

gstreamer-rs: https://gitlab.freedesktop.org/gstreamer/gstreamer-rs/-/merge_requests/630

TODO

sdroege · 2020-11-16T07:45:14Z

The exact same code would also apply to flags types, or not? Can that be unified so (mostly) the same code can cover both?

Also if you could submit all the cleanup commits as a separate PR that would speed up reviewing here. The cleanups all look fine from what I saw so far and could be merged more or less immediately :)

sdroege · 2020-11-16T07:57:28Z

Can you also check what (if any) effects this has on https://github.com/gtk-rs/gtk-rs and submit a WIP PR?

MarijnS95 · 2020-11-16T08:58:10Z

The exact same code would also apply to flags types, or not? Can that be unified so (mostly) the same code can cover both?

I guess you mean bitfields (inconveniently called codegen/flags.rs)? Those should indeed share all the same semantics, I'll see if I can rework the current analysis to work for both and unify most of the codegen.

Also if you could submit all the cleanup commits as a separate PR that would speed up reviewing here. The cleanups all look fine from what I saw so far and could be merged more or less immediately :)

Pushed as #986, including some commits that change generated output (ie. f.write_str()) but are not fully relevant to this PR either.

Can you also check what (if any) effects this has on https://github.com/gtk-rs/gtk-rs and submit a WIP PR?

As mentioned in the edit above I or someone else will have to go through gtk and remove the same handwritten implementations that clash with generated functions, while also validating semantics of generated code (strings...).

sdroege · 2020-11-16T09:27:46Z

I guess you mean bitfields (inconveniently called codegen/flags.rs)

Yeah, bitfield is very confusing too because that's also a completely separate concept in C :)

sdroege · 2020-11-16T10:48:38Z

This needs to be rebased now

sdroege · 2020-11-18T07:30:13Z

Can you also check what (if any) effects this has on https://github.com/gtk-rs/gtk-rs and submit a WIP PR?

As mentioned in the edit above I or someone else will have to go through gtk and remove the same handwritten implementations that clash with generated functions, while also validating semantics of generated code (strings...).

That's OK for now, just submit a PR that does not remove the handwritten functions so we can easily compare what is generated there too :)

MarijnS95 · 2020-11-18T19:43:14Z

That's OK for now, just submit a PR that does not remove the handwritten functions so we can easily compare what is generated there too :)

@sdroege Removed them anyway because it was only one class, as well as fixing multiple doc issues that appeared when testing with --all-features.

There's a temp commit here to make sure the CI builds on the fixed gtk-rs branch. Once this gets closer I guess we'll merge gtk-rs, bump the commit off and make sure it succeeds one final time before merging?

MarijnS95 · 2020-11-18T22:31:47Z

@sdroege Pushed a simple yet unsustainable version of a safer, static .to_string implementation. We can discuss the details over at our thread on gstreamer-rs, I want to take this opportunity for something else: quote. Continuing onward from #992 (comment): this to_string is my first actual block of code in gir, and using writeln + format is horrible. I pushed a commit on top that replaces this block with a quote macro to show what it'd look like. It should probably not be part of this PR but I don't think it's particularly bad to restart the discussion of switching to this crate.

The benefit of having Display on TokenStream makes it a drop-in replacement for localized segments of code without requiring gir in its entirety to be converted, a massive win IMO.

The only problem I see right now is formatting in comments and macros (bitflags! {}), the rest is covered by fmt without ever thinking about indentation, \n and \t 😁

Further advantages:

Sort of syntax checking at compile-time (ie. no mismatched braces, but remember it's merely tokens);
Highlighting in an IDE;
Code looks way closer to the generated result;
No writeln, indentation or Option unpacking boilerplate;
Support for iterable expansion.

Conversion is pretty easy, too: copy an existing block of generated code (instead of tearing down writeln!) and substitute variable parts with #variable_to_expand.

src/analysis/enums.rs

src/codegen/enums.rs

sdroege · 2020-11-19T07:45:30Z

There's a temp commit here to make sure the CI builds on the fixed gtk-rs branch. Once this gets closer I guess we'll merge gtk-rs, bump the commit off and make sure it succeeds one final time before merging?

What's that commit and why? :)

sdroege · 2020-11-19T07:49:04Z

I pushed a commit on top that replaces this block with a quote macro to show what it'd look like. It should probably not be part of this PR but I don't think it's particularly bad to restart the discussion of switching to this crate.

Let's indeed do that as a separate PR afterwards. I agree that the string manipulations without quote are annoying. Last time I tried moving to quote the big problem was that it was rather annoying to a) do comments, b) format code in a reasonable way (e.g. quote didn't output empty lines between any items IIRC), c) even rustfmt was not converting the code into something reasonable looking.

If that can be solved I'm fine with moving to quote. But as said, different PR. Not too many things at once :)

src/analysis/enums.rs

src/codegen/object.rs

src/codegen/record.rs

src/analysis/enums.rs

sdroege · 2020-11-19T08:00:45Z

Looks ok to me otherwise. So

The comments above
Adding support for flags (which should be exactly the same)
Moving the quote stuff into a separate PR
Removing the commits from codegen/enum: Do not generate doc(cfg) on match arms #994 after that one is merged

Doesn't seem like too much missing here :)

MarijnS95 · 2020-11-19T23:29:37Z

There's a temp commit here to make sure the CI builds on the fixed gtk-rs branch. Once this gets closer I guess we'll merge gtk-rs, bump the commit off and make sure it succeeds one final time before merging?

What's that commit and why? :)

That commit switched the CI over to use my enum functions gtk-rs branch so that we can see the whole thing working and make sure the CI succeeds :)

I pushed a commit on top that replaces this block with a quote macro to show what it'd look like. It should probably not be part of this PR but I don't think it's particularly bad to restart the discussion of switching to this crate.

Let's indeed do that as a separate PR afterwards. I agree that the string manipulations without quote are annoying. Last time I tried moving to quote the big problem was that it was rather annoying to a) do comments, b) format code in a reasonable way (e.g. quote didn't output empty lines between any items IIRC), c) even rustfmt was not converting the code into something reasonable looking.

I'm glad to hear quote gets a second chance! The only issues with rustfmt are, as highlighted: not being able to format inside comments (duh) and inside macro calls, notably (actually, limited to) #[cfg] annotations inside bitflags! {}. It's sad but seems like we have to write quite a bit of Punct::new(..., Spacing::Joint) to clean those up. Same for commented out functions. I wonder if we can do a pre-formatting pass on these codeblocks before stringifying them into their respective // / bitflags! {} cages instead.

Guess it only makes sense to, for a first approach, convert (or implement new) tiny bits of gir in quote as long as they have no or little effect on the output. Over time this can be scaled up, but that's not a hard requirement at all :)

If that can be solved I'm fine with moving to quote. But as said, different PR. Not too many things at once :)

Of course, pushing it here was merely to showcase :D

sdroege · 2020-11-20T07:43:56Z

I'm glad to hear quote gets a second chance! The only issues with rustfmt are, as highlighted: not being able to format inside comments (duh) and inside macro calls, notably (actually, limited to) #[cfg] annotations inside bitflags! {}. It's sad but seems like we have to write quite a bit of Punct::new(..., Spacing::Joint) to clean those up. Same for commented out functions. I wonder if we can do a pre-formatting pass on these codeblocks before stringifying them into their respective // / bitflags! {} cages instead.

I think that can make sense. AFAIU you can call rustfmt also as a library so that might be worth looking into.

Guess it only makes sense to, for a first approach, convert (or implement new) tiny bits of gir in quote as long as they have no or little effect on the output. Over time this can be scaled up, but that's not a hard requirement at all :)

Agreed. But now let's first try to get this PR done and not put too much other noise in here :)

MarijnS95 · 2020-11-21T23:41:41Z

EDIT: Moved updated list to the main PR description. That's easier to find and shows a nice progess bar in the list of open pull requests.

src/analysis/imports.rs

sdroege · 2020-11-28T11:27:18Z

If there is an unknown value in a returned enum (fn from_*(xxx) -> EnumType), turn that into a Result<..., BoolError>;

What is this one about?

src/analysis/enums.rs

src/analysis/flags.rs

src/analysis/enums.rs

This is used in enum function generation where all enum types asssociated with a given crate are defined in the same file.

This not only cleans up unnecessary mutable variables in the codegen loop and a cloned Imports instance (so that it can remain immutable during codegen), it also adds the missing rejection logic to enum analysis. No harm was caused because these checks happened at codegen prior, but they omit some unnecessary imports.

This function is responsible for generating a Display impl, and caused unused std::fmt import previously.

sdroege · 2020-12-06T09:44:23Z

@GuillaumeGomez On you now, this one is "1 change requested" from you :)

sdroege · 2020-12-06T09:58:32Z

Actually pending a decision on https://gitlab.freedesktop.org/gstreamer/gstreamer-rs/-/merge_requests/630#note_719829 from my side

Analysis was copied from enums which adds an std::fmt when generate_display_trait is true, but the codegen for it was missing. Instead of removing the import, provide a simple Display trait that forwards to bitflags' Debug implementation.

When get_name or to_string is available and adheres to the right conditions a Display impl leveraging this C implementation is generated. This will clash with a pure Rust impl if generate_display_trait is true which should be disabled to prefer the C function call instead.

Complements: 5ce31f2 ("codegen: Add version condition on special function traits")

src/analysis/special_functions.rs

sdroege

Still looks good to me. GStreamer generated code is ready too

GuillaumeGomez · 2020-12-07T10:55:57Z

No more .unwrap() in the generated code so I'm happy.

MarijnS95 mentioned this pull request Nov 16, 2020

Simplify match blocks into ? operator using SSR #992

Merged

MarijnS95 mentioned this pull request Nov 18, 2020

Replace manual enum functions with autogenerated ones gtk-rs/gtk3-rs#155

Merged