Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow non-constant string message for assert #62695

Merged
merged 1 commit into from Oct 31, 2022

Conversation

Spartan322
Copy link
Contributor

@Spartan322 Spartan322 commented Jul 4, 2022

Fixes #47157
Based on #47165 reimplemented with modifications suggested from #47165 (comment)

Removes constant string requirement for assert message, analyzer does not account for some dynamic assignment.

Original Behavior

assert(false, "Constant String")

var _testDynamicString = "test"
assert(false, _testDynamicString) # Errors: Expected constant string for assert error message

var _testExplicitString : String = "test"
assert(false, _testExplicitString) # Errors: Expected constant string for assert error message

var _testImplicitString := "test"
assert(false, _testImplicitString) # Errors: Expected constant string for assert error message

var _testExplicitHint : int = 1
assert(false, _testExplicitHint) # Errors: Expected constant string for assert error message

var _testImplicitHint := 1
assert(false, _testImplicitHint) # Errors: Expected constant string for assert error message

var _testDynamicAssign = 1
assert(false, _testDynamicAssign) # Errors: Expected constant string for assert error message
_testDynamicAssign = "test"
assert(false, _testDynamicAssign) # Errors: Expected constant string for assert error message
_testDynamicAssign = 1
assert(false, _testDynamicAssign) # Errors: Expected constant string for assert error message

New Behavior

assert(false, "Constant String")

var _testDynamicString = "test"
assert(false, _testDynamicString)

var _testExplicitString : String = "test"
assert(false, _testExplicitString)

var _testImplicitString := "test"
assert(false, _testImplicitString)

var _testExplicitHint : int = 1
assert(false, _testExplicitHint) # Errors: Expected string for assert error message

var _testImplicitHint := 1
assert(false, _testImplicitHint) # Errors: Expected string for assert error message

var _testDynamicAssign = 1
assert(false, _testDynamicAssign) # Errors: Expected string for assert error message
_testDynamicAssign = "test"
assert(false, _testDynamicAssign)
_testDynamicAssign = 1
assert(false, _testDynamicAssign)

Potential Considerations

Since there is the capacity for non-string assert messages to escape the error here, as was suggested at #47157 (comment) a warning may be suitable in place of the original error. I was unsure whether that was desirable to put here or not so I have left any attempt out.

@Spartan322 Spartan322 requested a review from a team as a code owner July 4, 2022 04:55
@akien-mga akien-mga added this to the 4.0 milestone Jul 4, 2022
@KoBeWi
Copy link
Member

KoBeWi commented Aug 25, 2022

Seems like this will happily accept non-string values and use them as a message. Maybe it should result in runtime error?
Although if it works then no reason to block it I guess 🤔

The PR fixes the issue, so it would be nice to get it merged. It blocks porting of my plugin to 4.0 >_>

Copy link
Member

@Chaosus Chaosus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works okay, but the error should be emitted for the following statement

	var _testDynamicAssign = "TEST"
	_testDynamicAssign = 1
	assert(false, _testDynamicAssign) # Prints Assertion failed: 1

the type retrieved by _reduce_expression is incorrect, but the fixing may be difficult, so I would approve it if the fix is urgent since it correct (but incomplete).

@TokisanGames
Copy link
Contributor

Most of us are using formatted strings in asserts and aren't so concerned about an error from printing a number. Many of us are also static typing, so will never encounter this. I have all of my asserts commented out and replaced with dummy constant strings. It would be nice to merge this so I can undo all of those hacks and get informative asserts sooner rather than later.

@akien-mga akien-mga merged commit b43cc96 into godotengine:master Oct 31, 2022
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Asserts take only constant strings
6 participants