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

[PgBiel]

Related to (but can be merged independently of) #443. Fixes #399.
Note: Turns out this PR was smaller than I expected, so both PRs could theoretically be fused together; however, this particular PR should be faster to review (/has less design decisions).

Explicitly allows #[cfg] to be used above methods in virtual #[godot_api] impls, following solution 1 from #379 (comment).

It does this by just transporting the #[cfg] attrs to the virtual methods' generated FFI glue, thus avoiding their registration in Godot if the original method implementation was conditionally removed from compilation.

Implementation details

I had to "cheat" a bit when an expression was being generated, replacing a generated e.g.

Some(create::<thing>)

with

match () {
  #[cfg(...)]
  () => Some(create::<thing>),
  _ => None,
}

This could have been done with

'blk: {
  #[cfg(...)]
  break 'blk Some(...);
  break 'blk None;
}

But that would leave us vulnerable to a certain warning regarding shadowed labels (if an outer block also used 'blk for some reason), and that warning doesn't seem to be dismissible/#[allow(...)]-able at all (this related issue rust-lang/rust#31745 was closed, but doesn't seem to have been entirely fixed...?).

The match counterpart only requires #[allow(unreachable_patterns)] (which I added to the generated code) to avoid warnings regarding when both patterns are present.

Tests

Added some basic tests in itest which test compilation, but suggestions for runtime checks on the tests are appreciated (e.g. something that checks ClassDB or similar), as most of the errors related to the old behavior of #[cfg] usage on virtual methods here would occur on runtime.

One immediate result from the current itest block is that e.g. this:

#[godot_api]
impl RefCountedVirtual for GdSelfReference {
    fn init(base: Base<Self::Base>) -> Self {
        Self {
            internal_value: 0,
            base,
        }
    }

    #[cfg(any())]
    fn cfg_removes_this() {}
}

currently fails to compile as the trait does not have that method, but compiles fine after this PR.

[GodotRust]

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

[Bromeon]

Thanks a lot! Like #443, great work.

It's a shame that Rust forces us to use these tricks, but I'm amazed by what you came up with 😉

[Bromeon]

This is a very cool trick!

Can you add #[allow(unreachable_patterns)] right here? The more local, the better -- otherwise we may accidentally swallow legitimate warnings in unrelated code.

Also, does clippy not complain if the #[cfg] evaluates to false and the remaining code becomes

match () {
   _ => None,
}

Or maybe even without that, having a match () which can only have one value?

[PgBiel]

This is a very cool trick!

Thanks!

Can you add #[allow(unreachable_patterns)] right here? The more local, the better -- otherwise we may accidentally swallow legitimate warnings in unrelated code.

Unfortunately, since the match is being used as an expression, we can't due to rust-lang/rust#15701 (currently results in an error). Adding macros above match arms does compile, but for this purpose is useless as #[allow(...)] over a single match arm doesn't dismiss any related warnings.

Also, does clippy not complain if the #[cfg] evaluates to false and the remaining code becomes

match () {
   _ => None,
}

Great catch:

warning: this match could be replaced by its scrutinee and body
   --> examples/dodge-the-creeps/rust/src/main_scene.rs:103:31
    |
103 |           let _x: Option<i32> = match () {
    |  _______________________________^
104 | |             _ => None,
105 | |         };
    | |_________^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#match_single_binding
    = note: `#[warn(clippy::match_single_binding)]` on by default
help: consider using the scrutinee and body instead
    |
103 ~         let _x: Option<i32> = ();
104 ~         None;
    |

I've added #[allow(clippy::match_single_binding)] as well now.

Or maybe even without that, having a match () which can only have one value?

That doesn't seem to generate a warning by itself.

[Bromeon]

Thanks a lot! You now have this:

            #[allow(unreachable_patterns)] // due to the cfg-based match statements
            #[allow(clippy::match_single_binding)] // avoid warning on single-arm matches

Could you clarify the 2 comments slightly? e.g. the 2nd could be "For #[cfg(false)], match contains only single arm".

Also nitpicking, but we try to use capitalization and full stops for comments, even if they're not prose (not 100% consistent though) 😉

[PgBiel]

Okay, I've tried to improve the comments a bit; feel free to suggest further changes.

Also, I managed to move the allows to the match itself by placing the match inside a block, as showcased below:

fn main() {
    let x = {
        #[allow(unreachable_patterns)]
        #[allow(clippy::match_single_binding)]
        match () {
            () => true,
            _ => false,
        }
    };
    // ^instead of let x = match () { ... }
    println!("x = {x}");
}

I'm not sure why this works - seems to work even with something as simple as just 5i32, so perhaps it's some form of workaround for the aforementioned Rust compiler issue. The sample above (on my limited local tests) seems to compile with Rust 1.69.0 as well, so this shouldn't raise MSRV.

[Bromeon]

Could also be panic!, in case it's ever compiled in and called?

Or maybe even compile_error!, if that works here? (Similar thoughs apply below).

[PgBiel]

Great idea, done!

[PgBiel]

Hmmm, CI's runtime tests just made me notice a bug - it seems that specifying init more than once (as an example) is making only the last init's cfg attrs count, thus Godot thinks GDSelfReference (used in the test) is abstract as init is not registered due to the last init being removed from compilation (in reality, it does have a concrete init implementation, so it should be registered).

I think I can fix this for all methods by adding more match arms (one for each time the method is declared).

Edit: Yep, that worked! We now have something like

match () {
  #[cfg(...)]
  () => Some(create::<thing>),
  #[cfg(...)]
  () => Some(create::<thing>),
  #[cfg(...)]
  () => Some(create::<thing>),
  _ => None,
}

one arm for each implementation of the same method. This guarantees the generated FFI glue will be kept if at least one of the implementations 'survives'. (If more than one 'survives', then Rust should generate an error anyway.)

I'll probably try to add some more runtime tests over the next few days.

Though CI is still failing in one check which seems to be unrelated to this PR.

[Bromeon]

Oh nice, great improvements!

Yes, the CI is an epic maintenance chore lately. I don't know if it's due to upstream breakages or other changes, but this is now the 3rd problem within a week or so:

• validated calls issue (fixed upstream)
• macOS spuriously fails to read version (not yet fixed)
• memcheck dlopen which is now suddenly a problem

[Bromeon]

Thanks a lot!

[PgBiel]

@Bromeon thanks!! Btw, do you think this deserved some runtime tests like in #443 ? Before the merge, I was thinking of adding something using class_has_method, but I wasn't sure if that would properly check virtual methods... Unless the no inheritance thing would be enough to check whether or not we have overridden something?