Skip to content

Commit

Permalink
Fix variables from being renamed with a property.
Browse files Browse the repository at this point in the history
  • Loading branch information
@D8H
D8H committed Nov 24, 2024
1 parent 0d811c4 commit ec408b7
Show file tree
Hide file tree
Showing 5 changed files with 110 additions and 61 deletions.
15 changes: 14 additions & 1 deletion Core/GDCore/Extensions/Metadata/ValueTypeMetadata.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ class GD_CORE_API ValueTypeMetadata {
}

/**
* \brief Return true if the type of the parameter is a number.
* \brief Return true if the type of the parameter is a variable.
* \note If you had a new type of parameter, also add it in the IDE (
* see EventsFunctionParametersEditor, ParameterRenderingService
* and ExpressionAutocompletion) and in the EventsCodeGenerator.
Expand All @@ -138,6 +138,19 @@ class GD_CORE_API ValueTypeMetadata {
return gd::ValueTypeMetadata::GetPrimitiveValueType(name) == "variable";
}

/**
* \brief Return true if the type of the parameter is a variable and not a
* property or a parameter.
*/
bool IsVariableOnly() const {
return
// Any variable.
name == "variable" ||
// Old, "pre-scoped" variables:
name == "objectvar" || name == "globalvar" ||
name == "scenevar";
}

