Macros in macros

[Bromeon]

Our proc-macros don't work very well if the body contains declarative macros. Sometimes, other (non-gdext) attributes like #[allow(...)] also cause problems.

This should be addressed, possibly needs upstream venial changes.

Ideally, the order of attribute macros should not matter, either.

#[rustfmt::skip]
#[itest]
fn test_xy() {}

#[itest]
#[rustfmt::skip]
fn test_xy() {}

[patataofcourse]

Is something like #[gdext_attr(decl_macro!())] included under this issue / bug report? I'm not sure if "the body" means the body of an attribute, the body of an item a derive/attr is applied to, or something else.

[Bromeon]

Good point! Macros inside attributes are currently not supported either, but I'm not sure if we want to support them at all. Compared to the other use cases, they seem quite rare.

[patataofcourse]

Yeah, that's why I wanted to clarify - the lack of them makes attributes feel a bit less semantically Rust-y, but usually they appear as a result of hacks or workarounds for things that aren't in gdext yet, so I'm not sure if they're worth adding

[CenTdemeern1]

I was told to post this error I got here:

It happened when I added rustfmt::skip here:

[PgBiel]

A report on the state of things

So, I just spent quite some time debugging the #[godot_api] macros (in the plural because the implementations for #[godot_api] impl Struct and #[godot_api] impl ThingVirtual for Struct are separate) and I had some pretty enlightening conclusions (which also affect #399).

I was wondering why #[cfg], when placed under #[func] in a #[godot_api] impl Struct construction, seemed to "work", while it just completely failed when used in an implementation of virtual methods (as reported in #399).

The reason is this particular line:

gdext/godot-macros/src/class/godot_api.rs

Line 375 in 88a7934

found = new_found;

The TL;DR is that, when parsing #[godot_api] applied on inherent impl blocks (that is, on non-virtual impl blocks), the attribute extractor, which is the proc macro function which tries to find #[func] or similar attributes on impl entries, will keep searching for #[func]-like attributes even after finding the #[func]-like attribute (or #[signal] etc.) for a particular entry. While it makes sense to keep searching in order to detect weird situations such as #[func] #[func] or #[func] #[signal] and error on them, the problem is that, while searching, the found variable - holding info from the first found #[func]-like attribute - is overwritten - even when we find something that isn't a #[func]-like attribute.

This means that, when #[cfg] or any other attribute is placed below #[func], the attribute extractor will return None since it will analyze cfg and replace the found #[func]'s Some(BoundAttr::Func(...)) with None. In other words, #[cfg] under a #[func] doesn't "work" - it just makes the proc macro completely ignore the function, thus leaving it there as a normal function without any FFI "glue" whatsoever! In fact, this will happen for any unrecognized macro which appears under #[func] - the function will simply not be recognized as a Godot API function. And the result is that a compilation error will appear: Attribute macro #[func] not declared, because, since the function was ignored, the special (and undeclared) attribute #[func] was never removed by the #[godot_api] proc macro!

Then why does my PR #439 work (and not cause #[rustfmt::skip] to fail when under #[func])? Because it makes the attribute extractor completely skip #[macros::like::this], thus not stopping it from finding a #[func] or keeping a #[func] that it already found. Which means that, if a #[segmented::attribute] macro were to somehow delete the function like #[cfg] does, or even rename it, another compilation error will ensue - the same error we get when placing #[cfg] above #[func] - since, by the point Rust reads #[cfg] (when above #[func]) or #[some::deleting::macro] (when below #[func], which now 'works' since my PR was merged), or any other such macros, the FFI glue has already been generated assuming the function exists with its original name and parameters.

Therefore, #[cfg]-like macros (anything that can affect a function's signature) don't work at all in both usages of #[godot_api] - and the fact that a #[cfg] (or any other macro) below a #[func] in #[godot_api] impl Struct makes the method (or signal etc.) be totally ignored by #[godot_api] is a bug (and maybe even issue-worthy!).

