Allow #[cfg] to be used with #[godot_api] - Part 1: inherent impls #443

PgBiel · 2023-10-06T06:48:54Z

This is my attempt at allowing #[cfg] to work with items in #[godot_api] impl blocks. This PR only covers inherent impls (non-virtual #[godot_api] impl blocks). As such, this does not close the #399 issue. That's the job of PR #444.

This PR implements solution 1 mentioned in #379 (comment). That is, this is a solution specifically for #[cfg], and does not guard against other possible conditional compilation macros. Those should be used on the entire impl if possible (so as to run before #[godot_api]).
If solution 2 is ever made viable, it would supersede this PR, but ATM there doesn't seem to be an easy and viable way for it, so this should work in the meantime.

The gist of this PR is that, for every Godot API attribute type (func, signal, constant), we store the attributes specified by the user, and later filter them specifically looking for #[cfg] attributes. Matching attributes are directly forwarded to the generated FFI glue corresponding to each func/signal/constant, such that nothing is registered to Godot if the func/signal/constant is conditionally removed from compilation through #[cfg] (since it would necessarily also remove the FFI glue).

Added some tests to itests as proposed in #379 (comment), although the tests can't catch everything as, e.g., signals are always removed from the final impl block so they don't error on duplicates (for instance) regardless of this PR.

Examples

`#[func]`

Consider the following modified code from the Dodge the Creeps example:

#[godot_api]
impl Main {
    #[cfg(any())]
    #[func]
    fn game_over(&mut self) {
    /* ... */
}

Before this PR: Does not compile:

