Fix variables from being renamed with a property (#7186)

2025-10-15 10:19:04 +00:00 · 2024-11-26 17:58:59 +01:00
parent e4f3db71c5
commit e111706a27
4 changed files with 176 additions and 67 deletions
--- a/Core/GDCore/IDE/Events/EventsPropertyReplacer.cpp
+++ b/Core/GDCore/IDE/Events/EventsPropertyReplacer.cpp
@@ -41,6 +41,7 @@ class GD_CORE_API ExpressionPropertyReplacer
      const gd::Platform& platform_,
      const gd::ProjectScopedContainers& projectScopedContainers_,
      const gd::PropertiesContainer& targetPropertiesContainer_,
+      bool isParentTypeAVariable_,
      const std::unordered_map<gd::String, gd::String>& oldToNewPropertyNames_,
      const std::unordered_set<gd::String>& removedPropertyNames_)
      : hasDoneRenaming(false),
@@ -48,6 +49,7 @@ class GD_CORE_API ExpressionPropertyReplacer
        platform(platform_),
        projectScopedContainers(projectScopedContainers_),
        targetPropertiesContainer(targetPropertiesContainer_),
+        isParentTypeAVariable(isParentTypeAVariable_),
        oldToNewPropertyNames(oldToNewPropertyNames_),
        removedPropertyNames(removedPropertyNames_){};
  virtual ~ExpressionPropertyReplacer(){};
@@ -69,16 +71,21 @@ class GD_CORE_API ExpressionPropertyReplacer
  void OnVisitNumberNode(NumberNode& node) override {}
  void OnVisitTextNode(TextNode& node) override {}
  void OnVisitVariableNode(VariableNode& node) override {
+    if (isParentTypeAVariable) {
+        // Do nothing, it's a variable.
+        if (node.child) node.child->Visit(*this);
+        return;
+    }
+
    auto& propertiesContainersList =
        projectScopedContainers.GetPropertiesContainersList();

    // The node represents a variable or an object name on which a variable
    // will be accessed, or a property with a child.

-    // Match the potential *new* name of the property, because refactorings are
-    // done after changes in the variables container.
    projectScopedContainers.MatchIdentifierWithName<void>(
-      GetPotentialNewName(node.name),
+      // The property name is changed after the refactor operation.
+      node.name,
      [&]() {
        // Do nothing, it's an object variable.
        if (node.child) node.child->Visit(*this);
@@ -100,16 +107,7 @@ class GD_CORE_API ExpressionPropertyReplacer
        // Do nothing, it's a parameter.
        if (node.child) node.child->Visit(*this);
      }, [&]() {
-        // This is something else - potentially a deleted property.
-        // Check if it's coming from the target container with
-        // properties to replace.
-        if (propertiesContainersList.HasPropertiesContainer(
-                targetPropertiesContainer)) {
-          // The node represents a property, that can come from the target
-          // (because the target is in the scope), replace or remove it:
-          RenameOrRemovePropertyOfTargetPropertyContainer(node.name);
-        }
-
+        // Do nothing, it's something else.
        if (node.child) node.child->Visit(*this);
      });
  }
@@ -118,17 +116,24 @@ class GD_CORE_API ExpressionPropertyReplacer
  }
  void OnVisitVariableBracketAccessorNode(
      VariableBracketAccessorNode& node) override {
+    bool isGrandParentTypeAVariable = isParentTypeAVariable;
+    isParentTypeAVariable = false;
    node.expression->Visit(*this);
+    isParentTypeAVariable = isGrandParentTypeAVariable;
    if (node.child) node.child->Visit(*this);
  }
  void OnVisitIdentifierNode(IdentifierNode& node) override {
+    if (isParentTypeAVariable) {
+        // Do nothing, it's a variable.
+        return;
+    }
+
    auto& propertiesContainersList =
        projectScopedContainers.GetPropertiesContainersList();

-    // Match the potential *new* name of the property, because refactorings are
-    // done after changes in the variables container.
    projectScopedContainers.MatchIdentifierWithName<void>(
-      GetPotentialNewName(node.identifierName),
+      // The property name is changed after the refactor operation
+      node.identifierName,
      [&]() {
        // Do nothing, it's an object variable.
      }, [&]() {
@@ -145,22 +150,29 @@ class GD_CORE_API ExpressionPropertyReplacer
      }, [&]() {
        // Do nothing, it's a parameter.
      }, [&]() {
-        // This is something else - potentially a deleted property.
-        // Check if it's coming from the target container with
-        // properties to replace.
-        if (propertiesContainersList.HasPropertiesContainer(
-                targetPropertiesContainer)) {
-          // The node represents a property, that can come from the target
-          // (because the target is in the scope), replace or remove it:
-          RenameOrRemovePropertyOfTargetPropertyContainer(node.identifierName);
-        }
+        // Do nothing, it's something else.
      });
  }
  void OnVisitObjectFunctionNameNode(ObjectFunctionNameNode& node) override {}