(This also raises the question of whether we should even allow non-Godot FFI functions to appear in #[godot_api] impl blocks, since one can have multiple inherent impls for a single struct - in my opinion, we shouldn't, to avoid this kind of mistake, but that'd require some design discussion.)

---

Note that all of this happens because #[godot_api] applied on the impl is run before the proc macros on individual methods are applied (that is, before #[cfg], #[func] etc. are even expanded whatsoever). Which is good in a way, since it allows us to define "local-scoped" macros which don't actually exist and remove them later (as we do with #[func], #[signal], #[constant]). But it's bad because we can't allow the unrecognized macros to change the function's signature later, since, by then, we have already generated the function's FFI glue with the assumption that the function's signature would not change at all, as explained above.

Ideally, we could solve this by manually expanding the unrecognized attributes before generating the FFI glue. Sounds easy, right? Well, unfortunately, it isn't possible at the moment as it requires a currently unstable feature called proc_macro_expand; were it stable, this issue (#379), along with the related cfg issue (#399), would be instantly solved, since we'd have full power over proc macro expansions - on the #[godot_api] implementation, we'd just have to remove #[func] and expand the remaining attributes on the method until we reach a final function signature we can analyze and generate FFI glue for (or not, if it was removed along the way).

Until that feature is stabilized (if ever), though, we'll have to make do. An important conclusion here is that we can't currently work around the order of execution of proc macros in a sane way (currently, proc macros are evaluated "outside-in", that is, the impl macro goes first, and then the macros on the methods). Therefore, any fixes for this problem will necessarily be "incomplete" in a way at the moment.

In the following responses, I'll try to detail some possible solutions and ways forward, based on my research on this topic.

[PgBiel]

Possible solutions and conclusions

#[itest]

I'm listing this first since it's the initial example mentioned in this issue. #[itest] should be the most immediate macro to 'fix', since it is applied directly on the affected functions and not on impl blocks. Therefore, macros which appear above #[itest] will be applied before it (remember, outside-in). Thus, most macros already work with #[itest] when placed above it, as they will already have 'done their job' on the function (and those attributes are not even visible to #[itest] anymore).

The problem here lies with the attributes below #[itest], which are executed after it. Currently, #[itest] simply removes those attributes from the final function - which, in a way, can be wiser than blindly passing them through, as, in the worst case, that could lead to the function's signature being altered (or even removed) by the passed macro in spite of the additional test case 'glue' code generated by #[itest] (which assumes the function's signature doesn't change). This might, however, not be that much of a concern, since, in my local tests, it seems that #[cfg], the most relevant conditional compilation macro, takes priority over #[itest] (even when appearing below it), and we could just assume that other macros appearing under #[itest] are purely descriptive (such as #[rustfmt::skip]) and pass them through. Still, those macros could change the function signature, which is a concern - but then maybe they should just go above #[itest] (the #[itest] user would be notified with an error anyways).

In particular, though, it seems that #[doc] macros are always evaluated last, so they are always "under" #[itest], even if the docstring itself appears above it. That could be the largest motivator to allow attributes "under" #[itest].

---

As a solution for this, I initially thought of reordering the attributes when #[itest] wasn't the last one in the attribute list (having #[itest] place itself further below in the attribute list). The problem, however, is that, at least in my local testing, it seemed to compete with other macros in that sense and generate infinite recursion. In particular, certain macros (such as #[doc]) always want to be evaluated after #[itest]. So, I don't believe that this is possible.

Instead, we could consider the following solutions, based on the tools that are currently available to us:

1. Passing through all macros under #[itest] to the final function - this could work since the user would be notified with an error if they accidentally pass through some macro that modifies the signature (the solution would be just to move it above #[itest]).

2. Passing only certain macros under #[itest] through to the final test function, in a case-by-case basis, such as, most importantly, #[doc] - other macros under #[itest] wouldn't be passed through (current behavior) and would have to be specified above #[itest]. This approach is incomplete as certain attributes wouldn't ever appear in the final test function when that could be otherwise desired, but it could work for the vast majority of scenarios.

3. We could also consider adding a special attribute macro, such as #[passthrough(my_attr_macro)], to let the user force passing through some attributes to the final test function, but I think that's very unnecessary as users don't usually use #[itest] anyway.

[PgBiel]

#[godot_api]

For #[godot_api] macro(s), the main problem is regarding conditional compilation, as it's not as straight-forward as with #[itest] where #[cfg] always takes priority, and macros which don't affect methods' signatures can be safely passed through (such as #[rustfmt::skip]). Given that #[godot_api] macros are specified on impl blocks, a 'fix' for macros which potentially change (or remove) a function's signature is more complicated since all the unrecognized macros will necessarily run after the macro at the impl level has already fully executed. On the other hand, this does also give us some degree of power (since we can manipulate the macros before they run), which we could try to leverage in a potential solution.

Prior art: pyo3

I'd like to mention some prior work here regarding this problem we face. pyo3 is a crate which notoriously uses the magic of macros at impl blocks to declare Python classes, much like we do with GDExtension classes (it's very similar overall, actually!).

