Godot-proposals: `assert()` should evaluate its arguments in release, and then no-op

[Wavesonics]

Describe the project you are working on:
Nuclear reactor simulation

Describe the problem or limitation you are having in your project:
Many functions return a status code that is often not checked as it should not fail in normal cases.

If it does fail, something bad has happened. For instance:
get_tree().change_scene("res://actors/fuel_rod/FuelRod.tscn")

I want to be able to assert that this is true, and have it crash debug builds so it fails loudly.

The first way that I'd think to do this is nice and compact:
assert(get_tree().change_scene("res://actors/fuel_rod/FuelRod.tscn") == OK)

However, the assert() is entirely compiled out in Release, so the change scene never executes in release builds, and everything breaks.

Describe the feature / enhancement and how it helps to overcome the problem or limitation:
assert() should still evaluate it's arguments in release, it's just that assert() behavior should be a nop

Describe how your proposal will work, with code, pseudocode, mockups, and/or diagrams:
assert() still evaluates it's arguments, but assert then does nothing with the result.

This is also more in-line with how assert works in other languages, so it'll help new-comers adapt to GDScript and not have some WTF is happening moments when things don't work in release.

If this enhancement will not be used often, can it be worked around with a few lines of script?:
Yes, but it's much less compact. And so many Godot functions return codes that if left unchecked result in warnings. In order to check them all means lots of extra code bloat:

var result := get_tree().change_scene("res://actors/fuel_rod/FuelRod.tscn")
assert(result != OK)

Is there a reason why this should be core and not an add-on in the asset library?:
It has to be core as it's a language level feature of GDScript

[Calinou]

If we do this, we lose the performance advantages of being able to leave out error checking in release mode :slightly_frowning_face:

push_error() should still work to print error messages in release mode. If printing a message to the console isn't enough, you could use the following to display a blocking alert and exit the project once the user acknowledged it:

# panic.gd
extends Object

static func panic(message: String) -> void:
    # Will automatically print an error message in the console as well.
    OS.alert(message)
    # Non-zero exit code to indicate failure.
    get_tree().quit(1)

Usage:

const Panic = preload("res://panic.gd")

func _ready() -> void:
    Panic.panic("Error :(")

[Scony]

IMO, providing a simple mechanism that would warn a user if there are function calls in assert being discarded during game export would help a lot. And it's definitely doable since just a parse tree is needed - nothing more.

[bluenote10]

What about the solution taken by the Nim language as well: Having two flavors of assert: The normal assert is optimized out in a release completely, while the emphasized doAssert is always on (and would internally use push_error to at least log failed assertions in release to stdout). This allows users to precisely choose on a per-case basis if performance or correctness is preferred.

In my opinion there is no big benefit of making a failed release assertion a noop for performance reason, because a failed assertion probably indicates something is fairly broken. It could be a time safer when collecting bug reports from users, if the stdout would already contain information on failed doAsserts.

[Scony]

@bluenote10 doAssert is an interesting solution but it does not help with the problem where someone accidentally puts a call into assert and hence this call is silently discarded in the release (note that most expressions are pure [w/o side effects] which poses no problem if they are disarded).

[bluenote10]

@Scony You're right. In this case it might be better to have it the other way around: assert always does assert, and in performance critical places a special assertOptimized can be used. Since users now have to consciously opt-in to the more tricky variant, the risk of accidents is significantly lower (the documentation could have a clear warning).

[dalexeev]

There is the print_debug function, which, unlike print, does nothing in release builds. What if we also distinguish between assert and assert_debug? Also, there are proposals to add the version keyword/annotation.

[snoopdouglas]

I'd also prefer that an assert evaluates its expression but doesn't throw in production ~(similar to C)~ [turns out C's behaves like this too].

At the very least, yes, a warning would be very useful here (but again, it should warn only if the expression has side-effects).

[Calinou]

(but again, it should warn only if the expression has side-effects).

The issue is that we don't know in advance which methods have side effects and which ones don't.
The const qualifier for built-in methods doesn't necessarily guarantee the method won't have any side effects. It just makes it less likely for it to have side effects.

[snoopdouglas]

we don't know in advance which methods have side effects and which ones don't

I know this'd be something for the long-term (because it'd probably be a lot of effort), but ideally we'd want the GDScript compiler's static analysis to figure this out for us, right?