Allow ivar names to be elided

[silvanshade]

This PR makes the ivar names optional (re: my earlier comment).

I attempted to do the same for the ivar helper module but it seemed problematic without more serious refactoring of the macros. The reason being that you wouldn't want to create the helper module if there were no ivars specified, but this is hard to detect without more intermediate macros.

One thought I had though is that it feels like many of the macros are rather complicated. I wonder if it might not be worth considering rewriting many of them as proc-macros.

There are the usual downsides to that approach, but on the other hand, it would make some things much easier to implement (like allowing the ivar helper module to be optional) and it would allow us much more control over what kind of error messages we could produce.

I also wonder, especially given the size of icrate, whether it might make an impact in terms of performance. As I note in #393, there's at least one framework (Matter) that I couldn't get to finish compiling even after raising the recursion limit significantly (up to 2048). In that case I think the issue was partly due to a giant enum, which with a proc macro, we could handle with iteration rather than recursion.

I realize it would be quite a lot of work to rewrite those but that's something I would be willing to help with if you think it's a good idea.

[madsmtm]

Linking your comment here: 42b9538#r98502810.

I was just wondering, but is there a rationale for needing to be able to specify these manually? I'm referring to the ivar name and the module name. Maybe they could just be created automatically in the macro invocation? Or perhaps at least allow a macro rule case where they can be elided?

For the module name, yes, they could probably be generated automatically without too much trouble by using the paste macro or similar - but that requires proc-macros, see below.

For the instance variable name, I provided a short rationale in #409, but I'll try to expand on it:

1. The name must be unique across all superclasses (this is something I only just now figured out). There is no way to statically know which names are taken, and that may even change in new OS versions, so e.g. in winit we'll probably have to do "winit_ivar".
2. The name will be present in the final binary, which is something that the user should be aware about
   • Users that want to "protect" their code / keep the binary size small are used to being able to strip debug info, and then be done with it. They will not be able to with this.
   • Users that want to give a better debugging experience would be unsatisfied with a long, auto-generated name.

But perhaps I'm being pedantic, and nobody actually cares what the name is? What do you think?

To avoid conflicts though, I'd probably prefer the name being something like concat!("_", class_name, "_", field_name) instead.

One thought I had though is that it feels like many of the macros are rather complicated. I wonder if it might not be worth considering rewriting many of them as proc-macros.

When I started this project, it was a direct competitor to objc, which meant that the extra compile-time for a procedural macro + syn + quote was out of the question (and I dismissed e.g. #[derive(Encode, RefEncode)] on those grounds as well: #55). Though I do admit, now that we have icrate, things have probably changed enough to reconsider.

Another issue with procedural macros is that they're less visible to IDEs, though I'm not sure to what extent this is, and what the recommendations are for overcoming this? Will probably at least require some work to allow the macro to always emit compile_error!(...) instead of panicking, such that type-information is available to IDEs in most cases.

would allow us much more control over what kind of error messages we could produce.

This is the biggest point for me, complex declarative macros have quite terrible UI.

And e.g. extern_methods! could be gone altogether if we had attribute macros method and method_id. Well, perhaps not, we'd still need a way to mark it as unsafe. But it could be #[extern_methods] unsafe impl ... instead. Would even allow putting #[extern_methods] inside declare_class!, which would save some repetition.

I also wonder, especially given the size of icrate, whether it might make an impact in terms of performance. As I note in #393, there's at least one framework (Matter) that I couldn't get to finish compiling even after raising the recursion limit significantly (up to 2048). In that case I think the issue was partly due to a giant enum, which with a proc macro, we could handle with iteration rather than recursion.

There are ways to deal with that, see TT munchers performance. For extern_enum! specifically, the current implementation was mostly just a hack, it will likely go away after #310.

I realize it would be quite a lot of work to rewrite those but that's something I would be willing to help with if you think it's a good idea.

I would be very interested in a proof-of-concept of replacing e.g. extern_methods! with a procedural macro (as a test it doesn't have to handle method vs. method_id and such, but one that does roughly the same work (beware of emitting todo!() or likewise, since that will allow the compiler to do less work)), and seeing what the effect on performance would be for a cargo build --package=icrate --features=Foundation_all - since that's the only real holdout for me.

[silvanshade]

Thanks for the detailed response, that clarifies a lot of things.

But perhaps I'm being pedantic, and nobody actually cares what the name is? What do you think?

Good question.

I think if it comes down to the choice between better ergonomics for typical, non-debugging use cases (i.e., you just want to declare a class and assume everything is going to work), versus being a little easier to read in the case where things go wrong and you do need to do the debugging, I would probably favor better ergonomics in the typical case.

So in that sense, if uniqueness is the issue, I would opt for some autogenerated naming scheme even if the results are a bit unwieldy. I suppose we could still allow the names to be optional in the case that the user really cares, right?

I would be very interested in a proof-of-concept of replacing e.g. extern_methods! with a procedural macro (as a test it doesn't have to handle method vs. method_id and such, but one that does roughly the same work (beware of emitting todo!() or likewise, since that will allow the compiler to do less work)), and seeing what the effect on performance would be for a cargo build --package=icrate --features=Foundation_all - since that's the only real holdout for me.

Okay, sounds goods. I will give that a try and we can see what the results look like.

[madsmtm]

So in that sense, if uniqueness is the issue, I would opt for some autogenerated naming scheme even if the results are a bit unwieldy. I suppose we could still allow the names to be optional in the case that the user really cares, right?

Yeah, I would be fine with that (though being able to specify it manually is very useful for the case where you want to use objc2 to interface with Rust from Objective-C, so allowing manual overriding would still be nice).

I did intentionally remove the ability to specify the class name automatically, since I suspect it's mostly just causes code-smells (e.g. in winit I am doing struct WinitView { ... } and struct WinitApplicationDelegate { ... }, which could've been shortened a lot) (and I don't think there's a way to query the current crate, such that we could autogenerate names there as well?).

[madsmtm]

Looking at it again, actually, I think I've been mistaken? If you use class_getInstanceVariable with the correct class, then you can access instance variables correctly no matter if two classes may have the same instance variable name.

In effect, I think it is just be our usage of class_getInstanceVariable that's incorrect, we use the dynamic class from obj.class(), instead of the static class from ClassType::class, which is what causes the undesired behaviour!

(This also means that usage of object_getInstanceVariable/object_setInstanceVariable/AnyObject::ivar/AnyObject::set_ivar should be heavily discouraged, since they all exhibit this problem!)

Was/am going to fix this PR as part of #414 anyhow, just writing down my thoughts for now though.