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

Inconsistent implicit conversion between types in debug and release builds when export hint doesn't match default initialization #39215

Closed
TomWor opened this issue Jun 1, 2020 · 10 comments

Comments

@TomWor
Copy link

TomWor commented Jun 1, 2020

Godot version:
3.2 Stable

Issue description:
This code doesn't lead to an editor warning (exporting int when value is String).
It does run in the editor when pressing play, but it does not work on export.
There it gives the error message: "Invalid operands 'String' and 'String' in operator '-(negation)'"

export(int) var speed = "8"
...
func _moveTrack(delta):
    for tile in current_active_track_tiles:
        tile.translate(Vector3(0,0,-speed * delta))
@ghost
Copy link

ghost commented Jun 1, 2020

It should work if you remove the quotation marks.
export(int) var speed = 8

@ThakeeNathees
Copy link
Contributor

ThakeeNathees commented Jun 1, 2020

when you press play press play, the editor casting the variable speed to int by the export hint (int). but with out the editor it'll be "8" string and that's why the error, I don't think it's a bug, but need core developers' opinion for the expected behavior.
and adding types everywhere gives warning like export(int) var x:String = '8' or export(int) var x:int = '8'.

@TomWor
Copy link
Author

TomWor commented Jun 1, 2020

@JulianDH97 Thanks, I had already figured that out, just wanted to file it here as I think this is a bug.

@ThakeeNathees I'll use static types more often now, it seems a good idea to catch those issues.
Your explanation makes sense but I still think this should be considered an issue. It should at least print a warning in the console. I tested the game in-editor and everything worked fine until I exported the project and then the issue popped up - I think this should at least be seen as a usability issue that could be smoothed out by a warning or error in the console.

@hilfazer
Copy link
Contributor

hilfazer commented Jun 5, 2020

Invalid operands 'String' and 'String' in operator '-(negation)'

Weird message. 2 operands for an unary operator?

Otherwise there's no bug - your variable was typed dynamically so it was possible to assign anything to it. Export hint only limits what can be set via Inspector.

@TomWor
Copy link
Author

TomWor commented Jun 5, 2020

Shouldn't the editor play action closely match the behaviour of the exported binary?

@Calinou
Copy link
Member

Calinou commented Jun 5, 2020

Shouldn't the editor play action closely match the behaviour of the exported binary?

In theory, yes. In practice, subtle bugs like these can appear as the exported binary doesn't include RTTI (run-time type information) to decrease its binary size. Not to mention many debugging checks are also disabled in release builds.

@akien-mga akien-mga changed the title Invalid operands 'String' and 'String' in operator Inconsistent implicit conversion from String to int between debug and release builds Feb 10, 2022
@akien-mga akien-mga added this to the 3.5 milestone Feb 10, 2022
@akien-mga
Copy link
Member

Confirmed in 3.x (7244ea7), it's probably a duplicate of #56609.

@akien-mga akien-mga changed the title Inconsistent implicit conversion from String to int between debug and release builds Inconsistent implicit conversion between types in debug and release builds when export hint doesn't match default initialization Feb 10, 2022
@akien-mga
Copy link
Member

We discussed this on chat with @vnen, the way we'd propose to fix this is:

  • Don't coerce variable types to that set in the export hint on debug. So export(int) var speed = "8" would still be a String and behave a such, unless speed is changed in the Inspector, which will present an int spin slider. That may lead to surprises for the developer, but export(int) var speed = "8" is invalid code and asking for trouble :)
  • To mitigate the above, a warning should be introduced for when the export hint type doesn't match the type of the default value. This could likely also be raised when the export hint type (export(int)) doesn't match an actual type hint (inferred or explicit). So all these cases should IMO raise a warning:
export(int) var speed = "8"  # Warning
export(int) var speed := "8"  # Warning
export(int) var speed : String = "8"  # Warning

This doesn't need to raise a warning per se as it would behave correctly:

export(int) var speed : int = "8"  # No warning

It's a bit silly in this case, but here's a case with the same issue and a more common use case:

export(float) var speed : float = 8  # No warning

Here a float is very clearly wanted and the explicit type hint should coerce 8 to 8.0.

On the other hand these should still raise a warning, as here no coercion would happen and thus errors will ensue:

export(float) var speed = 8  # Warning 
export(float) var speed := 8  # Warning

@vnen
Copy link
Member

vnen commented Feb 27, 2022

I realized I didn't comment on this, so here's my thoughts on this.

  • Don't coerce variable types to that set in the export hint on debug. So export(int) var speed = "8" would still be a String and behave a such, unless speed is changed in the Inspector, which will present an int spin slider. That may lead to surprises for the developer, but export(int) var speed = "8" is invalid code and asking for trouble :)

I do agree not coercing is the way to go, but doing so will reintroduce #27575 (which was fixed by adding this coercion). I couldn't figure out yet another for that. That's the reason I haven't done a PR for this yet.

  • To mitigate the above, a warning should be introduced for when the export hint type doesn't match the type of the default value. This could likely also be raised when the export hint type (export(int)) doesn't match an actual type hint (inferred or explicit). So all these cases should IMO raise a warning:

We could add the warning but keep the coercion for now, since the above bug is arguably worse than this one. At least until we can find a proper fix for that. The warning would tell that those values are coerced only in editor, not when exported.

@akien-mga
Copy link
Member

Fixed by #58686.

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

No branches or pull requests

7 participants