Merge pull request #1443 from 4ian/fix/visit-function-node

Fix WholeProjectRefactorer potential wrong renaming/moving of parameters

Core/GDCore/IDE/Events/ExpressionsParameterMover.cpp

@@ -87,7 +87,8 @@ class GD_CORE_API ExpressionParameterMover
         };
 
     if (node.functionName == functionName) {
-      if (!objectType.empty() && !node.objectName.empty()) {
+      if (behaviorType.empty() && !objectType.empty() &&
+          !node.objectName.empty()) {
         // Move parameter of an object function
         const gd::String& thisObjectType = gd::GetTypeOfObject(
             globalObjectsContainer, objectsContainer, node.objectName);
@@ -103,7 +104,7 @@ class GD_CORE_API ExpressionParameterMover
           moveParameter(node.parameters);
           hasDoneMoving = true;
         }
-      } else {
+      } else if (behaviorType.empty() && objectType.empty()) {
         // Move parameter of a free function
         moveParameter(node.parameters);
         hasDoneMoving = true;
@@ -119,10 +120,12 @@ class GD_CORE_API ExpressionParameterMover
   bool hasDoneMoving;
   const gd::ObjectsContainer& globalObjectsContainer;
   const gd::ObjectsContainer& objectsContainer;
-  const gd::String& behaviorType;  // The behavior type for which the expression
+  const gd::String& behaviorType;  // The behavior type of the function which
-                                   // must be replaced (optional)
+                                   // must have a parameter moved (optional).
-  const gd::String& objectType;    // The object type for which the expression
+  const gd::String& objectType;    // The object type of the function which
-                                   // must be replaced (optional)
+                                   // must have a parameter moved (optional). If
+                                   // `behaviorType` is not empty, it takes
+                                   // precedence over `objectType`.
   const gd::String& functionName;
   std::size_t oldIndex;
   std::size_t newIndex;

Core/GDCore/IDE/Events/ExpressionsRenamer.cpp

@@ -73,7 +73,8 @@ class GD_CORE_API ExpressionFunctionRenamer
   void OnVisitIdentifierNode(IdentifierNode& node) override {}
   void OnVisitFunctionNode(FunctionNode& node) override {
     if (node.functionName == oldFunctionName) {
-      if (!objectType.empty() && !node.objectName.empty()) {
+      if (behaviorType.empty() && !objectType.empty() &&
+          !node.objectName.empty()) {
         // Replace an object function
         const gd::String& thisObjectType = gd::GetTypeOfObject(
             globalObjectsContainer, objectsContainer, node.objectName);
@@ -89,7 +90,7 @@ class GD_CORE_API ExpressionFunctionRenamer
           node.functionName = newFunctionName;
           hasDoneRenaming = true;
         }
-      } else {
+      } else if (behaviorType.empty() && objectType.empty()) {
         // Replace a free function
         node.functionName = newFunctionName;
         hasDoneRenaming = true;
@@ -106,9 +107,11 @@ class GD_CORE_API ExpressionFunctionRenamer
   const gd::ObjectsContainer& globalObjectsContainer;
   const gd::ObjectsContainer& objectsContainer;
   const gd::String& behaviorType;  // The behavior type for which the expression
-                                   // must be replaced (optional)
+                                   // must be replaced (optional).
   const gd::String& objectType;    // The object type for which the expression
-                                   // must be replaced (optional)
+                                   // must be replaced (optional). If
+                                   // `behaviorType` is not empty, it takes
+                                   // precedence over `objectType`.
   const gd::String& oldFunctionName;
   const gd::String& newFunctionName;
 };

Core/GDCore/IDE/WholeProjectRefactorer.cpp

@@ -470,7 +470,17 @@ void WholeProjectRefactorer::RenameBehaviorProperty(
   auto& properties = eventsBasedBehavior.GetPropertyDescriptors();
   if (!properties.Has(oldPropertyName)) return;
 
-  const auto& property = properties.Get(oldPropertyName);
+  // Order is important: we first rename the expressions then the instructions,
+  // to avoid being unable to fetch the metadata (the types of parameters) of
+  // instructions after they are renamed.
+  gd::ExpressionsRenamer expressionRenamer =
+      gd::ExpressionsRenamer(project.GetCurrentPlatform());
+  expressionRenamer.SetReplacedBehaviorExpression(
+      GetBehaviorFullType(eventsFunctionsExtension.GetName(),
+                          eventsBasedBehavior.GetName()),
+      EventsBasedBehavior::GetPropertyExpressionName(oldPropertyName),
+      EventsBasedBehavior::GetPropertyExpressionName(newPropertyName));
+  ExposeProjectEvents(project, expressionRenamer);
 
   gd::InstructionsTypeRenamer actionRenamer = gd::InstructionsTypeRenamer(
       project,
@@ -495,15 +505,6 @@ void WholeProjectRefactorer::RenameBehaviorProperty(
           eventsBasedBehavior.GetName(),
           EventsBasedBehavior::GetPropertyConditionName(newPropertyName)));
   ExposeProjectEvents(project, conditionRenamer);
-
-  gd::ExpressionsRenamer expressionRenamer =
-      gd::ExpressionsRenamer(project.GetCurrentPlatform());
-  expressionRenamer.SetReplacedBehaviorExpression(
-      GetBehaviorFullType(eventsFunctionsExtension.GetName(),
-                          eventsBasedBehavior.GetName()),
-      EventsBasedBehavior::GetPropertyExpressionName(oldPropertyName),
-      EventsBasedBehavior::GetPropertyExpressionName(newPropertyName));
-  ExposeProjectEvents(project, expressionRenamer);
 }
 
 void WholeProjectRefactorer::RenameEventsBasedBehavior(

Core/GDCore/IDE/WholeProjectRefactorer.h

@@ -50,7 +50,10 @@ class GD_CORE_API WholeProjectRefactorer {
                                   gd::ArbitraryEventsWorkerWithContext& worker);
 
   /**
-   * \brief Refactor the project after an events function extension is renamed
+   * \brief Refactor the project **before** an events function extension is renamed.
+   *
+   * \warning Do the renaming of the specified extension after calling this.
+   * This is because the extension is expected to have its old name for the refactoring.
    */
   static void RenameEventsFunctionsExtension(
       gd::Project& project,
@@ -59,7 +62,10 @@ class GD_CORE_API WholeProjectRefactorer {
       const gd::String& newName);
 
   /**
-   * \brief Refactor the project after an events function is renamed
+   * \brief Refactor the project **before** an events function is renamed.
+   *
+   * \warning Do the renaming of the specified function after calling this.
+   * This is because the function is expected to have its old name for the refactoring.
    */
   static void RenameEventsFunction(
       gd::Project& project,
@@ -68,8 +74,11 @@ class GD_CORE_API WholeProjectRefactorer {
       const gd::String& newFunctionName);
 
   /**
-   * \brief Refactor the project after an events function of a behavior is
+   * \brief Refactor the project **before** an events function of a behavior is
    * renamed.
+   *
+   * \warning Do the renaming of the specified function after calling this.
+   * This is because the function is expected to have its old name for the refactoring.
    */
   static void RenameBehaviorEventsFunction(
       gd::Project& project,
@@ -79,8 +88,11 @@ class GD_CORE_API WholeProjectRefactorer {
       const gd::String& newFunctionName);
 
   /**
-   * \brief Refactor the project after an events function parameter
+   * \brief Refactor the project **before** an events function parameter
-   * was moved.
+   * is moved.
+   *
+   * \warning Do the move of the specified function parameters after calling this.
+   * This is because the function is expected to be in its old state for the refactoring.
    */
   static void MoveEventsFunctionParameter(
       gd::Project& project,
@@ -90,8 +102,11 @@ class GD_CORE_API WholeProjectRefactorer {
       std::size_t newIndex);
 
   /**
-   * \brief Refactor the project after the parmaeter of an events function of a
+   * \brief Refactor the project **before** the parameter of an events function of a
-   * behavior was moved.
+   * behavior is moved.
+   *
+   * \warning Do the move of the specified function parameters after calling this.
+   * This is because the function is expected to be in its old state for the refactoring.
    */
   static void MoveBehaviorEventsFunctionParameter(
       gd::Project& project,
@@ -102,8 +117,11 @@ class GD_CORE_API WholeProjectRefactorer {
       std::size_t newIndex);
 
   /**
-   * \brief Refactor the project after a property of a behavior is
+   * \brief Refactor the project **before** a property of a behavior is
    * renamed.
+   *
+   * \warning Do the renaming of the specified property after calling this.
+   * This is because the property is expected to have its old name for the refactoring.
    */
   static void RenameBehaviorProperty(
       gd::Project& project,
@@ -113,7 +131,10 @@ class GD_CORE_API WholeProjectRefactorer {
       const gd::String& newPropertyName);
 
   /**
-   * \brief Refactor the project after a behavior is renamed.
+   * \brief Refactor the project **before** a behavior is renamed.
+   *
+   * \warning Do the renaming of the specified behavior after calling this.
+   * This is because the behavior is expected to have its old name for the refactoring.
    */
   static void RenameEventsBasedBehavior(
       gd::Project& project,

Core/tests/WholeProjectRefactorer.cpp

@@ -126,8 +126,7 @@ gd::EventsFunctionsExtension &SetupProjectWithEventsFunctionExtension(
       layout.GetEvents().InsertEvent(event);
     }
 
-    // Create an event in the layout referring to
+    // Create an event in the layout using "MyProperty" action
-    // MyEventsExtension::MyEventsBasedBehavior::MyBehaviorEventsFunction
     {
       gd::StandardEvent event;
       gd::Instruction instruction;
@@ -138,6 +137,33 @@ gd::EventsFunctionsExtension &SetupProjectWithEventsFunctionExtension(
       layout.GetEvents().InsertEvent(event);
     }
 
+    // Create an event in the layout using "MyProperty" condition
+    {
+      gd::StandardEvent event;
+      gd::Instruction instruction;
+      instruction.SetType(
+          "MyEventsExtension::MyEventsBasedBehavior::" +
+          gd::EventsBasedBehavior::GetPropertyConditionName("MyProperty"));
+      event.GetConditions().Insert(instruction);
+      layout.GetEvents().InsertEvent(event);
+    }
+
+    // Create an event in the layout using "MyProperty" expression
+    {
+      gd::StandardEvent event;
+      gd::Instruction instruction;
+      instruction.SetType("MyExtension::DoSomething");
+      instruction.SetParametersCount(1);
+      instruction.SetParameter(
+          0,
+          gd::Expression(
+              "ObjectWithMyBehavior.MyBehavior::" +
+              gd::EventsBasedBehavior::GetPropertyExpressionName("MyProperty") +
+              "()"));
+      event.GetActions().Insert(instruction);
+      layout.GetEvents().InsertEvent(event);
+    }
+
     // Create an event in ExternalEvents1 referring to
     // MyEventsExtension::MyEventsBasedBehavior::MyBehaviorEventsFunctionExpression
     {
@@ -148,6 +174,22 @@ gd::EventsFunctionsExtension &SetupProjectWithEventsFunctionExtension(
       instruction.SetParameter(
           0,
           gd::Expression("1 + "
+                         "ObjectWithMyBehavior.MyBehavior::"
+                         "MyBehaviorEventsFunctionExpression(123, 456, 789)"));
+      event.GetActions().Insert(instruction);
+      externalEvents.GetEvents().InsertEvent(event);
+    }
+
+    // Create an event in ExternalEvents1 **wrongly** referring to
+    // MyEventsExtension::MyEventsBasedBehavior::MyBehaviorEventsFunctionExpression
+    {
+      gd::StandardEvent event;
+      gd::Instruction instruction;
+      instruction.SetType("MyExtension::DoSomething");
+      instruction.SetParametersCount(1);
+      instruction.SetParameter(
+          0,
+          gd::Expression("2 + "
                          "ObjectWithMyBehavior::MyBehavior."
                          "MyBehaviorEventsFunctionExpression(123, 456, 789)"));
       event.GetActions().Insert(instruction);
@@ -506,7 +548,7 @@ TEST_CASE("WholeProjectRefactorer", "[common]") {
                 .GetBehavior("MyBehavior")
                 .GetTypeName() == "MyRenamedExtension::MyEventsBasedBehavior");
 
-    // Check if events based behaviors functions have been renamed in
+    // Check if events-based behavior methods have been renamed in
     // instructions
     REQUIRE(static_cast<gd::StandardEvent &>(
                 project.GetLayout("LayoutWithBehaviorFunctions")
@@ -518,7 +560,7 @@ TEST_CASE("WholeProjectRefactorer", "[common]") {
             "MyRenamedExtension::MyEventsBasedBehavior::"
             "MyBehaviorEventsFunction");
 
-    // Check if events based behaviors properties have been renamed in
+    // Check if events-based behaviors properties have been renamed in
     // instructions
     REQUIRE(static_cast<gd::StandardEvent &>(
                 project.GetLayout("LayoutWithBehaviorFunctions")
@@ -530,7 +572,7 @@ TEST_CASE("WholeProjectRefactorer", "[common]") {
             "MyRenamedExtension::MyEventsBasedBehavior::"
             "SetPropertyMyProperty");
 
-    // Check events based behaviors functions have *not* been renamed in
+    // Check events-based behavior methods have *not* been renamed in
     // expressions
     REQUIRE(static_cast<gd::StandardEvent &>(
                 project.GetExternalEvents("ExternalEventsWithBehaviorFunctions")
@@ -541,7 +583,7 @@ TEST_CASE("WholeProjectRefactorer", "[common]") {
                 .GetParameter(0)
                 .GetPlainString() ==
             "1 + "
-            "ObjectWithMyBehavior::MyBehavior."
+            "ObjectWithMyBehavior.MyBehavior::"
             "MyBehaviorEventsFunctionExpression(123, 456, 789)");
   }
   SECTION("(Free) events function renamed") {
@@ -586,23 +628,18 @@ TEST_CASE("WholeProjectRefactorer", "[common]") {
     SetupProjectWithDummyPlatform(project, platform);
     auto &eventsExtension = SetupProjectWithEventsFunctionExtension(project);
 
-    gd::WholeProjectRefactorer::MoveEventsFunctionParameter(project,
-                                                     eventsExtension,
-                                                     "MyEventsFunction",
-                                                     0, 2);
     gd::WholeProjectRefactorer::MoveEventsFunctionParameter(
-        project,
+        project, eventsExtension, "MyEventsFunction", 0, 2);
-        eventsExtension,
+    gd::WholeProjectRefactorer::MoveEventsFunctionParameter(
-        "MyEventsFunctionExpression",
+        project, eventsExtension, "MyEventsFunctionExpression", 0, 1);
-        0, 1);
 
     // Check that events function calls in instructions have been updated
-    auto& action = static_cast<gd::StandardEvent &>(
+    auto &action = static_cast<gd::StandardEvent &>(
-                project.GetLayout("LayoutWithFreeFunctions")
+                       project.GetLayout("LayoutWithFreeFunctions")
-                    .GetEvents()
+                           .GetEvents()
-                    .GetEvent(0))
+                           .GetEvent(0))
-                .GetActions()
+                       .GetActions()
-                .Get(0);
+                       .Get(0);
     REQUIRE(action.GetParameter(0).GetPlainString() == "Second parameter");
     REQUIRE(action.GetParameter(1).GetPlainString() == "Third parameter");
     REQUIRE(action.GetParameter(2).GetPlainString() == "First parameter");
@@ -644,7 +681,7 @@ TEST_CASE("WholeProjectRefactorer", "[common]") {
                 .GetTypeName() ==
             "MyEventsExtension::MyRenamedEventsBasedBehavior");
 
-    // Check if events based behaviors functions have been renamed in
+    // Check if events-based behavior methods have been renamed in
     // instructions
     REQUIRE(static_cast<gd::StandardEvent &>(
                 project.GetLayout("LayoutWithBehaviorFunctions")
@@ -656,7 +693,7 @@ TEST_CASE("WholeProjectRefactorer", "[common]") {
             "MyEventsExtension::MyRenamedEventsBasedBehavior::"
             "MyBehaviorEventsFunction");
 
-    // Check if events based behaviors properties have been renamed in
+    // Check if events-based behaviors properties have been renamed in
     // instructions
     REQUIRE(static_cast<gd::StandardEvent &>(
                 project.GetLayout("LayoutWithBehaviorFunctions")
@@ -668,7 +705,7 @@ TEST_CASE("WholeProjectRefactorer", "[common]") {
             "MyEventsExtension::MyRenamedEventsBasedBehavior::"
             "SetPropertyMyProperty");
 
-    // Check events based behaviors functions have *not* been renamed in
+    // Check events-based behavior methods have *not* been renamed in
     // expressions
     REQUIRE(static_cast<gd::StandardEvent &>(
                 project.GetExternalEvents("ExternalEventsWithBehaviorFunctions")
@@ -679,7 +716,7 @@ TEST_CASE("WholeProjectRefactorer", "[common]") {
                 .GetParameter(0)
                 .GetPlainString() ==
             "1 + "
-            "ObjectWithMyBehavior::MyBehavior."
+            "ObjectWithMyBehavior.MyBehavior::"
             "MyBehaviorEventsFunctionExpression(123, 456, 789)");
   }
   SECTION("(Events based Behavior) events function renamed") {
@@ -703,7 +740,7 @@ TEST_CASE("WholeProjectRefactorer", "[common]") {
         "MyBehaviorEventsFunctionExpression",
         "MyRenamedBehaviorEventsFunctionExpression");
 
-    // Check if events based behaviors functions have been renamed in
+    // Check if events-based behavior methods have been renamed in
     // instructions
     REQUIRE(static_cast<gd::StandardEvent &>(
                 project.GetLayout("LayoutWithBehaviorFunctions")
@@ -715,7 +752,7 @@ TEST_CASE("WholeProjectRefactorer", "[common]") {
             "MyEventsExtension::MyEventsBasedBehavior::"
             "MyRenamedBehaviorEventsFunction");
 
-    // Check events based behaviors functions have been renamed in
+    // Check events-based behavior methods have been renamed in
     // expressions
     REQUIRE(static_cast<gd::StandardEvent &>(
                 project.GetExternalEvents("ExternalEventsWithBehaviorFunctions")
@@ -726,8 +763,22 @@ TEST_CASE("WholeProjectRefactorer", "[common]") {
                 .GetParameter(0)
                 .GetPlainString() ==
             "1 + "
-            "ObjectWithMyBehavior::MyBehavior."
+            "ObjectWithMyBehavior.MyBehavior::"
             "MyRenamedBehaviorEventsFunctionExpression(123, 456, 789)");
+
+    // Check that a ill-named function that looks a bit like a behavior method
+    // (but which is actually a free function) is *not* renamed.
+    REQUIRE(static_cast<gd::StandardEvent &>(
+                project.GetExternalEvents("ExternalEventsWithBehaviorFunctions")
+                    .GetEvents()
+                    .GetEvent(1))
+                .GetActions()
+                .Get(0)
+                .GetParameter(0)
+                .GetPlainString() ==
+            "2 + "
+            "ObjectWithMyBehavior::MyBehavior."
+            "MyBehaviorEventsFunctionExpression(123, 456, 789)");
   }
   SECTION("(Events based Behavior) events function parameter moved") {
     gd::Project project;
@@ -742,27 +793,29 @@ TEST_CASE("WholeProjectRefactorer", "[common]") {
         eventsExtension,
         eventsBasedBehavior,
         "MyBehaviorEventsFunction",
-        0, 2);
+        0,
+        2);
     gd::WholeProjectRefactorer::MoveBehaviorEventsFunctionParameter(
         project,
         eventsExtension,
         eventsBasedBehavior,
         "MyBehaviorEventsFunctionExpression",
-        0, 2);
+        0,
+        2);
 
-    // Check if events based behaviors functions have been renamed in
+    // Check if parameters of events-based behavior methods have been moved in
     // instructions
-    auto& action = static_cast<gd::StandardEvent &>(
+    auto &action = static_cast<gd::StandardEvent &>(
-                project.GetLayout("LayoutWithBehaviorFunctions")
+                       project.GetLayout("LayoutWithBehaviorFunctions")
-                    .GetEvents()
+                           .GetEvents()
-                    .GetEvent(0))
+                           .GetEvent(0))
-                .GetActions()
+                       .GetActions()
-                .Get(0);
+                       .Get(0);
     REQUIRE(action.GetParameter(0).GetPlainString() == "Second parameter");
     REQUIRE(action.GetParameter(1).GetPlainString() == "Third parameter");
     REQUIRE(action.GetParameter(2).GetPlainString() == "First parameter");
 
-    // Check events based behaviors functions have been renamed in
+    // Check parameters of events-based behavior methods have been moved in
     // expressions
     REQUIRE(static_cast<gd::StandardEvent &>(
                 project.GetExternalEvents("ExternalEventsWithBehaviorFunctions")
@@ -773,8 +826,22 @@ TEST_CASE("WholeProjectRefactorer", "[common]") {
                 .GetParameter(0)
                 .GetPlainString() ==
             "1 + "
-            "ObjectWithMyBehavior::MyBehavior."
+            "ObjectWithMyBehavior.MyBehavior::"
             "MyBehaviorEventsFunctionExpression(456, 789, 123)");
+
+    // Check that a ill-named function that looks a bit like a behavior method
+    // (but which is actually a free function) has its parameter *not* moved.
+    REQUIRE(static_cast<gd::StandardEvent &>(
+                project.GetExternalEvents("ExternalEventsWithBehaviorFunctions")
+                    .GetEvents()
+                    .GetEvent(1))
+                .GetActions()
+                .Get(0)
+                .GetParameter(0)
+                .GetPlainString() ==
+            "2 + "
+            "ObjectWithMyBehavior::MyBehavior."
+            "MyBehaviorEventsFunctionExpression(123, 456, 789)");
   }
   SECTION("(Events based Behavior) property renamed") {
     gd::Project project;
@@ -790,7 +857,7 @@ TEST_CASE("WholeProjectRefactorer", "[common]") {
                                                        "MyProperty",
                                                        "MyRenamedProperty");
 
-    // Check if events based behaviors property has been renamed in
+    // Check if events-based behaviors property has been renamed in
     // instructions
     REQUIRE(static_cast<gd::StandardEvent &>(
                 project.GetLayout("LayoutWithBehaviorFunctions")
@@ -801,5 +868,25 @@ TEST_CASE("WholeProjectRefactorer", "[common]") {
                 .GetType() ==
             "MyEventsExtension::MyEventsBasedBehavior::"
             "SetPropertyMyRenamedProperty");
+
+    REQUIRE(static_cast<gd::StandardEvent &>(
+                project.GetLayout("LayoutWithBehaviorFunctions")
+                    .GetEvents()
+                    .GetEvent(2))
+                .GetConditions()
+                .Get(0)
+                .GetType() ==
+            "MyEventsExtension::MyEventsBasedBehavior::"
+            "PropertyMyRenamedProperty");
+
+    REQUIRE(static_cast<gd::StandardEvent &>(
+                project.GetLayout("LayoutWithBehaviorFunctions")
+                    .GetEvents()
+                    .GetEvent(3))
+                .GetActions()
+                .Get(0)
+                .GetParameter(0)
+                .GetPlainString() ==
+            "ObjectWithMyBehavior.MyBehavior::PropertyMyRenamedProperty()");
   }
 }