The important part is how they deal with conditional compilation inside an impl block (annotated with #[pymethods]).
In particular, as noted in PyO3/pyo3#498, it just didn't work in the past for e.g. #[cfg].

They dealt with this directly by just passing #[cfg] invocations through to the generated FFI glue (PyO3/pyo3#769). That is, the cfg macro was given an exception to be copied and pasted to the underlying FFI glue code generated by #[pymethods]. And, honestly, that's good enough for 99% of scenarios, as it's rare (albeit possible), in Rust, to use macros different from #[cfg] which perform conditional compilation, let alone macros which actually rename functions, or even add/remove parameters.

The alternative, for non-#[cfg] conditional compilation macros, is to use them directly on top of impl blocks, forcing them to run before #[pymethods] (or, in our case, #[godot_api]). However, that'd entail potentially disabling or changing all of your methods at once, when oftentimes you only want to conditionally compile one or another method. pyo3 doesn't have this problem, though, since they were smart and applied some (feature-gated) magic which allows methods exposed to Python to be split across multiple impl blocks. That is, one can specify multiple #[pymethods] impl blocks for a single pyclass declared in Rust. That is made possible mostly thanks to the inventory crate, which, however, has its own drawbacks, as it doesn't work in every supported Rust platform (it does work in all major desktop and mobile platforms - not sure about Web -, but seems to have some potential issues on MacOS: dtolnay/inventory#52). Additionally, it seems to be causing some linker problems for PyO3 users (PyO3/pyo3#341).

Related PyO3 problem (but slightly off-topic)

As a honorary mention, there are also two issues (PyO3/pyo3#780 and PyO3/pyo3#1003) which could also become relevant to us in the future: #[cfg_attr] can become a problem when used to conditionally compile certain attribute macros, such as (in that case) the #[pyclass] macro, as then it wouldn't parse the "local" attribute macros and they would generate an "unknown macro" error. The implemented workaround for this (see PyO3/pyo3#2692) added x_all (get_all and set_all) parameters to #[pyclass], such that the "local" macros were no longer needed when all one wanted was to annotate everything with that macro, thus allowing one to conditionally compile #[pyclass] itself without further trouble. This could be relevant if someone tried to e.g. conditionally compile #[godot_api] itself and not have errors on #[func], but that sounds very unlikely, so we should probably just ignore this for now.

Proposed solutions

For conditional compilation macros to work properly when using #[godot_api], we can consider the following:

1. (Easiest and most immediate solution) Just pass through #[cfg] attributes used on methods to their generated FFI glue code (much like the initial PyO3 solution). This would close Virtual function calls panic with not implemented if they are conditionally excluded from compilation #399 and is probably possible to implement right now with much value and without much trouble.

2. (Viability unknown) To enable non-#[cfg] conditional compilation macros to work, we could try to, somehow, replace #[func] etc. by real macros which manage the FFI glue code to be generated for the methods they are being applied on. That is, #[func] itself would generate some Rust code which would enable FFI for the method it annotates, thus enabling it to work like #[itest] and be aware of/be affected by other macros. (For virtual trait impls, we would just manually add the #[func]-like macro under the hood.) The problem here is that this doesn't seem to be immediately possible as we can't place an impl inside another impl, at least AFAIK. So this would need further investigation, but could become a very powerful solution if made possible. (Still, solution 1 will already solve the vast majority of scenarios.)

3. (Hardest solution in principle, but useful on its own) Allow #[godot_api] impl blocks to be split like PyO3 allows, thus allowing conditional compilation on individual impl blocks - one impl block for each method where this is required. However, this wouldn't work for virtual trait impl blocks (in principle). But that suggestion is actually useful on its own, as this could allow defining different Godot API methods on different files, so it deserves a separate issue.

---

This concludes my report. This should hopefully be enough material for further discussions on this topic. 😄

[Bromeon]

Thanks a lot for all this effort! ❤️

To move this into practice, we could start defining concrete test cases that a user would expect to work. We can then measure and trade off solutions based on how well they solve them. For this, it's great to have a nuanced and detailed record like you wrote.

We could start with simple ones like these + the ones you already added in #439:

#[func]
fn func();

#[cfg(FALSE)] // idiom for something not defined
#[func]
fn func();

#[func]
#[cfg(FALSE)]
fn func();

...

[Bromeon]

I added venial support for declarative macros in PoignardAzur/venial#50.
Additionally, there have been three PRs around proc-macro attributes referenced above.

I'd say this is good for now, if we encounter new cases, we can open a new issue 🙂