Fix expressions involving a variable (or property) of type number/string wrongly rejected when an operator like + was used after them (#6467)

* For example, "Your score is " + NumberVariable + " points" was rejected, because the second operator type was not properly inferred.
* If something does not work in your game anymore, double check if your expressions in the events sheets are not underlined in red. Sometimes, adding `ToString(` or `ToNumber(` around it might be required.
This commit is contained in:
Florian Rival
2024-03-28 09:42:41 +01:00
committed by GitHub
parent 0dd4650aae
commit 7e8b44af2e
3 changed files with 478 additions and 6 deletions

View File

@@ -86,8 +86,10 @@ class GD_CORE_API ExpressionValidator : public ExpressionParser2NodeWorker {
void OnVisitOperatorNode(OperatorNode& node) override {
ReportAnyError(node);
// The "required" type ("parentType") will be used when visiting the first operand.
// Note that it may be refined thanks to this first operand (see later).
node.leftHandSide->Visit(*this);
const Type leftType = childType;
const Type leftType = childType; // Store the type of the first operand.
if (leftType == Type::Number) {
if (node.op == ' ') {
@@ -120,15 +122,19 @@ class GD_CORE_API ExpressionValidator : public ExpressionParser2NodeWorker {
node.rightHandSide->location);
}
parentType = leftType;
// The "required" type ("parentType") of the second operator is decided by:
// - the parent type. Unless it can (`number|string`) or should (`unknown`) be refined, then:
// - the first operand.
parentType = ShouldTypeBeRefined(parentType) ? leftType : parentType;
node.rightHandSide->Visit(*this);
const Type rightType = childType;
// The type is decided by the first operand, unless it can (`number|string`)
// or should (`unknown`) be refined, in which case we go for the right
// operand (which got visited knowing the type of the first operand, so it's
// The type of the overall operator ("childType") is decided by:
// - the parent type. Unless it can (`number|string`) or should (`unknown`) be refined, then:
// - the first operand. Unless it can (`number|string`) or should (`unknown`) be refined, then:
// - the right operand (which got visited knowing the type of the first operand, so it's
// equal or strictly more precise than the left operand).
childType = (leftType == Type::Unknown || leftType == Type::NumberOrString) ? leftType : rightType;
childType = ShouldTypeBeRefined(parentType) ? (ShouldTypeBeRefined(leftType) ? leftType : rightType) : parentType;
}
void OnVisitUnaryOperatorNode(UnaryOperatorNode& node) override {
ReportAnyError(node);
@@ -395,6 +401,10 @@ class GD_CORE_API ExpressionValidator : public ExpressionParser2NodeWorker {
}
}
static bool ShouldTypeBeRefined(Type type) {
return (type == Type::Unknown || type == Type::NumberOrString);
}
static Type StringToType(const gd::String &type);
static const gd::String &TypeToString(Type type);
static const gd::String unknownTypeString;

View File

@@ -30,6 +30,21 @@ TEST_CASE("ExpressionCodeGenerator", "[common][events]") {
layout1.GetVariables().InsertNew("MySceneBooleanVariable").SetBool(true);
layout1.GetVariables().InsertNew("MySceneStructureVariable").GetChild("MyChild");
layout1.GetVariables().InsertNew("MySceneStructureVariable2").GetChild("MyChild");
layout1.GetVariables().InsertNew("MySceneEmptyArrayVariable").CastTo(gd::Variable::Type::Array);
{
auto& variable = layout1.GetVariables().InsertNew("MySceneNumberArrayVariable");
variable.CastTo(gd::Variable::Type::Array);
variable.PushNew().SetValue(1);
variable.PushNew().SetValue(2);
variable.PushNew().SetValue(3);
}
{
auto& variable = layout1.GetVariables().InsertNew("MySceneStringArrayVariable");
variable.CastTo(gd::Variable::Type::Array);
variable.PushNew().SetString("1");
variable.PushNew().SetString("2");
variable.PushNew().SetString("3");
}
auto &mySpriteObject = layout1.InsertNewObject(project, "MyExtension::Sprite", "MySpriteObject", 0);
mySpriteObject.GetVariables().InsertNew("MyNumberVariable").SetValue(123);
@@ -1295,6 +1310,221 @@ TEST_CASE("ExpressionCodeGenerator", "[common][events]") {
REQUIRE(output == "getVariableForObject(MySpriteObject, MyOtherSpriteObject).getChild(\"Child\").getChild(\"Grandchild\")");
}
}
SECTION("Type conversions (valid operators with variables having different types than the expression)") {
SECTION("Expression/parent type is 'string'") {
{
auto node =
parser.ParseExpression("\"You have \" + MySceneVariable + \" points\"");
gd::ExpressionCodeGenerator expressionCodeGenerator("string",
"",
codeGenerator,
context);
REQUIRE(node);
node->Visit(expressionCodeGenerator);
REQUIRE(expressionCodeGenerator.GetOutput() == "\"You have \" + getLayoutVariable(MySceneVariable).getAsString() + \" points\"");
}
{
auto node =
parser.ParseExpression("MySceneVariable + MySceneStringVariable");
gd::ExpressionCodeGenerator expressionCodeGenerator("string",
"",
codeGenerator,
context);
REQUIRE(node);
node->Visit(expressionCodeGenerator);
REQUIRE(expressionCodeGenerator.GetOutput() == "getLayoutVariable(MySceneVariable).getAsString() + getLayoutVariable(MySceneStringVariable).getAsString()");
}
}
SECTION("Expression/parent type is 'string' (with an unknown variable)") {
{
auto node =
parser.ParseExpression("\"You have \" + MySceneStructureVariable.MyChild.CantKnownTheTypeSoStayGeneric + \" points\"");
gd::ExpressionCodeGenerator expressionCodeGenerator("string",
"",
codeGenerator,
context);
REQUIRE(node);
node->Visit(expressionCodeGenerator);
REQUIRE(expressionCodeGenerator.GetOutput() == "\"You have \" + getLayoutVariable(MySceneStructureVariable).getChild(\"MyChild\").getChild(\"CantKnownTheTypeSoStayGeneric\").getAsString() + \" points\"");
}
}
SECTION("Expression/parent type is 'string' (2 number variables)") {
{
auto node =
parser.ParseExpression("MySceneVariable + MySceneVariable2 + \"world\"");
gd::ExpressionCodeGenerator expressionCodeGenerator("string",
"",
codeGenerator,
context);
REQUIRE(node);
node->Visit(expressionCodeGenerator);
REQUIRE(expressionCodeGenerator.GetOutput() == "getLayoutVariable(MySceneVariable).getAsString() + getLayoutVariable(MySceneVariable2).getAsString() + \"world\"");
}
{
auto node =
parser.ParseExpression("MySceneVariable + MySceneVariable2 + MySceneStringVariable");
gd::ExpressionCodeGenerator expressionCodeGenerator("string",
"",
codeGenerator,
context);
REQUIRE(node);
node->Visit(expressionCodeGenerator);
REQUIRE(expressionCodeGenerator.GetOutput() == "getLayoutVariable(MySceneVariable).getAsString() + getLayoutVariable(MySceneVariable2).getAsString() + getLayoutVariable(MySceneStringVariable).getAsString()");
}
}
SECTION("Expression/parent type is 'string' (array variable)") {
{
auto node =
parser.ParseExpression("\"hello\" + MySceneNumberArrayVariable[2] + \"world\"");
gd::ExpressionCodeGenerator expressionCodeGenerator("string",
"",
codeGenerator,
context);
REQUIRE(node);
node->Visit(expressionCodeGenerator);
REQUIRE(expressionCodeGenerator.GetOutput() == "\"hello\" + getLayoutVariable(MySceneNumberArrayVariable).getChild(2).getAsString() + \"world\"");
}
{
auto node =
parser.ParseExpression("\"hello\" + MySceneEmptyArrayVariable[2] + \"world\"");
gd::ExpressionCodeGenerator expressionCodeGenerator("string",
"",
codeGenerator,
context);
REQUIRE(node);
node->Visit(expressionCodeGenerator);
REQUIRE(expressionCodeGenerator.GetOutput() == "\"hello\" + getLayoutVariable(MySceneEmptyArrayVariable).getChild(2).getAsString() + \"world\"");
}
}
SECTION("Expression/parent type is 'number'") {
{
auto node =
parser.ParseExpression("123 + MySceneVariable + 456");
gd::ExpressionCodeGenerator expressionCodeGenerator("number",
"",
codeGenerator,
context);
REQUIRE(node);
node->Visit(expressionCodeGenerator);
REQUIRE(expressionCodeGenerator.GetOutput() == "123 + getLayoutVariable(MySceneVariable).getAsNumber() + 456");
}
{
auto node =
parser.ParseExpression("MySceneStringVariable + MySceneVariable");
gd::ExpressionCodeGenerator expressionCodeGenerator("number",
"",
codeGenerator,
context);
REQUIRE(node);
node->Visit(expressionCodeGenerator);
REQUIRE(expressionCodeGenerator.GetOutput() == "getLayoutVariable(MySceneStringVariable).getAsNumber() + getLayoutVariable(MySceneVariable).getAsNumber()");
}
}
SECTION("Expression/parent type is 'string' (with an unknown variable)") {
{
auto node =
parser.ParseExpression("123 + MySceneStructureVariable.MyChild.CantKnownTheTypeSoStayGeneric + 456");
gd::ExpressionCodeGenerator expressionCodeGenerator("number",
"",
codeGenerator,
context);
REQUIRE(node);
node->Visit(expressionCodeGenerator);
REQUIRE(expressionCodeGenerator.GetOutput() == "123 + getLayoutVariable(MySceneStructureVariable).getChild(\"MyChild\").getChild(\"CantKnownTheTypeSoStayGeneric\").getAsNumber() + 456");
}
}
SECTION("Expression/parent type is 'number' (2 string variables)") {
{
auto node =
parser.ParseExpression("MySceneStringVariable + MySceneStringVariable + 456");
gd::ExpressionCodeGenerator expressionCodeGenerator("number",
"",
codeGenerator,
context);
REQUIRE(node);
node->Visit(expressionCodeGenerator);
REQUIRE(expressionCodeGenerator.GetOutput() == "getLayoutVariable(MySceneStringVariable).getAsNumber() + getLayoutVariable(MySceneStringVariable).getAsNumber() + 456");
}
{
auto node =
parser.ParseExpression("MySceneStringVariable + MySceneStringVariable + MySceneVariable");
gd::ExpressionCodeGenerator expressionCodeGenerator("number",
"",
codeGenerator,
context);
REQUIRE(node);
node->Visit(expressionCodeGenerator);
REQUIRE(expressionCodeGenerator.GetOutput() == "getLayoutVariable(MySceneStringVariable).getAsNumber() + getLayoutVariable(MySceneStringVariable).getAsNumber() + getLayoutVariable(MySceneVariable).getAsNumber()");
}
}
SECTION("Expression/parent type is 'number' (array variable)") {
{
auto node =
parser.ParseExpression("123 + MySceneNumberArrayVariable[2] + 456");
gd::ExpressionCodeGenerator expressionCodeGenerator("number",
"",
codeGenerator,
context);
REQUIRE(node);
node->Visit(expressionCodeGenerator);
REQUIRE(expressionCodeGenerator.GetOutput() == "123 + getLayoutVariable(MySceneNumberArrayVariable).getChild(2).getAsNumber() + 456");
}
{
auto node =
parser.ParseExpression("123 + MySceneEmptyArrayVariable[2] + 456");
gd::ExpressionCodeGenerator expressionCodeGenerator("number",
"",
codeGenerator,
context);
REQUIRE(node);
node->Visit(expressionCodeGenerator);
REQUIRE(expressionCodeGenerator.GetOutput() == "123 + getLayoutVariable(MySceneEmptyArrayVariable).getChild(2).getAsNumber() + 456");
}
}
SECTION("Multiple type conversions in sub expressions or same expression") {
{
auto node =
parser.ParseExpression("\"hello\" + MySceneNumberArrayVariable[2 + MySceneStringVariable] + \"world\" + MySceneVariable + \"world 2\"");
gd::ExpressionCodeGenerator expressionCodeGenerator("string",
"",
codeGenerator,
context);
REQUIRE(node);
node->Visit(expressionCodeGenerator);
REQUIRE(expressionCodeGenerator.GetOutput() == "\"hello\" + getLayoutVariable(MySceneNumberArrayVariable).getChild(2 + getLayoutVariable(MySceneStringVariable).getAsNumber()).getAsString() + \"world\" + getLayoutVariable(MySceneVariable).getAsString() + \"world 2\"");
}
{
auto node =
parser.ParseExpression("\"hello\" + MySceneNumberArrayVariable[\"foo\" + MySceneVariable + \"bar\"] + \"world\" + MySceneVariable + \"world 2\"");
gd::ExpressionCodeGenerator expressionCodeGenerator("string",
"",
codeGenerator,
context);
REQUIRE(node);
node->Visit(expressionCodeGenerator);
REQUIRE(expressionCodeGenerator.GetOutput() == "\"hello\" + getLayoutVariable(MySceneNumberArrayVariable).getChild(\"foo\" + getLayoutVariable(MySceneVariable).getAsString() + \"bar\").getAsString() + \"world\" + getLayoutVariable(MySceneVariable).getAsString() + \"world 2\"");
}
}
}
SECTION("Mixed test (1)") {
{
auto node = parser.ParseExpression("-+-MyExtension::MouseX(,)");

View File

@@ -2954,6 +2954,238 @@ TEST_CASE("ExpressionParser2", "[common][events]") {
}
}
SECTION("Valid operators with variables having different types than the expression") {
SECTION("Expression/parent type is 'string'") {
// A trivial test (everything is a string).
{
auto node = parser.ParseExpression("\"You have \" + MySceneStringVariable + \" points\"");
REQUIRE(node != nullptr);
gd::ExpressionValidator validator(platform, projectScopedContainers, "string");
node->Visit(validator);
REQUIRE(validator.GetFatalErrors().size() == 0);
}
// A string concatenated with a number variable (will have to be casted to a string in code generation)
{
auto node = parser.ParseExpression("\"You have \" + MySceneNumberVariable");
REQUIRE(node != nullptr);
gd::ExpressionValidator validator(platform, projectScopedContainers, "string");
node->Visit(validator);
REQUIRE(validator.GetFatalErrors().size() == 0);
}
// A string concatenated with a number variable (will have to be casted to a string in code generation)
// and then with a string again.
{
auto node = parser.ParseExpression("\"You have \" + MySceneNumberVariable + \" points\"");
REQUIRE(node != nullptr);
gd::ExpressionValidator validator(platform, projectScopedContainers, "string");
node->Visit(validator);
REQUIRE(validator.GetFatalErrors().size() == 0);
}
// A string concatenated with an unknown variable (will have to be casted to a string in code generation)
// and then with a string again.
{
auto node = parser.ParseExpression("\"You have \" + MySceneStructureVariable.MyChild.CantKnownTheTypeSoStayGeneric + \" points\"");
REQUIRE(node != nullptr);
gd::ExpressionValidator validator(platform, projectScopedContainers, "string");
node->Visit(validator);
REQUIRE(validator.GetFatalErrors().size() == 0);
}
}
SECTION("Expression/parent type is 'number'") {
// A trivial test (everything is a string).
{
auto node = parser.ParseExpression("123 + MySceneNumberVariable + 456");
REQUIRE(node != nullptr);
gd::ExpressionValidator validator(platform, projectScopedContainers, "number");
node->Visit(validator);
REQUIRE(validator.GetFatalErrors().size() == 0);
}
// A number concatenated with a string variable (will have to be casted to a number in code generation)
{
auto node = parser.ParseExpression("123 + MySceneStringVariable");
REQUIRE(node != nullptr);
gd::ExpressionValidator validator(platform, projectScopedContainers, "number");
node->Visit(validator);
REQUIRE(validator.GetFatalErrors().size() == 0);
}
// A number concatenated with a string variable (will have to be casted to a number in code generation)
// and then with a number again.
{
auto node = parser.ParseExpression("123 + MySceneStringVariable + 456");
REQUIRE(node != nullptr);
gd::ExpressionValidator validator(platform, projectScopedContainers, "number");
node->Visit(validator);
REQUIRE(validator.GetFatalErrors().size() == 0);
}
// A number concatenated with an unknown variable (will have to be casted to a number in code generation)
// and then with a number again.
{
auto node = parser.ParseExpression("123 + MySceneStructureVariable.MyChild.CantKnownTheTypeSoStayGeneric + 456");
REQUIRE(node != nullptr);
gd::ExpressionValidator validator(platform, projectScopedContainers, "number");
node->Visit(validator);
REQUIRE(validator.GetFatalErrors().size() == 0);
}
}
SECTION("Expression/parent type is 'number|string'") {
SECTION("Expression/parent inferred type is 'string'") {
// A trivial test (everything is a string).
{
auto node = parser.ParseExpression("\"You have \" + MySceneStringVariable + \" points\"");
REQUIRE(node != nullptr);
gd::ExpressionValidator validator(platform, projectScopedContainers, "number|string");
node->Visit(validator);
REQUIRE(validator.GetFatalErrors().size() == 0);
}
// A string concatenated with a number variable (will have to be casted to a string in code generation)
{
auto node = parser.ParseExpression("\"You have \" + MySceneNumberVariable");
REQUIRE(node != nullptr);
gd::ExpressionValidator validator(platform, projectScopedContainers, "number|string");
node->Visit(validator);
REQUIRE(validator.GetFatalErrors().size() == 0);
}
// A string concatenated with a number variable (will have to be casted to a string in code generation)
// and then with a string again.
{
auto node = parser.ParseExpression("\"You have \" + MySceneNumberVariable + \" points\"");
REQUIRE(node != nullptr);
gd::ExpressionValidator validator(platform, projectScopedContainers, "number|string");
node->Visit(validator);
REQUIRE(validator.GetFatalErrors().size() == 0);
}
// A string concatenated with an unknown variable (will have to be casted to a string in code generation)
// and then with a string again.
{
auto node = parser.ParseExpression("\"You have \" + MySceneStructureVariable.MyChild.CantKnownTheTypeSoStayGeneric + \" points\"");
REQUIRE(node != nullptr);
gd::ExpressionValidator validator(platform, projectScopedContainers, "number|string");
node->Visit(validator);
REQUIRE(validator.GetFatalErrors().size() == 0);
}
}
SECTION("Expression/parent inferred type is 'number'") {
// A trivial test (everything is a string).
{
auto node = parser.ParseExpression("123 + MySceneNumberVariable + 456");
REQUIRE(node != nullptr);
gd::ExpressionValidator validator(platform, projectScopedContainers, "number|string");
node->Visit(validator);
REQUIRE(validator.GetFatalErrors().size() == 0);
}
// A number concatenated with a string variable (will have to be casted to a number in code generation)
{
auto node = parser.ParseExpression("123 + MySceneStringVariable");
REQUIRE(node != nullptr);
gd::ExpressionValidator validator(platform, projectScopedContainers, "number|string");
node->Visit(validator);
REQUIRE(validator.GetFatalErrors().size() == 0);
}
// A number concatenated with a string variable (will have to be casted to a number in code generation)
// and then with a number again.
{
auto node = parser.ParseExpression("123 + MySceneStringVariable + 456");
REQUIRE(node != nullptr);
gd::ExpressionValidator validator(platform, projectScopedContainers, "number|string");
node->Visit(validator);
REQUIRE(validator.GetFatalErrors().size() == 0);
}
// A number concatenated with an unknown variable (will have to be casted to a number in code generation)
// and then with a number again.
{
auto node = parser.ParseExpression("123 + MySceneStructureVariable.MyChild.CantKnownTheTypeSoStayGeneric + 456");
REQUIRE(node != nullptr);
gd::ExpressionValidator validator(platform, projectScopedContainers, "number|string");
node->Visit(validator);
REQUIRE(validator.GetFatalErrors().size() == 0);
}
}
}
}
SECTION("Invalid operators with variables having different types than the expression") {
// Try to do a sum between numbers in a string expression
{
auto node = parser.ParseExpression("\"You have \" + MySceneNumberVariable + 2 + \" points\"");
REQUIRE(node != nullptr);
gd::ExpressionValidator validator(platform, projectScopedContainers, "string");
node->Visit(validator);
REQUIRE(validator.GetFatalErrors().size() == 1);
REQUIRE(validator.GetFatalErrors()[0]->GetMessage() == "You entered a number, but a text was expected (in quotes).");
REQUIRE(validator.GetFatalErrors()[0]->GetStartPosition() == 38);
REQUIRE(validator.GetFatalErrors()[0]->GetEndPosition() == 39);
}
// Try to do a sum between numbers in a number|string expression (that is inferred as a string with the first operand)
{
auto node = parser.ParseExpression("\"You have \" + MySceneNumberVariable + 2 + \" points\"");
REQUIRE(node != nullptr);
gd::ExpressionValidator validator(platform, projectScopedContainers, "string");
node->Visit(validator);
REQUIRE(validator.GetFatalErrors().size() == 1);
REQUIRE(validator.GetFatalErrors()[0]->GetMessage() == "You entered a number, but a text was expected (in quotes).");
REQUIRE(validator.GetFatalErrors()[0]->GetStartPosition() == 38);
REQUIRE(validator.GetFatalErrors()[0]->GetEndPosition() == 39);
}
// Try to do a string concatenation in a number expression
{
auto node = parser.ParseExpression("123 + MySceneStringVariable + \"hello\" + 456");
REQUIRE(node != nullptr);
gd::ExpressionValidator validator(platform, projectScopedContainers, "number");
node->Visit(validator);
REQUIRE(validator.GetFatalErrors().size() == 1);
REQUIRE(validator.GetFatalErrors()[0]->GetMessage() == "You entered a text, but a number was expected.");
REQUIRE(validator.GetFatalErrors()[0]->GetStartPosition() == 30);
REQUIRE(validator.GetFatalErrors()[0]->GetEndPosition() == 37);
}
// Try to do a string concatenation in a number|string expression (that is inferred as a number with the first operand)
{
auto node = parser.ParseExpression("123 + MySceneStringVariable + \"hello\" + 456");
REQUIRE(node != nullptr);
gd::ExpressionValidator validator(platform, projectScopedContainers, "number|string");
node->Visit(validator);
REQUIRE(validator.GetFatalErrors().size() == 1);
REQUIRE(validator.GetFatalErrors()[0]->GetMessage() == "You entered a text, but a number was expected.");
REQUIRE(validator.GetFatalErrors()[0]->GetStartPosition() == 30);
REQUIRE(validator.GetFatalErrors()[0]->GetEndPosition() == 37);
}
}
SECTION("Valid function call with object variable") {
{
// Note that in this test we need to use an expression with "objectvar",