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 🙁

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?

[Calinou]

@vnen What's your opinion on this?

[vnen]

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?

This is not useful unless we add const functions in GDScript too (which is not in the plans). While I do want to improve the static analysis over time, I'm not sure if or when this point will be reached.

Regarding assert itself. I've always counted on it not evaluating the expression on release. I don't think changing that is good because right now you can work around this, and if we change then there's no way to not evaluate the argument on release.

So IMO if this is really needed, we should:

• Add a warning to assert if the argument has a function call (which is the only thing I can think of that can have side-effects, since you can't do assignments). This does requires extra changes in the analyzer to keep track of calls, but sounds doable.
• Add an extra keyword that will perform the operation even on release but only perform the assertion on debug. How to name that is an issue, can't think of a good one right now.

Of course that if adding another one we can flip the meanings, but I would prefer to keep behavior consistent with previous versions.

[Wavesonics]

Personally I think adding a new keyword like mentioned above like doAssert() or something, would solve the problem, and be pretty minimal effort?

[nathanfranke]

How about assert(bool) and assert_ok(error)? For assert_ok, we evaluate the expression on debug and release but only compare to OK on debug.

Edit: I'm not a huge fan anymore because whether or not the method is evaluated is arbitrary.

[quantumedbox]

In my personal experience with Nim doAssert approach is immensely handy and it could be used quite a lot with checks for connect success.

Currently for correct code you required to write if branch for each case, which isn't ergonomic, or define your own implementation in GDScript, something like:

func do_assert(status: bool, msg: String = "") -> void:
  if not status:
    if msg.empty():
      push_error("{source}:{function}:{line}".format(get_stack()[1]))
    else:
      push_error("{source}:{function}:{line} ".format(get_stack()[1]) + msg)
    get_tree().quit(1)

Which might potentially be quite costly in frequently ran code snippets, such as instancing of common scenes (enemies, for example), but more importantly it requires calling it on node that is already entered the scene (for get_tree call and exit), which might not always be the case.

Current assert might be preserved if somehow it could be possible to infer whether asserted expression has side-effects or not, and execute side-effect expressions both on release and debug. But I imagine there's no easy way to do it.

[nathanfranke]

In case it helps anyone, here are global methods I use in almost all my projects:

## Assert that the given error is OK.
func ok(err: int) -> void:
	assert(err == OK, "An error occurred.")

## Assert that the given flag is true.
func yes(flag: bool) -> void:
	assert(flag, "Flag is not true.")

Since these are functions, the parameters are always evaluated even in release builds.

Usage, (e.g. in AutoLoad G):

G.ok(get_tree().change_scene_to(load("res://scene.tscn")))

G.yes(tween.start())

[nathanfranke]

I'm taking a bit of a step to the side here, but my project really needs a way to simplify all my validation checks with RPC.

• Maybe assert should only be stripped if inside a preprocessor (proposal).

  assert(1 == 2) # Evaluated on debug and release
  
  feature debug:
  	assert(1 == 2) # Evaluated on debug only

• It would be helpful to have a way to evaluate an expression on debug and release, but only check if err != OK on debug. However, I have no idea how the syntax should look. I don't think the syntax should be anything like:

  assert(get_tree().change_scene(…) == OK)

  because it would be very arbitrary for the method to evaluate but not the condition. My first edge case to that would be:

  assert(OK == get_tree().change_scene(…))

  My best idea right now is:

  assert_ok_debug(get_tree().change_scene(…))

• How should assert work in debug builds? In my opinion, it should return (null?). This would help a lot with my RPC validation.

  if value == null:
      print_debug("Error: Value is null.")
      return # Return, the client sent us an invalid RPC. Continuing here would be bad.

  becomes

  assert(value != null, "Value is null.")

[YuriSizov]

We've discussed it in the proposal review meeting, and would like to ping @vnen again to make the final judgement if we should move forward with this or reject it.

[Zylann]

I just hit this... had important code inside assert and it was stripped entirely. Wasn't fun to debug in release as nothing logs at all, debug builds didn't repro, and GDScript skips all the failures that ensue silently.

At the very least I would expect debug builds to behave the same and strip the call... so that the build can be, well, debugged.

Having debug-only code block/line prefix explicitely could be nice to have instead of assert magically stripping.

debug assert(test)

debug:
    assert(a)
    assert(b)
    print(c)

Although naming would be quite unfortunate if this is stripped too in debug builds. Because when I debug, I usually use the editor xD A debug build is something else