    Checking dodge-the-creeps v0.1.0 (/.../gdext/examples/dodge-the-creeps/rust)
error[E0599]: no method named `game_over` found for struct `RefMut<'_, Main>` in the current scope
  --> examples/dodge-the-creeps/rust/src/main_scene.rs:27:8
   |
27 |     fn game_over(&mut self) {
   |        ^^^^^^^^^ method not found in `RefMut<'_, Main>`

For more information about this error, try `rustc --explain E0599`.
error: could not compile `dodge-the-creeps` (lib) due to previous error

After this PR: compiles (with some warnings since the methods it uses are now unused!).

`#[signal]`

Consider the following modified code from the Dodge the Creeps example:

#[godot_api]
impl Player {
    #[cfg(any())]
    #[signal]
    fn hit();
    /* ... */
}

It will compile with or without this PR since the hit() function is actually removed, so its implementation is not referred to by the FFI glue. But we can note that, without this PR, the signal is still registered in Godot:

With this PR, the cfg attribute is passed through to the FFI registration block, and thus it will disappear with the signal itself, ensuring the signal won't be registered if it is removed from compilation through #[cfg].

`#[constant]`

Consider the following added code to the Dodge the Creeps example:

#[godot_api]
impl Player {
    #[cfg(any())]
    #[constant]
    const MY_CONST: bool = true;
    /* ... */
}

Before this PR: Does not compile:

Checking dodge-the-creeps v0.1.0 (/.../gdext/examples/dodge-the-creeps/rust)
error[E0599]: no associated item named `MY_CONST` found for struct `Player` in the current scope
  --> examples/dodge-the-creeps/rust/src/player.rs:18:11
   |
6  |   pub struct Player {
   |   ----------------- associated item `MY_CONST` not found for this struct
...
15 |   impl Player {
   |  ______-
16 | |     #[cfg(any())]
17 | |     #[constant]
18 | |     const MY_CONST: bool = true;
   | |          -^^^^^^^^ associated item not found in `Player`
   | |__________|
   | 

For more information about this error, try `rustc --explain E0599`.
error: could not compile `dodge-the-creeps` (lib) due to previous error

After this PR: Compiles.

Other details

Commits

I divided this PR in 3 commits: one for #[func], one for #[signal] and one for #[constant]. While this organization is nice to work with, the commits can be squashed if needed.

`#[cfg(any())]`

This is a cfg call that represents a contradiction / something which is always false (equivalent to the #[cfg(FALSE)] idea, but more robust). Used in tests to ensure that the annotated member will be unconditionally removed from compilation, in order to verify that this doesn't cause errors.

Discussion points

I made this relatively quickly, so this might need some thoughts regarding design and stuff. The main one is: I created not only an attributes field at FuncDefinition (to store attributes specified above the function), but also the (analogous) SignalDefinition struct in order to store both the signal's function signature but also the attributes it originally used. This is because, currently, the reduce_to_signature function removes all attributes from the Function object it returns:

gdext/godot-macros/src/util/mod.rs

Lines 68 to 71 in 88a7934

    
           pub fn reduce_to_signature(function: &Function) -> Function { 
        
               let mut reduced = function.clone(); 
        
               reduced.vis_marker = None; // TODO needed? 
        
               reduced.attributes.clear();

And all the signatures used with #[func] and #[signal] attributes go through that function. So, in order to reduce the "blast radius" of the PR (as it could be a breaking change to pass around attributes with said signature objects), I simply added an additional attributes field for FuncDefinition, and created SignalDefinition to be able to have a similar attribute for signals.

(On an unrelated note, I believe it could be wise to apply the newtype pattern to create a struct Signature(Function) type to enforce statically that our invariants for signatures uphold, instead of passing raw Function objects, returned by reduce_to_signature, around - but that is a separate discussion, and is probably largely off-topic for this PR.)

Note that #[constant] didn't have to undergo much change, as their attributes are not cleared.

Another approach here, instead of creating new attributes fields, would be to just not clear attributes from signatures, but I don't know if that is safe (I took a look around where they are used, but couldn't determine with full certainty yet), mostly because they could be placed somewhere else and it might be bad for attributes to wherever that is too (...or it could also be desired, unless the attribute changes the body of the function). So, feel free to make a decision here.

I added a TODO comment which deserves a look; I added a review comment for it below.
Please feel free to suggest how I could add some runtime tests for this (to ensure the methods were actually 'deleted'). If that'd be excessive, feel free to say so as well.

GodotRust · 2023-10-06T06:54:50Z

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-443

godot-macros/src/class/godot_api.rs

PgBiel · 2023-10-09T04:14:37Z

godot-macros/src/class/godot_api.rs

        let mut param_types: Vec<TyExpr> = Vec::new();
        let mut param_names: Vec<String> = Vec::new();

-        for param in signature.params.inner {
+        for param in &signature.params.inner {


Just to add here that the usage of borrowing here was mostly to satisfy the borrow checker, as extract_cfg_attrs returns a reference to the given attributes, so they shouldn't die with signal in this loop if we want to use the cfg attrs later. We could, of course, just make util::extract_cfg_attrs return Vec<Attribute> by cloning them (that just didn't seem necessary, in principle). Feel free to decide the best approach here.

That's fine, but you could maybe use .iter() for explicitness.

Okay! Changed it.

Bromeon

Thanks a lot for addressing this and the accompanying discussion! 💪

If in the future we wanted to extend this to other attributes besides #[cfg], what would be necessary?

This might also create minor conflicts with #421; in case that is merged first, this would need rebase/merge.

Bromeon · 2023-10-09T13:41:04Z

godot-macros/src/class/data_models/field_var.rs

+                // Since we're analyzing a field, we don't have access to the function's
+                // attributes. We have to assume the function exists and has the name the user
+                // gave us.
+                // Ideally, we'd be able to place #[cfg_attr] on #[var(get)] and #[var(set)]
+                // separately, but that is not currently supported.
+                attributes: Vec::new(),


These are specifically non-gdext attributes, i.e. excluding #[func] etc? If yes, maybe name it third_party_attributes and mention that in the comment briefly (could also be at field declaration rather than initialization).

Btw, lines can be 120+ characters wide.

Alright. I used external_attributes as that felt less verbose, does that sound OK? (If not, that's totally fine, I can use third_party_attributes instead.)

I've changed the documentation comments a bit too.

Bromeon · 2023-10-09T13:42:15Z

godot-macros/src/class/godot_api.rs

        let mut param_types: Vec<TyExpr> = Vec::new();
        let mut param_names: Vec<String> = Vec::new();

-        for param in signature.params.inner {
+        for param in &signature.params.inner {


That's fine, but you could maybe use .iter() for explicitness.

godot-macros/src/class/godot_api.rs

Bromeon · 2023-10-09T13:58:42Z

(On an unrelated note, I believe it could be wise to apply the newtype pattern to create a struct Signature(Function) type to enforce statically that our invariants for signatures uphold, instead of passing raw Function objects, returned by reduce_to_signature, around - but that is a separate discussion, and is probably largely off-topic for this PR.)

Could be, but this is mostly created in one place at the moment? So it might not be that critical.

3. Please feel free to suggest how I could add some runtime tests for this (to ensure the methods were actually 'deleted'). If that'd be excessive, feel free to say so as well.

ClassDb::class_has_method() could help here. There are similar introspection methods for constants and signals.

PgBiel · 2023-10-12T01:37:23Z

If in the future we wanted to extend this to other attributes besides #[cfg], what would be necessary?

That's an important question. In general, you'll have to make sure the attribute you're forwarding to the FFI glue works with any statement, not just function signatures (for semi-obvious reasons: we're not creating a function in the FFI glue). If that's satisfied, and you're sure it'd be worth it to apply to the FFI glue, then you can just modify what happens at the execution sites of util::extract_cfg_attrs (which filters the external attributes, picking only attrs named cfg to be forwarded - just make it accept more things than #[cfg]).

Note: the above answer wouldn't apply fully to the other PR (#444) - while the procedure would be similar for that other PR, you'd also have to notice that I use a match construction specifically to make #[cfg] work, so it might not make sense to forward certain attributes to above a match arm like that. Overall, things should probably be analyzed on case-by-case basis.

This might also create minor conflicts with #421; in case that is merged first, this would need rebase/merge.

In principle, I don't see any conflicts which can arise from #421 with this PR at its current state - but future additions to itest in this PR could indeed conflict a bit, so I'll be on the watch for that. Thanks for the heads-up!

PgBiel · 2023-10-12T01:54:27Z

(On an unrelated note, I believe it could be wise to apply the newtype pattern to create a struct Signature(Function) type to enforce statically that our invariants for signatures uphold, instead of passing raw Function objects, returned by reduce_to_signature, around - but that is a separate discussion, and is probably largely off-topic for this PR.)

Could be, but this is mostly created in one place at the moment? So it might not be that critical.

Yeah I guess it's not that bad, just something to keep in mind in the future - might be useful to know in case this code is eventually modified further later.

Please feel free to suggest how I could add some runtime tests for this (to ensure the methods were actually 'deleted'). If that'd be excessive, feel free to say so as well.

ClassDb::class_has_method() could help here. There are similar introspection methods for constants and signals.

Thanks! Over the next few days, I will try to use this to make a more explicit runtime test. (Same applies for the other PR.)

PgBiel · 2023-10-12T16:51:22Z

Okay, added some proper integration tests! Feel free to suggest changes or additions in that regard.

With that, I believe most things in this PR should be OK now. Feel free to add further reviews as needed. 👍

Bromeon

Thanks! Looks mostly good, some comment/doc feedback.

Bromeon · 2023-10-13T21:37:51Z

itest/rust/src/register_tests/constant_test.rs

        ("A", HasConstants::A),
        ("B", HasConstants::B as i64),
        ("C", HasConstants::C as i64),
        ("D", HasConstants::D as i64),
+        ("CFG_REMOVES_CONSTANT", HasConstants::CFG_REMOVES_CONSTANT),


The name is a bit confusing, this looks like it tests for presence of a constant that was deliberately removed.

Maybe name them ONLY_ACTIVE_CFG_CONSTANT or so. And would be good to add a comment near the definition that the 3 are mutually exclusive.

The name is a bit confusing, this looks like it tests for presence of a constant that was deliberately removed.

True!

Maybe name them ONLY_ACTIVE_CFG_CONSTANT or so. And would be good to add a comment near the definition that the 3 are mutually exclusive.

Okay. I chose CFG_REMOVES_DUPLICATE_CONSTANT_DEF as that felt a bit more appropriate, but I can change it to ONLY_ACTIVE_CFG_CONSTANT if that'd be better.

Bromeon · 2023-10-13T21:39:33Z

itest/rust/src/register_tests/constant_test.rs

+    #[cfg(any())]
+    #[constant]
+    const CFG_REMOVES_CONSTANT_FFI_GLUE: bool = true;
+
+    #[constant]
+    #[cfg(any())]
+    const CFG_REMOVES_CONSTANT_FFI_GLUE: bool = true;


What is different here, compared to above? What does "FFI glue" mean? Note that tests should ideally be written against behavioral spec/constract, not implementation details.

Maybe choose a more expressive name and add a short description.

I agree it's not very clear. Should probably be better now.

Bromeon · 2023-10-13T21:40:45Z

itest/rust/src/register_tests/func_test.rs

+    #[cfg(any())]
+    #[func]
+    fn cfg_removes_function() -> bool {
+        false
+    }
+
+    #[func]
+    #[cfg(any())]
+    fn cfg_removes_function() -> bool {
+        false
+    }


Could also use compile_error!, no?

This might also apply elsewhere...

Missed it here. Thanks

Added compile_error! to constants as well now.

Bromeon · 2023-10-13T21:41:15Z

itest/rust/src/register_tests/func_test.rs

+    #[cfg(any())]
+    #[func]
+    fn cfg_removes_function_ffi_glue() -> bool {
+        true
+    }
+
+    #[func]
+    #[cfg(any())]
+    fn cfg_removes_function_ffi_glue() -> bool {
+        true
+    }


Also here, not exactly clear what "FFI glue" means 🤔

I've renamed the test functions now, should probably be clearer

Bromeon · 2023-10-13T21:41:53Z

itest/rust/src/register_tests/func_test.rs

+
+#[itest]
+fn cfg_doesnt_interfere_with_valid_method_impls() {
+    // if we re-implement this method but the re-implementation is removed, that should keep the non-removed


Nitpick: sentence should start with capital letter, and could fit on one line.

It seems to surpass 120 characters when kept at a single line, is that ok?

Ah, looked shorter 😀 but yes, 120-145 is fine.

Bromeon · 2023-10-13T21:43:18Z

itest/rust/src/register_tests/func_test.rs

+    let has_method = |name: &str| {
+        ClassDb::singleton()
+            .class_has_method_ex(GdSelfReference::class_name().to_string_name(), name.into())
+            .no_inheritance(true)
+            .done()
+    };


The has_constant, has_method and has_constant could probably be moved out to the global scope, with the class itself being a generic parameter. It's quite likely that we may want to use them elsewhere 🙂

Bromeon · 2023-10-13T21:45:58Z

godot-macros/src/class/godot_api.rs

+    /// The signal's function signature.
+    signature: Function,
+
+    /// The signal's non-gdext attributes (that is, excluding #[signal]).


This could be understood as "non-gdext attributes except #[signal]", maybe make it very clear with "The signal's non-gdext attributes (everything except #[signal])".

Maybe it's just me being a native speaker, but better to have it unambiguous 😉

Same above with #[func].

Bromeon

Thanks so much for the initiative and the repeated improvements! These quality-of-life improvements are what makes working with the library fun 😊

PgBiel commented Oct 6, 2023

View reviewed changes

godot-macros/src/class/godot_api.rs Outdated Show resolved Hide resolved

PgBiel force-pushed the allow-cfg-inherent-impl-godot-api branch 4 times, most recently from b0acd87 to b1a9a14 Compare October 7, 2023 21:39

PgBiel mentioned this pull request Oct 7, 2023

Allow #[cfg] to be used with #[godot_api] - Part 2: virtual impls #444

Merged

PgBiel commented Oct 9, 2023

View reviewed changes

PgBiel marked this pull request as ready for review October 9, 2023 04:16

Bromeon reviewed Oct 9, 2023

View reviewed changes

Bromeon added bug c: register Register classes, functions and other symbols to GDScript labels Oct 9, 2023

PgBiel force-pushed the allow-cfg-inherent-impl-godot-api branch from b1a9a14 to 3dc7d32 Compare October 12, 2023 01:26

PgBiel force-pushed the allow-cfg-inherent-impl-godot-api branch from 3dc7d32 to a88d02f Compare October 12, 2023 01:50

PgBiel force-pushed the allow-cfg-inherent-impl-godot-api branch from a88d02f to e06e67b Compare October 12, 2023 15:58

Bromeon reviewed Oct 13, 2023

View reviewed changes

PgBiel force-pushed the allow-cfg-inherent-impl-godot-api branch from e06e67b to 1b635d5 Compare October 13, 2023 22:39

PgBiel added 3 commits October 14, 2023 01:21

Allow #[cfg] to be used with #[func]

ab6995f

Allow #[cfg] to be used with #[signal]

627bba5

Allow #[cfg] to be used with #[constant]

18a4611

PgBiel force-pushed the allow-cfg-inherent-impl-godot-api branch from 1b635d5 to 18a4611 Compare October 14, 2023 04:21

Bromeon approved these changes Oct 14, 2023

View reviewed changes

Bromeon added this pull request to the merge queue Oct 14, 2023

Merged via the queue into godot-rust:master with commit 8165f62 Oct 14, 2023
14 checks passed

PgBiel deleted the allow-cfg-inherent-impl-godot-api branch October 14, 2023 14:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow #[cfg] to be used with #[godot_api] - Part 1: inherent impls #443

Allow #[cfg] to be used with #[godot_api] - Part 1: inherent impls #443

PgBiel commented Oct 6, 2023 •

edited

Loading

GodotRust commented Oct 6, 2023

PgBiel Oct 9, 2023

Bromeon Oct 9, 2023

PgBiel Oct 12, 2023

Bromeon left a comment

Bromeon Oct 9, 2023

PgBiel Oct 12, 2023

Bromeon Oct 9, 2023

Bromeon commented Oct 9, 2023

PgBiel commented Oct 12, 2023

PgBiel commented Oct 12, 2023

PgBiel commented Oct 12, 2023

Bromeon left a comment •

edited

Loading

Bromeon Oct 13, 2023

PgBiel Oct 13, 2023

Bromeon Oct 13, 2023

PgBiel Oct 13, 2023

Bromeon Oct 13, 2023

PgBiel Oct 13, 2023

PgBiel Oct 13, 2023

Bromeon Oct 13, 2023

PgBiel Oct 13, 2023 •

edited

Loading

Bromeon Oct 13, 2023

PgBiel Oct 13, 2023

Bromeon Oct 13, 2023

PgBiel Oct 13, 2023

Bromeon Oct 13, 2023

PgBiel Oct 13, 2023

Bromeon Oct 13, 2023

Bromeon left a comment

	pub fn reduce_to_signature(function: &Function) -> Function {
	let mut reduced = function.clone();
	reduced.vis_marker = None; // TODO needed?
	reduced.attributes.clear();

Allow #[cfg] to be used with #[godot_api] - Part 1: inherent impls #443

Allow #[cfg] to be used with #[godot_api] - Part 1: inherent impls #443

Conversation

PgBiel commented Oct 6, 2023 • edited Loading

Examples

#[func]

#[signal]

#[constant]

Other details

Commits

#[cfg(any())]

Discussion points

GodotRust commented Oct 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon commented Oct 9, 2023

PgBiel commented Oct 12, 2023

PgBiel commented Oct 12, 2023

PgBiel commented Oct 12, 2023

Bromeon left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PgBiel Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon left a comment

Choose a reason for hiding this comment

PgBiel commented Oct 6, 2023 •

edited

Loading

`#[func]`

`#[signal]`

`#[constant]`

`#[cfg(any())]`

Bromeon left a comment •

edited

Loading

PgBiel Oct 13, 2023 •

edited

Loading