Correct ConnectFlags classification (enum -> bitfield)

[Bromeon]

Works around ConnectFlags being exported as enum instead of bitfield in Godot.

Also adds a general mechanism in special_cases.rs to override this property.

[GodotRust]

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

[lilizoey]

I dont think this is correct? This seems to mention both adding a new trait and an implementation for that trait. However this is implementing a trait for a type that previously did not have that trait implemented, but the trait already existed beforehand. The justification for why that change isn't considered breaking is because it requires a blob import to cause breakage, however you can cause breakage without a blob import when implementing a trait for a type that previously didn't have it. For instance this code would break with this change:

use godot::classes::object::ConnectFlags;
use godot::obj::EngineEnum;

pub trait MyTrait: Sized {
  fn ord(self) -> i32 {
    10
  }
}

impl MyTrait for ConnectFlags {}

fn foo() {
  _ = ConnectFlags::DEFERRED.ord();
}

as the call to ord is now ambiguous.

[lilizoey]

ok here's a link where it is actually explicitly allowed: https://github.com/rust-lang/rfcs/blob/master/text/1105-api-evolution.md#minor-change-implementing-any-non-fundamental-trait
not sure why it's not on that semver documentation page

i guess that it being mentioned in that RFC as deferring to adding a default trait item means that this case actually falls under this point: https://doc.rust-lang.org/cargo/reference/semver.html#trait-new-default-item

which means we should discuss whether this is considered acceptable breakage? personally i'd say probably yeah, worst case breakage in this case would likely just mean someone made their own reimplementation of EngineEnum for ConnectFlags, in which case they can just remove that and their code continues working.

[Bromeon]

Thanks for double-checking this! I added your link as well.

I'd also say "minor" breakages should generally be considered acceptable. Otherwise we're walking a minefield and likely have to push the major (in our case minor) version for lots of situations where we just add things 🤔

[lilizoey]

Well the trait-new-default-item category considers it "possibly breaking" and not "minor", so if it does fall in under that category then it's not automatically minor.

[Bromeon]

You're right, my mistake. Either way I think we should probably consider this case by case, there are some obscure APIs that have hardly any impact, and others which are used by almost every user.

I wrote a bit about this concrete case in this comment.

[Bromeon]

The irony here is that removing EngineEnum would likely cause less breakage.

There are some fun parts leading to this:

1. ConnectFlags is used exclusively around signal connecting.

2. Object::connect_ex() requires u32.

3. Signal::connect() requires i64.

   • This one is used more rarely, because with the intermediate object, it makes signal setup even more unwieldy than it already is...

4. EngineEnum::ord() returns yet another type, i32 😀

5. People who work with connect flags thus have to use as-casts in addition to ord(). It's not enough that we don't provide type-safe ConnectFlags, but even the underlying integer types mismatch 😁

   People who use it do so almost certainly in this way:

   ConnectFlags::DEFERRED.ord() as u32
   // or 
   (ConnectFlags::DEFERRED.ord() | ConnectFlags::PERSIST.ord()) as u32

6. If we now add EngineBitfield but keep EngineEnum, we'll create an import ambiguity because both EngineEnum and EngineBitfield are in the prelude, which most people use.

TLDR: we're breaking more code by following SemVer guidelines than by violating them.

I'm not sure if we do users a favor if we include this in v0.2? Without type-safe connect functions, this is not that useful... it might be relevant for #1000 though 🤔