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

proc macro proof-of-concept #425

Closed
wants to merge 1 commit into from

Conversation

silvanshade
Copy link
Contributor

I started rewriting the proc-macro proof-of-concept from scratch based on ideas from #423 and from objrs and some things I learned from the first attempt. This is still just a sketch of course so pretty anything can be changed.

I haven't added a lot of the class related machinery back in yet but will get back to that soon.

One thing I did try that was new this time was some ideas for more thoroughly handling the Objective-C enum macros (e.g., NS_ENUM and related). I'm not entirely sure this approach will work but it seems like it might be a little nicer than what we currently have. See the test file for examples.

@silvanshade silvanshade force-pushed the proc-macros branch 5 times, most recently from a71aa52 to 3167015 Compare February 17, 2023 01:30
@silvanshade silvanshade changed the title work-in-progress proc macro proof-of-concept Feb 17, 2023
@silvanshade silvanshade force-pushed the proc-macros branch 11 times, most recently from e751cea to 192b167 Compare February 19, 2023 04:20
Copy link
Owner

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

Quick notes:

  • Did you get around to checking the compilation-speed difference yet?
  • To merge this, I would probably need one PR for each macro being replaced (where it makes sense, ns_enum, ns_options, ns_error_enum, ... can go in the same PR)
  • Ideally actually, I would prefer each macro being replaced by a function-like proc-macro that shares the exact same syntax, and then we can work on changing the syntax afterwards - but I recognize that may be a lot more work (and less glamorous), so it's okay if you don't want to do that.
  • Wrt. the implementation, I would like it if we could start with a solid error reporting foundation using something like proc-macro-error, or perhaps manually using compile_error!.

@@ -164,7 +164,7 @@ jobs:
- name: Install Rust toolchain
uses: actions-rs/toolchain@v1
with:
toolchain: '1.60'
toolchain: '1.62'
Copy link
Owner

Choose a reason for hiding this comment

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

MSRV has to follow winit (which is currently at 1.60)

Comment on lines 9 to 10
#[objc(interface)]
struct C {};
Copy link
Owner

Choose a reason for hiding this comment

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

The macro syntax here is unsound, since there's no unsafe marker to prevent the user from specifying the wrong superclass(es). We'd need to do something else, perhaps #[objc(interface, unsafe)] or similar.

@silvanshade
Copy link
Contributor Author

  • Did you get around to checking the compilation-speed difference yet?

I haven't yet but hopefully will be able to start testing that in the upcoming week.

I did notice this blog post, though, which you may also have seen. Although the comparison to proc-macros isn't explicitly made, it does suggest (I think) that there are some significant improvements to be had.

  • Ideally actually, I would prefer each macro being replaced by a function-like proc-macro that shares the exact same syntax, and then we can work on changing the syntax afterwards - but I recognize that may be a lot more work (and less glamorous), so it's okay if you don't want to do that.

Okay, I will keep this in mind. Since I didn't start doing it this way, I would prefer to wait until after the underlying implementation stabilizes and then perhaps go back and reimplement the current macro syntax using that.

  • To merge this, I would probably need one PR for each macro being replaced (where it makes sense, ns_enum, ns_options, ns_error_enum, ... can go in the same PR)

Understood. The main reason I am flattening the commit history right now is mostly because it's rather noisy. For the final PR I can provide more granularity there.

  • Wrt. the implementation, I would like it if we could start with a solid error reporting foundation using something like proc-macro-error, or perhaps manually using compile_error!.

Right, I am currently using proc-macro-error for this. Most of the individual functions return syn::Error and then at the top level this is converted to a compiler error using proc-macro-error. I've also been paying attention to trying to provide decent error location information and messages as well.

@silvanshade silvanshade force-pushed the proc-macros branch 5 times, most recently from 7aaee78 to db65bd4 Compare February 20, 2023 02:11
@madsmtm
Copy link
Owner

madsmtm commented Feb 20, 2023

Right, I am currently using proc-macro-error for this. Most of the individual functions return syn::Error and then at the top level this is converted to a compiler error using proc-macro-error. I've also been paying attention to trying to provide decent error location information and messages as well.

Ah, sorry, I should've checked the code if you were doing so already ;)

@silvanshade silvanshade force-pushed the proc-macros branch 16 times, most recently from ade42e3 to 554d293 Compare February 27, 2023 01:11
@silvanshade silvanshade force-pushed the proc-macros branch 7 times, most recently from ec4b745 to 314b920 Compare March 2, 2023 02:27
@silvanshade
Copy link
Contributor Author

@madsmtm I've gotten an initial implementation working and compiling icrate now. Basically I replaced extern_class, extern_methods, extern_protocol, and then the various enum macros.

With the enum macros, I copied the existing behavior (not the more advanced translation we've been discussing). The goal there was simply to avoid recursive macro_rules style macros for the sake of efficiency.

Compile times seem several seconds faster, but not as significant as I was expecting. That surprised me a little but I haven't investigated further yet or done thorough testing.

I did try compiling icrate with importing the Matter framework, which with the old-style macros required raising the recursion limit to around 2000 (and still not finishing). But in this case it also didn't finish (although it kept compiling at the default recursion limit at least).

I think that even if it turns out that with proc-macros it isn't much faster with regard to icrate compile times, there's still benefits with regard to macro maintainability.

There are also some other possibilities to explore that might actually have a bigger impact on performance, like switching from syn to venial, or trying to pre-compile the proc-macros via watt.

Another possibility could be to simply dispense with the macros altogether and use the underlying proc-macro code directly in header-translator for code generation.

Anyway, I probably won't do too much more work in this direction unless you think it's worth exploring further.

@madsmtm
Copy link
Owner

madsmtm commented Mar 2, 2023

Hmm, the part about non-improved performance is unfortunate. Did you try running e.g. cargo clean -picrate && cargo check -picrate --features=... a few times, to check the performance improvements to icrate in particular?

There might also be improvements found by compiling the objc2-proc-macro in release-mode instead:

[profile.dev.build-override]
opt-level = 3
# OR
[profile.dev.package.objc2-proc-macros]
opt-level = 3

Another possibility could be to simply dispense with the macros altogether and use the underlying proc-macro code directly in header-translator for code generation.

The possibility of doing the code-generation from the get-go inside header-translator is indeed also something I've considered, though the nice thing about extern_class!/extern_protocol! is that it creates an abstraction boundary that allows us to decouple objc2 and icrate versions a bit more.


I agree there's huge benefits to a proc-macro, especially improvements to declare_class! would be noticeable, basically everything in #428 would be much easier to implement with proc-macros.

Anyway, I probably won't do too much more work in this direction unless you think it's worth exploring further.

Understandable - I do think it's worth exploring further at some point, but as I've said before, my priorities do lie elsewhere right now. E.g. I'm more interested in getting the semantics / the output of the macros right first, and then we can always create the better user-interface proc-macros afterwards (and simply deprecate the old macros as aliases for the new ones).

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.

2 participants