Allow configuring safety level through cargo features #1278

beicause · 2025-08-17T08:39:11Z

Addresses #1276.

Allow configuring safety level through GDEXT_SAFETY_LEVEL_DEBUG and GDEXT_SAFETY_LEVEL_RELEASE environment variables. Not sure if it can be implemented as ExtensionLibrary function. Also implemented as Cargo features may be annoying.

level 0: no checks.
level 1: ensures object is alive.
level 2: and other checks that prevously debug_assertions (RTTI, ~~backtrace~~, Array elements validation)
(Correct me if I missed anything).

GodotRust · 2025-08-17T09:33:07Z

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

TitanNano · 2025-08-17T10:38:47Z

What if we call this GDEXT_SAFETY_CHECKS_* and the values something like STRICT, MINIMAL and NONE. I find this clearer as a user.

The cfg attribute could also be called with_safety_level or with_checks.

Bromeon

Thanks a lot for the pull request! 👍

Please see my comment #1046 (comment) and subsequent response by @TitanNano, where we detailed quite a bit of the design.

Some feedback:

My first impression is that I'd rather not have this configured via environment variables, that's rather brittle and easy to silently get wrong (e.g. someone sets the environment variable for Godot, not for rustc). Instead, it should be a Cargo feature. While features are technically additive, we already do a similar thing with api-4-2 etc.
- I know it can be slightly annoying since you can't easily enable a feature only in the Release config. But this isn't really easier with env vars.
Safety profiles should be named, not magic numbers. It's not obvious whether "0" means "no safety" or "no performance". An option is to use the term checks, but it could also be something else.
- The individual levels could be called fast-unsafe, balanced and paranoid. (To be discussed)
- Cargo feature would then be checks-fast-unsafe.
- Conditional compilation would be #[cfg(checks_at_least = "balanced")]. Let's avoid not if possible.

godot-core/src/obj/raw_gd.rs

Yarwin · 2025-08-17T11:34:26Z

I think including chosen safety profile in preamble would be good idea as well (something along the lines of "Initialize godot-rust (API v4.4.stable.official, runtime v4.4.stable.official) fast-unsafe/balanced/slow-safe mode"?).

gdext/godot-ffi/src/lib.rs

Lines 288 to 293 in 0dde85e

    
           fn print_preamble(version: GDExtensionGodotVersion) { 
        
               let api_version: &'static str = GdextBuild::godot_static_version_string(); 
        
               let runtime_version = read_version_string(&version); 
        
               println!("Initialize godot-rust (API {api_version}, runtime {runtime_version})"); 
        
           }

There are some validity checks which would be nice to cover, albeit I wouldn't sweat too much about it – we can cover them later 😄.

One such example is the PartialEq for Gd<T>:

gdext/godot-core/src/obj/gd.rs

Lines 1078 to 1087 in aaf3ed5

    
           impl<T: GodotClass> PartialEq for Gd<T> { 
        
               /// ⚠️ Returns whether two `Gd` pointers point to the same object. 
        
               /// 
        
               /// # Panics 
        
               /// When `self` or `other` is dead. 
        
               fn eq(&self, other: &Self) -> bool { 
        
                   // Panics when one is dead 
        
                   self.instance_id() == other.instance_id() 
        
               } 
        
           }