-  void OnVisitFunctionCallNode(FunctionCallNode& node) override {
-    for (auto& parameter : node.parameters) {
-      parameter->Visit(*this);
+  void OnVisitFunctionCallNode(FunctionCallNode &node) override {
+    bool isGrandParentTypeAVariable = isParentTypeAVariable;
+    for (auto &parameter : node.parameters) {
+      const auto &parameterMetadata =
+          gd::MetadataProvider::GetFunctionCallParameterMetadata(
+              platform, projectScopedContainers.GetObjectsContainersList(),
+              node, *parameter);
+      if (!parameterMetadata) {
+        continue;
+      }
+      const auto &parameterTypeMetadata =
+          parameterMetadata->GetValueTypeMetadata();
+      if (gd::EventsPropertyReplacer::CanContainProperty(
+              parameterTypeMetadata)) {
+        isParentTypeAVariable = parameterTypeMetadata.IsVariable();
+        parameter->Visit(*this);
+      }
    }
+    isParentTypeAVariable = isGrandParentTypeAVariable;
  }
  void OnVisitEmptyNode(EmptyNode& node) override {}

@@ -168,12 +180,6 @@ class GD_CORE_API ExpressionPropertyReplacer
  bool hasDoneRenaming;
  bool removedPropertyUsed;

-  const gd::String& GetPotentialNewName(const gd::String& oldName) {
-    return oldToNewPropertyNames.count(oldName) >= 1
-            ? oldToNewPropertyNames.find(oldName)->second
-            : oldName;
-  }
-
  bool RenameOrRemovePropertyOfTargetPropertyContainer(
      gd::String& propertyName) {
    if (oldToNewPropertyNames.count(propertyName) >= 1) {
@@ -198,6 +204,7 @@ class GD_CORE_API ExpressionPropertyReplacer
  const std::unordered_set<gd::String>& removedPropertyNames;

  gd::String objectNameToUseForVariableAccessor;
+  bool isParentTypeAVariable;
 };

 bool EventsPropertyReplacer::DoVisitInstruction(gd::Instruction& instruction,
@@ -216,20 +223,16 @@ bool EventsPropertyReplacer::DoVisitInstruction(gd::Instruction& instruction,
          const gd::Expression& parameterValue,
          size_t parameterIndex,
          const gd::String& lastObjectName) {
-        const gd::String& type = parameterMetadata.GetType();
-
-        if (!gd::ParameterMetadata::IsExpression("variable", type) &&
-            !gd::ParameterMetadata::IsExpression("number", type) &&
-            !gd::ParameterMetadata::IsExpression("string", type))
-          return;  // Not an expression that can contain properties.
-
+        if (!gd::EventsPropertyReplacer::CanContainProperty(
+          parameterMetadata.GetValueTypeMetadata())) {
+          return;
+        }
        auto node = parameterValue.GetRootNode();
        if (node) {
-          ExpressionPropertyReplacer renamer(platform,
-                                             GetProjectScopedContainers(),
-                                             targetPropertiesContainer,
-                                             oldToNewPropertyNames,
-                                             removedPropertyNames);
+          ExpressionPropertyReplacer renamer(
+              platform, GetProjectScopedContainers(), targetPropertiesContainer,
+              parameterMetadata.GetValueTypeMetadata().IsVariable(),
+              oldToNewPropertyNames, removedPropertyNames);
          node->Visit(renamer);

          if (renamer.IsRemovedPropertyUsed()) {
@@ -246,20 +249,16 @@ bool EventsPropertyReplacer::DoVisitInstruction(gd::Instruction& instruction,

 bool EventsPropertyReplacer::DoVisitEventExpression(
    gd::Expression& expression, const gd::ParameterMetadata& metadata) {
-  const gd::String& type = metadata.GetType();
-
-  if (!gd::ParameterMetadata::IsExpression("variable", type) &&
-      !gd::ParameterMetadata::IsExpression("number", type) &&
-      !gd::ParameterMetadata::IsExpression("string", type))
-    return false;  // Not an expression that can contain properties.
-
+  if (!gd::EventsPropertyReplacer::CanContainProperty(
+          metadata.GetValueTypeMetadata())) {
+    return false;
+  }
  auto node = expression.GetRootNode();
  if (node) {
-    ExpressionPropertyReplacer renamer(platform,
-                                       GetProjectScopedContainers(),
-                                       targetPropertiesContainer,
-                                       oldToNewPropertyNames,
-                                       removedPropertyNames);
+    ExpressionPropertyReplacer renamer(
+        platform, GetProjectScopedContainers(), targetPropertiesContainer,
+        metadata.GetValueTypeMetadata().IsVariable(), oldToNewPropertyNames,
+        removedPropertyNames);
    node->Visit(renamer);

    if (renamer.IsRemovedPropertyUsed()) {
@@ -272,6 +271,12 @@ bool EventsPropertyReplacer::DoVisitEventExpression(
  return false;
 }

+bool EventsPropertyReplacer::CanContainProperty(
+    const gd::ValueTypeMetadata &valueTypeMetadata) {
+  return valueTypeMetadata.IsVariable() || valueTypeMetadata.IsNumber() ||
+         valueTypeMetadata.IsString();
+}
+
 EventsPropertyReplacer::~EventsPropertyReplacer() {}

 }  // namespace gd
--- a/Core/GDCore/IDE/Events/EventsPropertyReplacer.h
+++ b/Core/GDCore/IDE/Events/EventsPropertyReplacer.h
@@ -41,6 +41,8 @@ class GD_CORE_API EventsPropertyReplacer
        removedPropertyNames(removedPropertyNames_){};
  virtual ~EventsPropertyReplacer();

+  static bool CanContainProperty(const gd::ValueTypeMetadata &valueTypeMetadata);
+
 private:
  bool DoVisitInstruction(gd::Instruction &instruction,
                          bool isCondition) override;
--- a/Core/tests/DummyPlatform.cpp
+++ b/Core/tests/DummyPlatform.cpp
@@ -265,9 +265,9 @@ void SetupProjectWithDummyPlatform(gd::Project& project,

  extension
      ->AddAction("SetNumberVariable",
-                  "Do something with number variables",
-                  "This does something with variables",
-                  "Do something with variables",
+                  "Change variable value",
+                  "Modify the number value of a variable.",
+                  "the variable _PARAM0_",
                  "",
                  "",
                  "")
@@ -278,9 +278,9 @@ void SetupProjectWithDummyPlatform(gd::Project& project,

  extension
      ->AddAction("SetStringVariable",
-                  "Do something with string variables",
-                  "This does something with variables",
-                  "Do something with variables",
+                  "Change text variable",
+                  "Modify the text (string) of a variable.",
+                  "the variable _PARAM0_",
                  "",
                  "",
                  "")
@@ -291,9 +291,9 @@ void SetupProjectWithDummyPlatform(gd::Project& project,

  extension
      ->AddAction("SetBooleanVariable",
-                  "Do something with boolean variables",
-                  "This does something with variables",
-                  "Do something with variables",
+                  "Change boolean variable",
+                  "Modify the boolean value of a variable.",
+                  "Change the variable _PARAM0_: _PARAM1_",
                  "",
                  "",
                  "")
@@ -358,6 +358,17 @@ void SetupProjectWithDummyPlatform(gd::Project& project,
      .AddParameter("soundfile", "Parameter 3 (an audio resource)")
      .SetFunctionName("doSomethingWithResources");

+  extension
+      ->AddAction("DoSomethingWithAnyVariable",
+                  "Do something with variables",
+                  "This does something with variables",
+                  "Do something with variables please",
+                  "",
+                  "",
+                  "")
+      .AddParameter("variable", "Any variable")
+      .SetFunctionName("doSomethingWithAnyVariable");
+
  extension
      ->AddAction("DoSomethingWithLegacyPreScopedVariables",
                  "Do something with variables",
--- a/Core/tests/WholeProjectRefactorer.cpp
+++ b/Core/tests/WholeProjectRefactorer.cpp
@@ -91,6 +91,20 @@ CreateInstructionWithNumberParameter(gd::Project &project,
  return event.GetActions().Insert(instruction);
 }

+const gd::Instruction &
+CreateInstructionWithVariableParameter(gd::Project &project,
+                                     gd::EventsList &events,
+                                     const gd::String &expression) {
+  gd::StandardEvent &event = dynamic_cast<gd::StandardEvent &>(
+      events.InsertNewEvent(project, "BuiltinCommonInstructions::Standard"));
+
+  gd::Instruction instruction;
+  instruction.SetType("MyExtension::DoSomethingWithAnyVariable");
+  instruction.SetParametersCount(1);
+  instruction.SetParameter(0, expression);
+  return event.GetActions().Insert(instruction);
+}
+
 enum TestEvent {
  FreeFunctionAction,
  FreeFunctionWithExpression,
@@ -3067,6 +3081,9 @@ TEST_CASE("WholeProjectRefactorer", "[common]") {
        eventsExtension, eventsBasedBehavior);
    auto &instruction = CreateInstructionWithNumberParameter(
        project, behaviorAction.GetEvents(), "MyProperty");
+    auto &instruction2 = CreateInstructionWithNumberParameter(
+        project, behaviorAction.GetEvents(),
+        "MyExtension::GetVariableAsNumber(MyVariable.MyChild[MyProperty])");

    gd::WholeProjectRefactorer::RenameEventsBasedBehaviorProperty(
        project, eventsExtension, eventsBasedBehavior, "MyProperty",
@@ -3074,6 +3091,39 @@ TEST_CASE("WholeProjectRefactorer", "[common]") {

    REQUIRE(instruction.GetParameter(0).GetPlainString() ==
            "MyRenamedProperty");
+    REQUIRE(instruction2.GetParameter(0).GetPlainString() ==
+            "MyExtension::GetVariableAsNumber(MyVariable.MyChild[MyRenamedProperty])");
+  }
+
+  SECTION("(Events based behavior) property not renamed (in variable parameter)") {
+    gd::Project project;
+    gd::Platform platform;
+    SetupProjectWithDummyPlatform(project, platform);
+    auto &eventsExtension = SetupProjectWithEventsFunctionExtension(project);
+    auto &eventsBasedBehavior =
+        eventsExtension.GetEventsBasedBehaviors().Get("MyEventsBasedBehavior");
+
+    auto &behaviorAction =
+        eventsBasedBehavior.GetEventsFunctions().InsertNewEventsFunction(
+            "MyBehaviorEventsFunction", 0);
+    gd::WholeProjectRefactorer::EnsureBehaviorEventsFunctionsProperParameters(
+        eventsExtension, eventsBasedBehavior);
+    // Properties can't actually be used in "variable" parameters.
+    auto &instruction = CreateInstructionWithVariableParameter(
+        project, behaviorAction.GetEvents(), "MyProperty");
+    auto &instruction2 = CreateInstructionWithNumberParameter(
+        project, behaviorAction.GetEvents(),
+        "MyExtension::GetVariableAsNumber(MyProperty)");
+
+    gd::WholeProjectRefactorer::RenameEventsBasedBehaviorProperty(
+        project, eventsExtension, eventsBasedBehavior, "MyProperty",
+        "MyRenamedProperty");
+
+    // "variable" parameters are left untouched.
+    REQUIRE(instruction.GetParameter(0).GetPlainString() ==
+            "MyProperty");
+    REQUIRE(instruction2.GetParameter(0).GetPlainString() ==
+            "MyExtension::GetVariableAsNumber(MyProperty)");
  }

  SECTION("(Events based behavior) shared property renamed") {
@@ -3142,6 +3192,9 @@ TEST_CASE("WholeProjectRefactorer", "[common]") {
        eventsExtension, eventsBasedBehavior);
    auto &instruction = CreateInstructionWithNumberParameter(
        project, behaviorAction.GetEvents(), "MySharedProperty");
+    auto &instruction2 = CreateInstructionWithNumberParameter(
+        project, behaviorAction.GetEvents(),
+        "MyExtension::GetVariableAsNumber(MyVariable.MyChild[MySharedProperty])");

    gd::WholeProjectRefactorer::RenameEventsBasedBehaviorSharedProperty(
        project, eventsExtension, eventsBasedBehavior, "MySharedProperty",
@@ -3149,6 +3202,8 @@ TEST_CASE("WholeProjectRefactorer", "[common]") {

    REQUIRE(instruction.GetParameter(0).GetPlainString() ==
            "MyRenamedSharedProperty");
+    REQUIRE(instruction2.GetParameter(0).GetPlainString() ==
+            "MyExtension::GetVariableAsNumber(MyVariable.MyChild[MyRenamedSharedProperty])");
  }

  SECTION("(Events based object) property renamed") {
@@ -3196,6 +3251,9 @@ TEST_CASE("WholeProjectRefactorer", "[common]") {
        eventsExtension, eventsBasedObject);
    auto &instruction = CreateInstructionWithNumberParameter(
        project, behaviorAction.GetEvents(), "MyProperty");
+    auto &instruction2 = CreateInstructionWithNumberParameter(
+        project, behaviorAction.GetEvents(),
+        "MyExtension::GetVariableAsNumber(MyVariable.MyChild[MyProperty])");

    gd::WholeProjectRefactorer::RenameEventsBasedObjectProperty(
        project, eventsExtension, eventsBasedObject, "MyProperty",
@@ -3203,6 +3261,39 @@ TEST_CASE("WholeProjectRefactorer", "[common]") {

    REQUIRE(instruction.GetParameter(0).GetPlainString() ==
            "MyRenamedProperty");
+    REQUIRE(instruction2.GetParameter(0).GetPlainString() ==
+            "MyExtension::GetVariableAsNumber(MyVariable.MyChild[MyRenamedProperty])");
+  }
+
+  SECTION("(Events based object) property not renamed (in variable parameter)") {
+    gd::Project project;
+    gd::Platform platform;
+    SetupProjectWithDummyPlatform(project, platform);
+    auto &eventsExtension = SetupProjectWithEventsFunctionExtension(project);
+    auto &eventsBasedObject =
+        eventsExtension.GetEventsBasedObjects().Get("MyEventsBasedObject");
+
+    auto &behaviorAction =
+        eventsBasedObject.GetEventsFunctions().InsertNewEventsFunction(
+            "MyObjectEventsFunction", 0);
+    gd::WholeProjectRefactorer::EnsureObjectEventsFunctionsProperParameters(
+        eventsExtension, eventsBasedObject);
+    // Properties can't actually be used in "variable" parameters.
+    auto &instruction = CreateInstructionWithVariableParameter(
+        project, behaviorAction.GetEvents(), "MyProperty");
+    auto &instruction2 = CreateInstructionWithNumberParameter(
+        project, behaviorAction.GetEvents(),
+        "MyExtension::GetVariableAsNumber(MyProperty)");
+
+    gd::WholeProjectRefactorer::RenameEventsBasedObjectProperty(
+        project, eventsExtension, eventsBasedObject, "MyProperty",
+        "MyRenamedProperty");
+
+    // "variable" parameters are left untouched.
+    REQUIRE(instruction.GetParameter(0).GetPlainString() ==
+            "MyProperty");
+    REQUIRE(instruction2.GetParameter(0).GetPlainString() ==
+            "MyExtension::GetVariableAsNumber(MyProperty)");
  }
 }
 // TODO: Check that this works when behaviors are attached to a child-object.