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.

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 conflict between variable or property and parameter in variable setters #7329

Merged
merged 3 commits into from
Jan 21, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 11 additions & 2 deletions Core/GDCore/Events/CodeGeneration/EventsCodeGenerator.h
Original file line number Diff line number Diff line change
Expand Up @@ -467,7 +467,14 @@ class GD_CORE_API EventsCodeGenerator {
*/
virtual gd::String GetCodeNamespace() { return ""; };

enum VariableScope { LAYOUT_VARIABLE = 0, PROJECT_VARIABLE, OBJECT_VARIABLE, ANY_VARIABLE };
enum VariableScope {
LAYOUT_VARIABLE = 0,
PROJECT_VARIABLE,
OBJECT_VARIABLE,
ANY_VARIABLE,
VARIABLE_OR_PROPERTY,
VARIABLE_OR_PROPERTY_OR_PARAMETER
};

/**
* Generate a single unique number for the specified instruction.
Expand Down Expand Up @@ -579,7 +586,9 @@ class GD_CORE_API EventsCodeGenerator {

} else if (scope == PROJECT_VARIABLE) {
return "getProjectVariable(" + variableName + ")";
} else if (scope == ANY_VARIABLE) {
} else if (scope == ANY_VARIABLE || scope == VARIABLE_OR_PROPERTY ||
scope == VARIABLE_OR_PROPERTY_OR_PARAMETER) {
// TODO Split the 3 cases to make tests stronger.
return "getAnyVariable(" + variableName + ")";
}

Expand Down
16 changes: 11 additions & 5 deletions Core/GDCore/Events/CodeGeneration/ExpressionCodeGenerator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -133,9 +133,12 @@ void ExpressionCodeGenerator::OnVisitVariableNode(VariableNode& node) {
if (gd::ParameterMetadata::IsExpression("variable", type)) {
// The node is a variable inside an expression waiting for a *variable* to be returned, not its value.
EventsCodeGenerator::VariableScope scope =
type == "variable" || type == "variableOrProperty" ||
type == "variableOrPropertyOrParameter"
type == "variable"
? gd::EventsCodeGenerator::ANY_VARIABLE
: type == "variableOrProperty"
? gd::EventsCodeGenerator::VARIABLE_OR_PROPERTY
: type == "variableOrPropertyOrParameter"
? gd::EventsCodeGenerator::VARIABLE_OR_PROPERTY_OR_PARAMETER
: type == "globalvar" ? gd::EventsCodeGenerator::PROJECT_VARIABLE
: type == "scenevar" ? gd::EventsCodeGenerator::LAYOUT_VARIABLE
: gd::EventsCodeGenerator::OBJECT_VARIABLE;
Expand Down Expand Up @@ -223,9 +226,12 @@ void ExpressionCodeGenerator::OnVisitIdentifierNode(IdentifierNode& node) {
codeGenerator.GenerateObject(node.identifierName, type, context);
} else if (gd::ParameterMetadata::IsExpression("variable", type)) {
EventsCodeGenerator::VariableScope scope =
type == "variable" || type == "variableOrProperty" ||
type == "variableOrPropertyOrParameter"
type == "variable"
? gd::EventsCodeGenerator::ANY_VARIABLE
: type == "variableOrProperty"
? gd::EventsCodeGenerator::VARIABLE_OR_PROPERTY
: type == "variableOrPropertyOrParameter"
? gd::EventsCodeGenerator::VARIABLE_OR_PROPERTY_OR_PARAMETER
: type == "globalvar" ? gd::EventsCodeGenerator::PROJECT_VARIABLE
: type == "scenevar" ? gd::EventsCodeGenerator::LAYOUT_VARIABLE
: gd::EventsCodeGenerator::OBJECT_VARIABLE;
Expand Down Expand Up @@ -254,7 +260,7 @@ void ExpressionCodeGenerator::OnVisitIdentifierNode(IdentifierNode& node) {
output += codeGenerator.GenerateVariableValueAs(type);
}, [&]() {
output += codeGenerator.GenerateGetVariable(
node.identifierName, gd::EventsCodeGenerator::ANY_VARIABLE, context,
node.identifierName, gd::EventsCodeGenerator::VARIABLE_OR_PROPERTY_OR_PARAMETER, context,
"", !node.childIdentifierName.empty());
if (!node.childIdentifierName.empty()) {
output += codeGenerator.GenerateVariableAccessor(node.childIdentifierName);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,13 +71,18 @@ bool EventsVariableInstructionTypeSwitcher::DoVisitInstruction(gd::Instruction&
.GetObjectsContainersList()
.GetObjectOrGroupVariablesContainer(lastObjectName);
}
} else if (type == "variableOrProperty") {
variablesContainer =
&GetProjectScopedContainers()
.GetVariablesContainersList()
.GetVariablesContainerFromVariableOrPropertyName(variableName);
} else {
if (GetProjectScopedContainers().GetVariablesContainersList().Has(
variableName)) {
variablesContainer =
&GetProjectScopedContainers()
.GetVariablesContainersList()
.GetVariablesContainerFromVariableName(variableName);
.GetVariablesContainerFromVariableOrPropertyOrParameterName(variableName);
}
}

Expand Down
4 changes: 2 additions & 2 deletions Core/GDCore/IDE/Events/EventsVariableReplacer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ class GD_CORE_API ExpressionVariableReplacer
[&]() {
// This is a variable.
if (&projectScopedContainers.GetVariablesContainersList()
.GetVariablesContainerFromVariableName(node.name) ==
.GetVariablesContainerFromVariableOrPropertyOrParameterName(node.name) ==
&targetVariablesContainer) {
// The node represents a variable, that can come from the target
// (because the target is in the scope), replace or remove it:
Expand Down Expand Up @@ -235,7 +235,7 @@ class GD_CORE_API ExpressionVariableReplacer
[&]() {
// This is a variable.
if (&projectScopedContainers.GetVariablesContainersList()
.GetVariablesContainerFromVariableName(
.GetVariablesContainerFromVariableOrPropertyOrParameterName(
node.identifierName) == &targetVariablesContainer) {
// The node represents a variable, that can come from the target
// (because the target is in the scope), replace or remove it:
Expand Down
4 changes: 2 additions & 2 deletions Core/GDCore/IDE/Events/ExpressionCompletionFinder.h
Original file line number Diff line number Diff line change
Expand Up @@ -1034,7 +1034,7 @@ class GD_CORE_API ExpressionCompletionFinder
description.SetVariableType(variable.GetType());
description.SetVariableScope(
projectScopedContainers.GetVariablesContainersList()
.GetVariablesContainerFromVariableName(variableName)
.GetVariablesContainerFromVariableOrPropertyOrParameterName(variableName)
.GetSourceType());
completions.push_back(description);

Expand Down Expand Up @@ -1086,7 +1086,7 @@ class GD_CORE_API ExpressionCompletionFinder
description.SetVariableType(variable.GetType());
description.SetVariableScope(
projectScopedContainers.GetVariablesContainersList()
.GetVariablesContainerFromVariableName(variableName)
.GetVariablesContainerFromVariableOrPropertyOrParameterName(variableName)
.GetSourceType());
completions.push_back(description);

Expand Down
6 changes: 3 additions & 3 deletions Core/GDCore/IDE/Events/ExpressionVariablePathFinder.h
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ class GD_CORE_API ExpressionVariablePathFinder
if (projectScopedContainers.GetVariablesContainersList().Has(identifier)) {
variablesContainer =
&(projectScopedContainers.GetVariablesContainersList()
.GetVariablesContainerFromVariableName(identifier));
.GetVariablesContainerFromVariableOrPropertyOrParameterName(identifier));
variableName = identifier;
if (childIdentifier) {
childVariableNames.push_back(*childIdentifier);
Expand All @@ -229,7 +229,7 @@ class GD_CORE_API ExpressionVariablePathFinder
identifier)) {
variablesContainer =
&(projectScopedContainers.GetVariablesContainersList()
.GetVariablesContainerFromVariableName(identifier));
.GetVariablesContainerFromVariableOrPropertyOrParameterName(identifier));
variableName = identifier;
// There is no support for "children" of properties.
}
Expand All @@ -241,7 +241,7 @@ class GD_CORE_API ExpressionVariablePathFinder
identifier)) {
variablesContainer =
&(projectScopedContainers.GetVariablesContainersList()
.GetVariablesContainerFromVariableName(identifier));
.GetVariablesContainerFromVariableOrPropertyOrParameterName(identifier));
variableName = identifier;
// There is no support for "children" of parameters.
}
Expand Down
2 changes: 1 addition & 1 deletion Core/GDCore/Project/ProjectScopedContainers.h
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ class ProjectScopedContainers {
return objectCallback();
else if (variablesContainersList.Has(name)) {
const auto &variablesContainer =
variablesContainersList.GetVariablesContainerFromVariableName(name);
variablesContainersList.GetVariablesContainerFromVariableOrPropertyOrParameterName(name);
const auto sourceType = variablesContainer.GetSourceType();
if (sourceType == gd::VariablesContainer::SourceType::Properties) {
return propertyCallback();
Expand Down
30 changes: 29 additions & 1 deletion Core/GDCore/Project/VariablesContainersList.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ const Variable& VariablesContainersList::Get(const gd::String& name) const {
}

const VariablesContainer &
VariablesContainersList::GetVariablesContainerFromVariableName(
VariablesContainersList::GetVariablesContainerFromVariableOrPropertyOrParameterName(
const gd::String &variableName) const {
for (auto it = variablesContainers.rbegin(); it != variablesContainers.rend();
++it) {
Expand All @@ -157,6 +157,34 @@ VariablesContainersList::GetVariablesContainerFromVariableName(
return badVariablesContainer;
}

const VariablesContainer &VariablesContainersList::
GetVariablesContainerFromVariableOrPropertyName(
const gd::String &variableName) const {
for (auto it = variablesContainers.rbegin(); it != variablesContainers.rend();
++it) {
if ((*it)->GetSourceType() !=
gd::VariablesContainer::SourceType::Parameters &&
(*it)->Has(variableName))
return **it;
}
return badVariablesContainer;
}

const VariablesContainer &VariablesContainersList::
GetVariablesContainerFromVariableNameOnly(
const gd::String &variableName) const {
for (auto it = variablesContainers.rbegin(); it != variablesContainers.rend();
++it) {
if ((*it)->GetSourceType() !=
gd::VariablesContainer::SourceType::Parameters &&
(*it)->GetSourceType() !=
gd::VariablesContainer::SourceType::Properties &&
(*it)->Has(variableName))
return **it;
}
return badVariablesContainer;
}

std::size_t
VariablesContainersList::GetVariablesContainerPositionFromVariableName(
const gd::String &variableName) const {
Expand Down
14 changes: 13 additions & 1 deletion Core/GDCore/Project/VariablesContainersList.h
Original file line number Diff line number Diff line change
Expand Up @@ -114,11 +114,23 @@ class GD_CORE_API VariablesContainersList {
return variablesContainers.at(firstLocalVariableContainerIndex - 1);
}

/**
* Get the variables container for a given variable or property or parameter.
*/
const VariablesContainer &
GetVariablesContainerFromVariableOrPropertyOrParameterName(const gd::String &variableName) const;

/**
* Get the variables container for a given variable or property.
*/
const VariablesContainer &
GetVariablesContainerFromVariableOrPropertyName(const gd::String &variableName) const;

/**
* Get the variables container for a given variable.
*/
const VariablesContainer &
GetVariablesContainerFromVariableName(const gd::String &variableName) const;
GetVariablesContainerFromVariableNameOnly(const gd::String &variableName) const;

/**
* Get the variables container index for a given variable.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1540,7 +1540,7 @@ TEST_CASE("WholeProjectRefactorer::ApplyRefactoringForVariablesContainer",
auto projectScopedContainers =
gd::ProjectScopedContainers::MakeNewProjectScopedContainersForProjectAndLayout(project, scene);
REQUIRE(&projectScopedContainers.GetVariablesContainersList()
.GetVariablesContainerFromVariableName("MyVariable") == &scene.GetVariables());
.GetVariablesContainerFromVariableOrPropertyOrParameterName("MyVariable") == &scene.GetVariables());

// Do the changes and launch the refactoring.
scene.GetVariables().ResetPersistentUuid();
Expand Down
17 changes: 13 additions & 4 deletions GDJS/GDJS/Events/CodeGeneration/EventsCodeGenerator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1363,12 +1363,21 @@ gd::String EventsCodeGenerator::GenerateGetVariable(
bool hasChild) {
gd::String output;
const gd::VariablesContainer* variables = NULL;
if (scope == ANY_VARIABLE) {
if (scope == ANY_VARIABLE || scope == VARIABLE_OR_PROPERTY ||
scope == VARIABLE_OR_PROPERTY_OR_PARAMETER) {
const auto variablesContainersList =
GetProjectScopedContainers().GetVariablesContainersList();
const auto& variablesContainer =
variablesContainersList.GetVariablesContainerFromVariableName(
variableName);
const auto &variablesContainer =
scope == VARIABLE_OR_PROPERTY_OR_PARAMETER
? variablesContainersList.GetVariablesContainerFromVariableOrPropertyOrParameterName(
variableName)
: scope == VARIABLE_OR_PROPERTY
? variablesContainersList
.GetVariablesContainerFromVariableOrPropertyName(
variableName)
: variablesContainersList
.GetVariablesContainerFromVariableNameOnly(
variableName);
const auto sourceType = variablesContainer.GetSourceType();
if (sourceType == gd::VariablesContainer::SourceType::Scene) {
variables = &variablesContainer;
Expand Down
12 changes: 6 additions & 6 deletions GDJS/GDJS/Extensions/Builtin/VariablesExtension.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ VariablesExtension::VariablesExtension() {
const auto variablesContainersList =
codeGenerator.GetProjectScopedContainers().GetVariablesContainersList();
const auto& variablesContainer =
variablesContainersList.GetVariablesContainerFromVariableName(
variablesContainersList.GetVariablesContainerFromVariableOrPropertyOrParameterName(
variableName);
const auto sourceType = variablesContainer.GetSourceType();
if (sourceType != gd::VariablesContainer::SourceType::Properties &&
Expand Down Expand Up @@ -75,7 +75,7 @@ VariablesExtension::VariablesExtension() {
const auto variablesContainersList =
codeGenerator.GetProjectScopedContainers().GetVariablesContainersList();
const auto& variablesContainer =
variablesContainersList.GetVariablesContainerFromVariableName(
variablesContainersList.GetVariablesContainerFromVariableOrPropertyOrParameterName(
variableName);
const auto sourceType = variablesContainer.GetSourceType();
if (sourceType != gd::VariablesContainer::SourceType::Properties &&
Expand Down Expand Up @@ -107,7 +107,7 @@ VariablesExtension::VariablesExtension() {
const auto variablesContainersList =
codeGenerator.GetProjectScopedContainers().GetVariablesContainersList();
const auto& variablesContainer =
variablesContainersList.GetVariablesContainerFromVariableName(
variablesContainersList.GetVariablesContainerFromVariableOrPropertyOrParameterName(
variableName);
const auto sourceType = variablesContainer.GetSourceType();
if (sourceType != gd::VariablesContainer::SourceType::Properties &&
Expand Down Expand Up @@ -181,7 +181,7 @@ VariablesExtension::VariablesExtension() {
const auto variablesContainersList =
codeGenerator.GetProjectScopedContainers().GetVariablesContainersList();
const auto& variablesContainer =
variablesContainersList.GetVariablesContainerFromVariableName(
variablesContainersList.GetVariablesContainerFromVariableOrPropertyName(
variableName);
const auto sourceType = variablesContainer.GetSourceType();
if (sourceType == gd::VariablesContainer::SourceType::Properties) {
Expand Down Expand Up @@ -244,7 +244,7 @@ VariablesExtension::VariablesExtension() {
const auto variablesContainersList =
codeGenerator.GetProjectScopedContainers().GetVariablesContainersList();
const auto& variablesContainer =
variablesContainersList.GetVariablesContainerFromVariableName(
variablesContainersList.GetVariablesContainerFromVariableOrPropertyName(
variableName);
const auto sourceType = variablesContainer.GetSourceType();
if (sourceType == gd::VariablesContainer::SourceType::Properties) {
Expand Down Expand Up @@ -304,7 +304,7 @@ VariablesExtension::VariablesExtension() {
const auto variablesContainersList =
codeGenerator.GetProjectScopedContainers().GetVariablesContainersList();
const auto& variablesContainer =
variablesContainersList.GetVariablesContainerFromVariableName(
variablesContainersList.GetVariablesContainerFromVariableOrPropertyName(
variableName);
const auto sourceType = variablesContainer.GetSourceType();
if (sourceType == gd::VariablesContainer::SourceType::Properties) {
Expand Down
4 changes: 3 additions & 1 deletion GDevelop.js/Bindings/Bindings.idl
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,9 @@ interface VariablesContainer {
interface VariablesContainersList {
boolean Has([Const] DOMString name);
[Const, Ref] Variable Get([Const] DOMString name);
[Const, Ref] VariablesContainer GetVariablesContainerFromVariableName([Const] DOMString variableName);
[Const, Ref] VariablesContainer GetVariablesContainerFromVariableOrPropertyOrParameterName([Const] DOMString variableName);
[Const, Ref] VariablesContainer GetVariablesContainerFromVariableOrPropertyName([Const] DOMString variableName);
[Const, Ref] VariablesContainer GetVariablesContainerFromVariableNameOnly([Const] DOMString variableName);

[Const, Ref] VariablesContainer GetVariablesContainer(unsigned long index);
unsigned long GetVariablesContainersCount();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,71 @@ describe('libGD.js - GDJS Behavior Code Generation integration tests', function
expect(behavior._getMyProperty()).toBe(456);
});

it('Can use a property in a variable action when a parameter with the same name exits', () => {
const project = new gd.ProjectHelper.createNewGDJSProject();
const eventsFunctionsExtension = project.insertNewEventsFunctionsExtension(
'MyExtension',
0
);
const eventsBasedBehavior = eventsFunctionsExtension
.getEventsBasedBehaviors()
.insertNew('MyBehavior', 0);

eventsBasedBehavior
.getPropertyDescriptors()
.insertNew('MyIdentifier', 0)
.setValue('123')
.setType('Number');

const eventsSerializerElement = gd.Serializer.fromJSObject([
{
type: 'BuiltinCommonInstructions::Standard',
conditions: [],
actions: [
{
type: { value: 'SetNumberVariable' },
parameters: ['MyIdentifier', '=', '456'],
},
],
},
]);
eventsBasedBehavior
.getEventsFunctions()
.insertNewEventsFunction('MyFunction', 0)
.getEvents()
.unserializeFrom(project, eventsSerializerElement);
gd.WholeProjectRefactorer.ensureBehaviorEventsFunctionsProperParameters(
eventsFunctionsExtension,
eventsBasedBehavior
);
// Add a parameter with the same name as the property.
// It won't be used as SetNumberVariable has a variableOrProperty parameter.
const eventsFunction = eventsBasedBehavior
.getEventsFunctions()
.insertNewEventsFunction('MyFunction', 0);
const parameter = eventsFunction
.getParameters()
.insertNewParameter(
'MyIdentifier',
eventsFunction.getParameters().getParametersCount()
);
parameter.setType('number');

const { runtimeScene, behavior } = generatedBehavior(
gd,
project,
eventsFunctionsExtension,
eventsBasedBehavior,
{ logCode: false }
);

// Check the default value is set.
expect(behavior._getMyIdentifier()).toBe(123);

behavior.MyFunction(222);
expect(behavior._getMyIdentifier()).toBe(456);
});

it('Can use a property in a variable condition', () => {
const project = new gd.ProjectHelper.createNewGDJSProject();
const scene = project.insertNewLayout('MyScene', 0);
Expand Down
Loading
Loading