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.

Sign up for GitHub

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

Fix variables from being renamed with a property #7186

Merged
merged 2 commits into from
Nov 26, 2024
Merged

Fix variables from being renamed with a property #7186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a4875dc
Fix variables from being renamed with a property
D8H Nov 24, 2024
4e2ab83
Make the test a bit stronger.
D8H Nov 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 63 additions & 58 deletions Core/GDCore/IDE/Events/EventsPropertyReplacer.cpp
View file
Open in desktop
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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it useful to explore the node's children if this one is __ (I'm not sure what it means)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, because of expression like: MyVariable.MyChild[MyProperty].

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other words, we must continue to explore because properties could be used in children (becuase a variable node can have sub variables, or bracket accessors, which allows to enter a sub-expression inside the brackets ([MyProperty]. [1 + MyProperty]...), and this sub-expression must be explored to rename the property that could be there.

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>(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why this "MatchIdentifierWithName" is not doing its job? If it's a variable, it should match it (and so do nothing), no?

Copy link
Collaborator Author

@D8H D8H Nov 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think isParentTypeAVariable is still required because when a node is an identifier, we don't want to replace it if the expression is a variable parameter even if the variable doesn't exist.
MatchIdentifierWithName doesn't know about the expected type for the current node. This is why isParentTypeAVariable is needed.

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.IsVariable();
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().IsVariable(),
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().IsVariable(), 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
Open in desktop
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
29 changes: 20 additions & 9 deletions Core/tests/DummyPlatform.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_",
"",
"",
"")
Expand All @@ -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_",
"",
"",
"")
Expand All @@ -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_",
"",
"",
"")
Expand Down Expand Up @@ -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",
Expand Down
Loading
Loading