/**
* \brief Return true if the type is a variable but from a specific scope
* (scene, project or object). In new code, prefer to use the more generic "variable"
Expand Down
121 changes: 63 additions & 58 deletions Core/GDCore/IDE/Events/EventsPropertyReplacer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,15 @@ 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),
removedPropertyUsed(false),
platform(platform_),
projectScopedContainers(projectScopedContainers_),
targetPropertiesContainer(targetPropertiesContainer_),
isParentTypeAVariable(isParentTypeAVariable_),
oldToNewPropertyNames(oldToNewPropertyNames_),
removedPropertyNames(removedPropertyNames_){};
virtual ~ExpressionPropertyReplacer(){};
Expand All @@ -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);
Expand All @@ -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);
});
}
Expand All @@ -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.
}, [&]() {
Expand All @@ -145,35 +150,36 @@ 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.IsVariableOnly();
parameter->Visit(*this);
}
}
isParentTypeAVariable = isGrandParentTypeAVariable;
}
void OnVisitEmptyNode(EmptyNode& node) override {}

private:
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) {
Expand All @@ -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,
Expand All @@ -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().IsVariableOnly(),
oldToNewPropertyNames, removedPropertyNames);
node->Visit(renamer);

if (renamer.IsRemovedPropertyUsed()) {
Expand All @@ -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().IsVariableOnly(), oldToNewPropertyNames,
removedPropertyNames);
node->Visit(renamer);

if (renamer.IsRemovedPropertyUsed()) {
Expand All @@ -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
2 changes: 2 additions & 0 deletions Core/GDCore/IDE/Events/EventsPropertyReplacer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
4 changes: 4 additions & 0 deletions Core/GDCore/Project/VariablesContainersList.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,10 @@ VariablesContainersList VariablesContainersList::
variablesContainersList.Push(extension.GetGlobalVariables());
variablesContainersList.Push(extension.GetSceneVariables());

gd::EventsFunctionTools::PropertiesToVariablesContainer(
eventsBasedBehavior.GetSharedPropertyDescriptors(), propertyVariablesContainer);
variablesContainersList.Push(propertyVariablesContainer);

gd::EventsFunctionTools::PropertiesToVariablesContainer(
eventsBasedBehavior.GetPropertyDescriptors(), propertyVariablesContainer);
variablesContainersList.Push(propertyVariablesContainer);
Expand Down
29 changes: 27 additions & 2 deletions Core/tests/WholeProjectRefactorer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3153,13 +3153,18 @@ TEST_CASE("WholeProjectRefactorer", "[common]") {
eventsExtension, eventsBasedBehavior);
auto &instruction = CreateInstructionWithNumberParameter(
project, behaviorAction.GetEvents(), "MyProperty");
auto &instruction2 = CreateInstructionWithNumberParameter(
project, behaviorAction.GetEvents(),
"MyExtension::GetVariableAsNumber(MyVariable[MyProperty])");

gd::WholeProjectRefactorer::RenameEventsBasedBehaviorProperty(
project, eventsExtension, eventsBasedBehavior, "MyProperty",
"MyRenamedProperty");

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

SECTION("(Events based behavior) property renamed (in variable setter)") {
Expand Down Expand Up @@ -3226,6 +3231,9 @@ TEST_CASE("WholeProjectRefactorer", "[common]") {
// 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",
Expand All @@ -3234,6 +3242,8 @@ TEST_CASE("WholeProjectRefactorer", "[common]") {
// "variable" parameters are left untouched.
REQUIRE(instruction.GetParameter(0).GetPlainString() ==
"MyProperty");
REQUIRE(instruction2.GetParameter(0).GetPlainString() ==
"MyExtension::GetVariableAsNumber(MyProperty)");
}

SECTION("(Events based behavior) property type changed (in variable setter)") {
Expand Down Expand Up @@ -3348,13 +3358,18 @@ TEST_CASE("WholeProjectRefactorer", "[common]") {
eventsExtension, eventsBasedBehavior);
auto &instruction = CreateInstructionWithNumberParameter(
project, behaviorAction.GetEvents(), "MySharedProperty");
auto &instruction2 = CreateInstructionWithNumberParameter(
project, behaviorAction.GetEvents(),
"MyExtension::GetVariableAsNumber(MyVariable[MySharedProperty])");

gd::WholeProjectRefactorer::RenameEventsBasedBehaviorSharedProperty(
project, eventsExtension, eventsBasedBehavior, "MySharedProperty",
"MyRenamedSharedProperty");

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

SECTION("(Events based object) property renamed") {
Expand Down Expand Up @@ -3402,13 +3417,18 @@ TEST_CASE("WholeProjectRefactorer", "[common]") {
eventsExtension, eventsBasedObject);
auto &instruction = CreateInstructionWithNumberParameter(
project, behaviorAction.GetEvents(), "MyProperty");
auto &instruction2 = CreateInstructionWithNumberParameter(
project, behaviorAction.GetEvents(),
"MyExtension::GetVariableAsNumber(MyVariable[MyProperty])");

gd::WholeProjectRefactorer::RenameEventsBasedObjectProperty(
project, eventsExtension, eventsBasedObject, "MyProperty",
"MyRenamedProperty");

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

SECTION("(Events based object) property renamed (in variable setter)") {
Expand Down Expand Up @@ -3475,6 +3495,9 @@ TEST_CASE("WholeProjectRefactorer", "[common]") {
// 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",
Expand All @@ -3483,9 +3506,11 @@ TEST_CASE("WholeProjectRefactorer", "[common]") {
// "variable" parameters are left untouched.
REQUIRE(instruction.GetParameter(0).GetPlainString() ==
"MyProperty");
REQUIRE(instruction2.GetParameter(0).GetPlainString() ==
"MyExtension::GetVariableAsNumber(MyProperty)");
}

SECTION("(Events based object) property renamed (in variable setter)") {
SECTION("(Events based object) property type changed (in variable setter)") {
gd::Project project;
gd::Platform platform;
SetupProjectWithDummyPlatform(project, platform);
Expand All @@ -3508,7 +3533,7 @@ TEST_CASE("WholeProjectRefactorer", "[common]") {
REQUIRE(instruction.GetType() == "SetNumberVariable");
}

SECTION("(Events based object) property renamed (in variable getter)") {
SECTION("(Events based object) property type changed (in variable getter)") {
gd::Project project;
gd::Platform platform;
SetupProjectWithDummyPlatform(project, platform);
Expand Down

0 comments on commit ec408b7

Please sign in to comment.