instance_id() checks for object validity, while it could be self.instance_id_unchecked() == other.instance_id_unchecked() (compare potentially dead objects) in fast profile level (or even balanced (2)? It won't cause UB, but may cause follow-up logic errors and panics 🤔).

(I'll underline – I don't think we should sweat to cover all (or even the aforementioned one) the cases, we should just make an issue for tracking them)

Bromeon · 2025-08-17T11:47:00Z

Yeah, I'd suggest not to add more specific checks in this PR, to get the basic mechanism working first 🙂

As for defaults, it should be:

in Debug mode: strict/paranoid/extensive checks (highest level)
in Release mode: balanced checks (middle level)

While it means the user won't get max performance per default, it's better if they don't risk UB by default.

But if we go for Cargo feature, we need to find a way how they can opt in... I could imagine that in Debug mode, many would still want to keep paranoid or balanced checks, while Release builds should have fast performance. It's unfortunately not that easy to specify Cargo features based on the build profile...

Bromeon · 2025-08-19T06:28:13Z

Hm, 6 new Cargo features is quite a lot, although I see the dilemma.

What would the alternatives be?

Bromeon · 2025-09-08T22:26:40Z

@beicause could you for now limit this to 3 features?

The compiler invocation is already different due to --release flag, so it should be acceptable to add a --feature xy flag as well. If it causes problems, we can still add more features, but I think I rather overdid it a bit in the past (with api-4-2-1 etc.)

beicause · 2025-09-09T02:02:59Z

@beicause could you for now limit this to 3 features?

The compiler invocation is already different due to --release flag, so it should be acceptable to add a --feature xy flag as well. If it causes problems, we can still add more features, but I think I rather overdid it a bit in the past (with api-4-2-1 etc.)

The problem is if you have already specified checks-x in Cargo.toml and then pass -r --features godot/checks-y, you will enable both checks-x and checks-y, which causes the build to fail.

Yarwin · 2025-09-09T06:41:15Z

Note – I was thinking earlier about exposing every safety level as an independent feature (the simplest possible implementation – no-checks-x, no-checks-y, no-checks-z) but unfortunately it causes a headache when combined with default features >:[.

Bromeon · 2025-09-12T10:38:35Z

The problem is if you have already specified checks-x in Cargo.toml and then pass -r --features godot/checks-y, you will enable both checks-x and checks-y, which causes the build to fail.

That is true, but the same happens with api-* features, no? Maybe here it's more common to want to specify one in Debug, one in Release though 🤔 maybe we should just bite the bullet and provide 6 more features 😞

Bromeon · 2025-09-14T08:33:13Z

As some stuff isn't yet clear and this can be added in a non-breaking way, I'll postpone this to after the initial v0.4 release.

beicause · 2025-09-24T15:57:28Z

I think we can just add two features: debug-checks-balanced and release-checks-fast-unsafe, and by default we use debug-checks-paranoid and release-checks-balanced. The other safety configs are probably not very useful.

Bromeon · 2025-09-24T20:46:39Z

Great idea! We can always add more if users have a concrete case for it 💪

Bromeon

Thanks for the update!

We should have some CI jobs that run itest with those features, otherwise we don't even know if code compiles, let alone it works.

Maybe we find a way to reuse existing CI jobs, so that total CI time isn't increased... Do you have any suggestions?

Bromeon · 2025-09-28T15:10:53Z

godot/Cargo.toml


+
+debug-checks-balanced = ["godot-core/debug-checks-balanced"]
+release-checks-fast = ["godot-core/release-checks-fast"]


Use consistent terminology: fast -> fast-unsafe everywhere

Bromeon · 2025-09-28T15:15:14Z

godot-ffi/src/lib.rs

+    let checks_mode = if cfg!(checks_at_least = "paranoid") {
+        "paranoid"
+    } else if cfg!(checks_at_least = "balanced") {
+        "balanced"
+    } else if cfg!(checks_at_least = "fast-unsafe") {
+        "fast-unsafe"
+    } else {
+        unreachable!();
+    };
+    println!("Initialize godot-rust (API {api_version}, runtime {runtime_version}, safety checks {checks_mode})");


Great idea! 👍

Could you extract this into a function? Might come in handy somewhere else...

fn safety_checks_string() -> &'static str { ... }

Bromeon · 2025-09-28T15:18:44Z

godot-core/src/obj/raw_gd.rs

+        {
+            let call_ctx = CallContext::gd::<T>(method_name);
+
+            let instance_id = self.check_dynamic_type(&call_ctx);


The RTTI check should probably only occur under paranoid level, no?

Bromeon added the feature Adds functionality to the library label Aug 17, 2025

Bromeon added this to the 0.4 milestone Aug 17, 2025

Bromeon reviewed Aug 17, 2025

View reviewed changes

godot-core/src/obj/raw_gd.rs Show resolved Hide resolved

beicause force-pushed the safety-level-config branch 3 times, most recently from 793ac8b to 99e3534 Compare August 18, 2025 05:28

Bromeon force-pushed the master branch 2 times, most recently from 779c043 to ddc5b76 Compare August 18, 2025 19:16

beicause force-pushed the safety-level-config branch 2 times, most recently from 37b86cd to 12a3cce Compare August 19, 2025 05:44

Bromeon modified the milestones: 0.4, 0.4.x Sep 14, 2025

beicause force-pushed the safety-level-config branch from 12a3cce to dcbc049 Compare September 24, 2025 14:10

beicause changed the title ~~Allow configuring safety level through environment variables~~ Allow configuring safety level through cargo features Sep 24, 2025

beicause force-pushed the safety-level-config branch 2 times, most recently from bdf8d01 to 20fb757 Compare September 24, 2025 15:15

Add cargo features to configure safety checks

a8eb3da

beicause force-pushed the safety-level-config branch from 20fb757 to a8eb3da Compare September 24, 2025 15:52

Bromeon mentioned this pull request Sep 24, 2025

Only provide function name to CallContext in debug #1331

Open

Bromeon reviewed Sep 28, 2025

View reviewed changes



		debug-checks-balanced = ["godot-core/debug-checks-balanced"]
		release-checks-fast = ["godot-core/release-checks-fast"]

Uh oh!

Allow configuring safety level through cargo features #1278

Are you sure you want to change the base?

Allow configuring safety level through cargo features #1278

Conversation

beicause commented Aug 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GodotRust commented Aug 17, 2025

Uh oh!

TitanNano commented Aug 17, 2025

Uh oh!

Bromeon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Yarwin commented Aug 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bromeon commented Aug 17, 2025

Uh oh!

Bromeon commented Aug 19, 2025

Uh oh!

Bromeon commented Sep 8, 2025

Uh oh!

beicause commented Sep 9, 2025

Uh oh!

Yarwin commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bromeon commented Sep 12, 2025

Uh oh!

Bromeon commented Sep 14, 2025

Uh oh!

beicause commented Sep 24, 2025

Uh oh!

Bromeon commented Sep 24, 2025

Uh oh!

Bromeon left a comment

Choose a reason for hiding this comment

Uh oh!

Bromeon Sep 28, 2025

Choose a reason for hiding this comment

Uh oh!

Bromeon Sep 28, 2025

Choose a reason for hiding this comment

Uh oh!

Bromeon Sep 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

beicause commented Aug 17, 2025 •

edited

Loading

Yarwin commented Aug 17, 2025 •

edited

Loading

Yarwin commented Sep 9, 2025 •

edited

Loading