Skip to content

Fix validating object variables too many times on export #9083

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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ClementPasteau wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix validating object variables too many times on export #9083

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
32 changes: 30 additions & 2 deletions Core/GDCore/Events/CodeGeneration/EventsCodeGenerator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,6 +386,7 @@ gd::String EventsCodeGenerator::GenerateConditionCode(
GetObjectsContainersList().ExpandObjectName(
objectName, context.GetCurrentObject());
for (std::size_t i = 0; i < realObjects.size(); ++i) {
expressionValidationFailed = false;
// Set up the context
gd::String objectType =
GetObjectsContainersList().GetTypeOfObject(realObjects[i]);
Expand All @@ -405,8 +406,13 @@ gd::String EventsCodeGenerator::GenerateConditionCode(
realObjects[i], objInfo, arguments, instrInfos, returnBoolean,
condition.IsInverted(), context);
}
// Expressions are validated against the group name, not the current
// object, so the result is the same for every object: validate them
// only once.
expressionValidationSkipped = !expressionValidationFailed;
context.SetNoCurrentObject();
}
expressionValidationSkipped = false;
}
} else if (instrInfos.IsBehaviorInstruction()) {
if (instrInfos.parameters.GetParametersCount() >= 2) {
Expand All @@ -424,6 +430,7 @@ gd::String EventsCodeGenerator::GenerateConditionCode(
MetadataProvider::GetBehaviorMetadata(platform, actualBehaviorType);

for (std::size_t i = 0; i < realObjects.size(); ++i) {
expressionValidationFailed = false;
// Setup context
AddIncludeFiles(autoInfo.includeFiles);
context.SetCurrentObject(realObjects[i]);
Expand All @@ -438,8 +445,13 @@ gd::String EventsCodeGenerator::GenerateConditionCode(
realObjects[i], behaviorName, autoInfo, arguments, instrInfos,
returnBoolean, condition.IsInverted(), context);
}
// Expressions are validated against the group name, not the current
// object, so the result is the same for every object: validate them
// only once.
expressionValidationSkipped = !expressionValidationFailed;
context.SetNoCurrentObject();
}
expressionValidationSkipped = false;
}
} else {
std::vector<std::pair<gd::String, gd::String> >
Expand Down Expand Up @@ -675,6 +687,7 @@ gd::String EventsCodeGenerator::GenerateActionCode(
GetObjectsContainersList().ExpandObjectName(
objectName, context.GetCurrentObject());
for (std::size_t i = 0; i < realObjects.size(); ++i) {
expressionValidationFailed = false;
// Setup context
gd::String objectType =
GetObjectsContainersList().GetTypeOfObject(realObjects[i]);
Expand All @@ -694,8 +707,13 @@ gd::String EventsCodeGenerator::GenerateActionCode(
realObjects[i], objInfo, functionCallName, arguments, instrInfos,
context, optionalAsyncCallbackName, optionalAsyncCallbackId);
}
// Expressions are validated against the group name, not the current
// object, so the result is the same for every object: validate them
// only once.
expressionValidationSkipped = !expressionValidationFailed;
context.SetNoCurrentObject();
}
expressionValidationSkipped = false;
}
} else if (instrInfos.IsBehaviorInstruction()) {
if (instrInfos.parameters.GetParametersCount() >= 2) {
Expand All @@ -713,6 +731,7 @@ gd::String EventsCodeGenerator::GenerateActionCode(

AddIncludeFiles(autoInfo.includeFiles);
for (std::size_t i = 0; i < realObjects.size(); ++i) {
expressionValidationFailed = false;
// Setup context
context.SetCurrentObject(realObjects[i]);
context.ObjectsListNeeded(realObjects[i]);
Expand All @@ -727,8 +746,13 @@ gd::String EventsCodeGenerator::GenerateActionCode(
arguments, instrInfos, context, optionalAsyncCallbackName,
optionalAsyncCallbackId);
}
// Expressions are validated against the group name, not the current
// object, so the result is the same for every object: validate them
// only once.
expressionValidationSkipped = !expressionValidationFailed;
context.SetNoCurrentObject();
}
expressionValidationSkipped = false;
}
} else {
vector<gd::String> arguments = GenerateParametersCodes(
Expand Down Expand Up @@ -1571,7 +1595,9 @@ EventsCodeGenerator::EventsCodeGenerator(const gd::Project& project_,
maxCustomConditionsDepth(0),
maxConditionsListsSize(0),
eventsListNextUniqueId(0),
diagnosticReport(nullptr) {};
diagnosticReport(nullptr),
expressionValidationSkipped(false),
expressionValidationFailed(false) {};

EventsCodeGenerator::EventsCodeGenerator(
const gd::Platform& platform_,
Expand All @@ -1586,6 +1612,8 @@ EventsCodeGenerator::EventsCodeGenerator(
maxCustomConditionsDepth(0),
maxConditionsListsSize(0),
eventsListNextUniqueId(0),
diagnosticReport(nullptr) {};
diagnosticReport(nullptr),
expressionValidationSkipped(false),
expressionValidationFailed(false) {};

} // namespace gd
15 changes: 15 additions & 0 deletions Core/GDCore/Events/CodeGeneration/EventsCodeGenerator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,6 +390,16 @@ class GD_CORE_API EventsCodeGenerator {

gd::DiagnosticReport* GetDiagnosticReport() { return diagnosticReport; }

/**
* \brief Return true if the validation of expressions can be skipped, because
* the instruction is generated again for another object of a group.
*/
bool IsExpressionValidationSkipped() const {
return expressionValidationSkipped;
}

void SetExpressionValidationFailed() { expressionValidationFailed = true; }

/**
* \brief Generate the full name for accessing to a boolean variable used for
* conditions.
Expand Down Expand Up @@ -879,6 +889,11 @@ class GD_CORE_API EventsCodeGenerator {
///< list function name.

gd::DiagnosticReport* diagnosticReport;
bool expressionValidationSkipped; ///< True when an instruction is generated
///< for the next objects of a group, after
///< its expressions were validated once.
bool expressionValidationFailed; ///< True if an expression validation
///< failed for the current instruction.
};

} // namespace gd
6 changes: 6 additions & 0 deletions Core/GDCore/Events/CodeGeneration/ExpressionCodeGenerator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,13 +52,19 @@ gd::String ExpressionCodeGenerator::GenerateExpressionCode(
return generator.GenerateDefaultValue(rootType);
}

if (codeGenerator.IsExpressionValidationSkipped()) {
node->Visit(generator);
return generator.GetOutput();
}

gd::ExpressionValidator validator(codeGenerator.GetPlatform(),
codeGenerator.GetProjectScopedContainers(),
rootType,
rootObjectName,
extraInfo);
node->Visit(validator);
if (!validator.GetFatalErrors().empty()) {
codeGenerator.SetExpressionValidationFailed();
std::cout << "Error: \"" << validator.GetFatalErrors()[0]->GetMessage()
<< "\" in: \"" << expression.GetPlainString() << "\" ("
<< rootType << ")" << std::endl;
Expand Down
58 changes: 58 additions & 0 deletions GDevelop.js/__tests__/GDJS.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,64 @@ describe('libGD.js - GDJS related tests', function () {

expect(code).toMatch('elseEventsChainSatisfied');
});
it('reports an invalid expression for each object of a group', function () {
const project = gd.ProjectHelper.createNewGDJSProject();
const layout = project.insertNewLayout('Scene', 0);
for (const objectName of ['MyObjectA', 'MyObjectB']) {
const object = layout
.getObjects()
.insertNewObject(project, 'Sprite', objectName, 0);
object.getVariables().insertNew('MyVariable', 0).setValue(0);
}
const group = layout
.getObjects()
.getObjectGroups()
.insertNew('MyGroup', 0);
group.addObject('MyObjectA');
group.addObject('MyObjectB');

const evt = layout
.getEvents()
.insertNewEvent(project, 'BuiltinCommonInstructions::Standard', 0);
const action = new gd.Instruction();
action.setType('SetNumberObjectVariable');
action.setParametersCount(4);
action.setParameter(0, 'MyGroup');
action.setParameter(1, 'MyVariable');
action.setParameter(2, '=');
action.setParameter(3, 'MyGroup.UndeclaredVariable');
gd.asStandardEvent(evt).getActions().insert(action, 0);
action.delete();

const layoutCodeGenerator = new gd.LayoutCodeGenerator(project);
const diagnosticReport = new gd.DiagnosticReport();
const code = layoutCodeGenerator.generateLayoutCompleteCode(
layout,
new gd.SetString(),
diagnosticReport,
true
);

// The action is generated once per object of the group, and the invalid
// expression is reported and replaced by a default value each time.
expect(diagnosticReport.count()).toBe(2);
expect(diagnosticReport.get(0).getActualValue()).toBe(
'UndeclaredVariable'
);
expect(diagnosticReport.get(1).getActualValue()).toBe(
'UndeclaredVariable'
);
expect(code).toMatch(
'gdjs.SceneCode.GDMyObjectAObjects1[i].returnVariable(gdjs.SceneCode.GDMyObjectAObjects1[i].getVariables().get("MyVariable")).setNumber(0);'
);
expect(code).toMatch(
'gdjs.SceneCode.GDMyObjectBObjects1[i].returnVariable(gdjs.SceneCode.GDMyObjectBObjects1[i].getVariables().get("MyVariable")).setNumber(0);'
);

diagnosticReport.delete();
layoutCodeGenerator.delete();
project.delete();
});
it('does not generate code for improperly set up actions/conditions', function () {
const project = gd.ProjectHelper.createNewGDJSProject();
const layout = project.insertNewLayout('Scene', 0);
Expand Down
Loading