Fix missing error messages on variable fields (#9078)

This commit is contained in:
D8H
2026-09-09 18:35:55 +02:00
committed by GitHub
parent 6a6290648f
commit 1149a67963
17 changed files with 148 additions and 258 deletions
@@ -191,6 +191,14 @@ class GD_CORE_API ExpressionValidator : public ExpressionParser2NodeWorker {
RaiseTypeError(
_("You entered a number, but a text was expected (in quotes)."),
node.location);
} else if (parentType == Type::Variable ||
parentType == Type::ObjectVariable ||
parentType == Type::LegacyVariable) {
RaiseTypeError(
_("The variable name looks like you're building an expression or a "
"formula. You can only use this for structure or arrays, for "
"example: Score[3]."),
node.location);
} else if (parentType != Type::Number &&
parentType != Type::NumberOrString) {
RaiseTypeError(_("You entered a number, but this type was expected:") +
@@ -204,6 +212,14 @@ class GD_CORE_API ExpressionValidator : public ExpressionParser2NodeWorker {
if (parentType == Type::Number) {
RaiseTypeError(_("You entered a text, but a number was expected."),
node.location);
} else if (parentType == Type::Variable ||
parentType == Type::ObjectVariable ||
parentType == Type::LegacyVariable) {
RaiseTypeError(
_("The variable name looks like you're building an expression or a "
"formula. You can only use this for structure or arrays, for "
"example: Score[\"Player1\"]."),
node.location);
} else if (parentType != Type::String &&
parentType != Type::NumberOrString) {
RaiseTypeError(_("You entered a text, but this type was expected:") +
+31 -21
View File
@@ -66,27 +66,8 @@ ParameterValidationResult InstructionValidator::ValidateParameter(
gd::ParameterMetadata::IsExpression("string", parameterType) ||
gd::ParameterMetadata::IsExpression("variable", parameterType)) {
// New object variable instructions require the variable to be
// declared while legacy ones don't.
// For legacy variable instruction, we pass an empty object name.
gd::String rootObjectName = "";
if (parameterType == "objectvar") {
const auto &objectsContainersList =
projectScopedContainers.GetObjectsContainersList();
rootObjectName = instruction.GetParameter(0).GetPlainString();
if (!gd::VariableInstructionSwitcher::IsSwitchableVariableInstruction(
instruction.GetType())) {
// Extensions still rely on legacy object variables instructions.
auto objectSourceType =
projectScopedContainers.GetObjectsContainersList()
.GetObjectsContainerSourceType(rootObjectName);
// Only child-object variable declarations are checked.
if (objectSourceType != gd::ObjectsContainer::SourceType::Object) {
rootObjectName = "";
}
}
}
gd::String rootObjectName = InstructionValidator::GetObjectNameForParameter(
projectScopedContainers, instruction, parameterType);
auto &expressionNode =
*instruction.GetParameter(parameterIndex).GetRootNode();
ExpressionValidator expressionValidator(platform, projectScopedContainers,
@@ -123,6 +104,35 @@ ParameterValidationResult InstructionValidator::ValidateParameter(
return result;
}
gd::String InstructionValidator::GetObjectNameForParameter(
const gd::ProjectScopedContainers projectScopedContainers,
const gd::Instruction &instruction, const gd::String &parameterType) {
// New object variable instructions require the variable to be
// declared while legacy ones don't.
// For legacy variable instruction, we pass an empty object name.
gd::String rootObjectName = "";
if (parameterType != "objectvar") {
return "";
}
if (instruction.GetParametersCount() == 0) {
return "";
}
rootObjectName = instruction.GetParameter(0).GetPlainString();
if (!gd::VariableInstructionSwitcher::IsSwitchableVariableInstruction(
instruction.GetType())) {
// Extensions still rely on legacy object variables instructions.
auto objectSourceType =
projectScopedContainers.GetObjectsContainersList()
.GetObjectsContainerSourceType(rootObjectName);
// Only child-object variable declarations are checked.
if (objectSourceType != gd::ObjectsContainer::SourceType::Object) {
return "";
}
}
return rootObjectName;
}
bool InstructionValidator::IsParameterValid(
const gd::Platform &platform,
const gd::ProjectScopedContainers projectScopedContainers,
+4
View File
@@ -63,6 +63,10 @@ public:
static gd::String GetRootVariableName(const gd::String &name);
static gd::String GetObjectNameForParameter(
const gd::ProjectScopedContainers projectScopedContainers,
const gd::Instruction &instruction, const gd::String &parameterType);
private:
static bool
HasRequiredBehaviors(const gd::Instruction &instruction,
+6 -2
View File
@@ -4033,7 +4033,9 @@ TEST_CASE("ExpressionParser2", "[common][events]") {
node->Visit(validator);
RequireFatalErrorsCount(validator, 1);
REQUIRE(validator.GetFatalErrors()[0]->GetMessage() ==
"You entered a number, but this type was expected: variable");
"The variable name looks like you're building an expression or a "
"formula. You can only use this for structure or arrays, for "
"example: Score[3].");
}
SECTION("string instead") {
auto node = parser.ParseExpression("\"text\"");
@@ -4043,7 +4045,9 @@ TEST_CASE("ExpressionParser2", "[common][events]") {
node->Visit(validator);
RequireFatalErrorsCount(validator, 1);
REQUIRE(validator.GetFatalErrors()[0]->GetMessage() ==
"You entered a text, but this type was expected: variable");
"The variable name looks like you're building an expression or a "
"formula. You can only use this for structure or arrays, for "
"example: Score[\"Player1\"].");
}
SECTION("Object variable with unary operator") {
+4
View File
@@ -3128,6 +3128,10 @@ interface InstructionValidator {
[Const, Ref] Instruction instruction,
[Const, Ref] InstructionMetadata metadata,
long parameterIndex);
[Const, Value] DOMString STATIC_GetObjectNameForParameter(
[Const, Ref] ProjectScopedContainers projectScopedContainers,
[Const, Ref] Instruction instruction,
[Const] DOMString parameterType);
};
interface ObjectTools {
+1
View File
@@ -746,6 +746,7 @@ typedef std::vector<gd::PropertyDescriptorChoice> VectorPropertyDescriptorChoice
#define STATIC_FillBehaviorParameters FillBehaviorParameters
#define STATIC_ValidateParameter ValidateParameter
#define STATIC_IsParameterValid IsParameterValid
#define STATIC_GetObjectNameForParameter GetObjectNameForParameter
#define STATIC_FixInvalidRequiredBehaviorProperties \
FixInvalidRequiredBehaviorProperties
#define STATIC_RemoveLayerInScene RemoveLayerInScene
+1
View File
@@ -2252,6 +2252,7 @@ export class ParameterValidationResult extends EmscriptenObject {
export class InstructionValidator extends EmscriptenObject {
static validateParameter(platform: Platform, projectScopedContainers: ProjectScopedContainers, instruction: Instruction, metadata: InstructionMetadata, parameterIndex: number): ParameterValidationResult;
static isParameterValid(platform: Platform, projectScopedContainers: ProjectScopedContainers, instruction: Instruction, metadata: InstructionMetadata, parameterIndex: number): boolean;
static getObjectNameForParameter(projectScopedContainers: ProjectScopedContainers, instruction: Instruction, parameterType: string): string;
}
export class ObjectTools extends EmscriptenObject {
@@ -2,6 +2,7 @@
declare class gdInstructionValidator {
static validateParameter(platform: gdPlatform, projectScopedContainers: gdProjectScopedContainers, instruction: gdInstruction, metadata: gdInstructionMetadata, parameterIndex: number): gdParameterValidationResult;
static isParameterValid(platform: gdPlatform, projectScopedContainers: gdProjectScopedContainers, instruction: gdInstruction, metadata: gdInstructionMetadata, parameterIndex: number): boolean;
static getObjectNameForParameter(projectScopedContainers: gdProjectScopedContainers, instruction: gdInstruction, parameterType: string): string;
delete(): void;
ptr: number;
};
@@ -103,10 +103,8 @@ export default (React.forwardRef<ParameterFieldProps, ParameterFieldInterface>(
return (
<React.Fragment>
<VariableField
forceDeclaration
project={project}
instruction={instruction}
isObjectVariable={false}
variablesContainers={variablesContainers}
enumerateVariables={enumerateGlobalAndSceneVariables}
parameterMetadata={props.parameterMetadata}
@@ -102,10 +102,8 @@ export default (React.forwardRef<ParameterFieldProps, ParameterFieldInterface>(
return (
<React.Fragment>
<VariableField
forceDeclaration
project={project}
instruction={instruction}
isObjectVariable={false}
variablesContainers={variablesContainers}
enumerateVariables={enumerateGlobalAndSceneVariables}
parameterMetadata={props.parameterMetadata}
@@ -99,10 +99,8 @@ export default (React.forwardRef<ParameterFieldProps, ParameterFieldInterface>(
return (
<React.Fragment>
<VariableField
forceDeclaration
project={project}
instruction={instruction}
isObjectVariable={false}
variablesContainers={variablesContainers}
enumerateVariables={enumerateGlobalAndSceneVariables}
parameterMetadata={props.parameterMetadata}
@@ -129,13 +129,14 @@ type Props = {|
const MAX_ERRORS_COUNT = 10;
const extractErrors = (
export const extractErrors = (
platform: gdPlatform,
project: gdProject,
projectScopedContainersAccessor: ProjectScopedContainersAccessor,
expressionType: string,
parameterMetadata: ?gdParameterMetadata,
expressionNode: gdExpressionNode,
objectName: string,
showDeprecatedInstructionWarning:
| 'no'
| 'icon'
@@ -149,7 +150,7 @@ const extractErrors = (
gd.JsPlatform.get(),
projectScopedContainersAccessor.get(),
expressionType,
'',
objectName,
parameterMetadata ? parameterMetadata.getExtraInfo() : ''
);
expressionNode.visit(expressionValidator);
@@ -496,6 +497,7 @@ export default class ExpressionField extends React.Component<Props, State> {
expressionType,
parameterMetadata,
expressionNode,
'',
showDeprecatedInstructionWarning
);
const extraErrorText = onExtractAdditionalErrors
@@ -49,7 +49,6 @@ export default (React.forwardRef<ParameterFieldProps, ParameterFieldInterface>(
return (
<React.Fragment>
<VariableField
isObjectVariable={false}
variablesContainers={variablesContainers}
enumerateVariables={enumerateGlobaleVariables}
parameterMetadata={props.parameterMetadata}
@@ -179,15 +179,8 @@ export default (React.forwardRef<ParameterFieldProps, ParameterFieldInterface>(
return (
<React.Fragment>
<VariableField
forceDeclaration={
instruction &&
gd.VariableInstructionSwitcher.isSwitchableVariableInstruction(
instruction.getType()
)
}
project={project}
instruction={instruction}
isObjectVariable={true}
variablesContainers={variablesContainers}
enumerateVariables={enumerateObjectVariables}
parameterMetadata={props.parameterMetadata}
@@ -76,7 +76,6 @@ export default (React.forwardRef<ParameterFieldProps, ParameterFieldInterface>(
return (
<React.Fragment>
<VariableField
isObjectVariable={false}
variablesContainers={variablesContainers}
enumerateVariables={enumerateSceneVariables}
parameterMetadata={props.parameterMetadata}
@@ -13,7 +13,6 @@ import classNames from 'classnames';
import {
icon,
nameAndIconContainer,
instructionWarningParameter,
instructionParameter,
} from '../EventsTree/ClassNames';
import SemiControlledAutoComplete, {
@@ -46,10 +45,10 @@ import ObjectVariableIcon from '../../UI/CustomSvgIcons/ObjectVariable';
import LocalVariableIcon from '../../UI/CustomSvgIcons/LocalVariable';
import PropertyIcon from '../../UI/CustomSvgIcons/Settings';
import ParameterIcon from '../../UI/CustomSvgIcons/Parameter';
import { ProjectScopedContainersAccessor } from '../../InstructionOrExpression/EventsScope';
import Link from '../../UI/Link';
import Add from '../../UI/CustomSvgIcons/Add';
import { type VariableDialogOpeningProps } from '../../VariablesList/VariablesEditorDialog';
import { extractErrors } from './GenericExpressionField';
import { useDebounce } from '../../Utils/UseDebounce';
const gd: libGDevelop = global.gd;
@@ -69,14 +68,12 @@ const getVariableTypeName = (
type Props = {
...ParameterFieldProps,
isObjectVariable: boolean,
variablesContainers: Array<gdVariablesContainer>,
getVariableSourceFromIdentifier: (
identifier: string,
projectScopedContainers: gdProjectScopedContainers
) => VariablesContainer_SourceType,
enumerateVariables: () => Array<EnumeratedVariable>,
forceDeclaration?: boolean,
openVariableEditorDialog: (VariableDialogOpeningProps => void) | null,
editEventsFunctionParameter: (VariableDialogOpeningProps => void) | null,
openEventsBasedEntityPropertyEditorDialog:
@@ -84,26 +81,11 @@ type Props = {
| null,
};
type VariableNameQuickAnalyzeResult = 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7;
export type VariableFieldInterface = {|
...ParameterFieldInterface,
updateAutocompletions: () => void,
|};
export const VariableNameQuickAnalyzeResults: {
[string]: VariableNameQuickAnalyzeResult,
} = {
OK: 0,
WRONG_QUOTE: 1,
WRONG_SPACE: 2,
WRONG_EXPRESSION: 3,
UNDECLARED_VARIABLE: 4,
NAME_COLLISION_WITH_OBJECT: 5,
PARAMETER_WITH_CHILD: 6,
PROPERTY_WITH_CHILD: 7,
};
export const getRootVariableName = (name: string): string => {
const dotPosition = name.indexOf('.');
const squareBracketPosition = name.indexOf('[');
@@ -130,83 +112,6 @@ const isRootVariableDeclared = (
);
};
// TODO: the entire VariableField could be reworked to be a "real" GenericExpressionField
// (of type: "variable" or the legacy: "scenevar", "globalvar" or "objectvar"). This will
// ensure we 100% validate and can autocomplete what is entered (and we can have also a simpler
// selector that offers the variables in the scope).
export const quicklyAnalyzeVariableName = (
name: string,
variablesContainers?: Array<gdVariablesContainer>,
getVariableSourceFromIdentifier?: (
identifier: string,
projectScopedContainers: gdProjectScopedContainers
) => VariablesContainer_SourceType | null,
projectScopedContainersAccessor?: ProjectScopedContainersAccessor,
isObjectVariable: boolean = false
): VariableNameQuickAnalyzeResult => {
if (!name) return VariableNameQuickAnalyzeResults.OK;
for (let i = 0; i < name.length; ++i) {
const character = name[i];
if (character === '[') {
// This probably starts an expression, so stop the analysis.
break;
} else if (character === ' ') {
return VariableNameQuickAnalyzeResults.WRONG_SPACE;
} else if (character === '"') {
return VariableNameQuickAnalyzeResults.WRONG_QUOTE;
} else if (
character === '(' ||
character === '+' ||
character === '-' ||
character === '/' ||
character === '*'
) {
return VariableNameQuickAnalyzeResults.WRONG_EXPRESSION;
}
}
const rootVariableName = getRootVariableName(name);
// Check at least the name of the root variable, it's the best we can do.
if (!isRootVariableDeclared(rootVariableName, variablesContainers)) {
return VariableNameQuickAnalyzeResults.UNDECLARED_VARIABLE;
}
if (!projectScopedContainersAccessor) {
return VariableNameQuickAnalyzeResults.OK;
}
const projectScopedContainers = projectScopedContainersAccessor.get();
if (
!isObjectVariable &&
projectScopedContainers
.getObjectsContainersList()
.hasObjectOrGroupNamed(rootVariableName)
) {
return VariableNameQuickAnalyzeResults.NAME_COLLISION_WITH_OBJECT;
}
if (
name.length !== rootVariableName.length &&
getVariableSourceFromIdentifier
) {
const variableSource = getVariableSourceFromIdentifier(
rootVariableName,
projectScopedContainers
);
if (variableSource === gd.VariablesContainer.Parameters) {
return VariableNameQuickAnalyzeResults.PARAMETER_WITH_CHILD;
}
if (variableSource === gd.VariablesContainer.Properties) {
return VariableNameQuickAnalyzeResults.PROPERTY_WITH_CHILD;
}
}
return VariableNameQuickAnalyzeResults.OK;
};
export const getVariableSourceIcon = (
variableSourceType: VariablesContainer_SourceType
): any => {
@@ -247,6 +152,11 @@ export const getVariableTypeIcon = (variableType: Variable_Type): any => {
}
};
// TODO: the entire VariableField could be reworked to be a "real" GenericExpressionField
// (of type: "variable" or the legacy: "scenevar", "globalvar" or "objectvar"). This will
// ensure we 100% validate and can autocomplete what is entered (and we can have also a simpler
// selector that offers the variables in the scope).
export default (React.forwardRef<Props, VariableFieldInterface>(
function VariableField(props: Props, ref) {
const {
@@ -255,7 +165,6 @@ export default (React.forwardRef<Props, VariableFieldInterface>(
variablesContainers,
enumerateVariables,
instruction,
forceDeclaration,
value,
onChange,
isInline,
@@ -264,7 +173,6 @@ export default (React.forwardRef<Props, VariableFieldInterface>(
onApply,
id,
onInstructionTypeChanged,
isObjectVariable,
getVariableSourceFromIdentifier,
openVariableEditorDialog,
editEventsFunctionParameter,
@@ -436,68 +344,78 @@ export default (React.forwardRef<Props, VariableFieldInterface>(
? parameterMetadata.getDescription()
: undefined;
const quicklyAnalysisResult = quicklyAnalyzeVariableName(
value,
variablesContainers,
getVariableSourceFromIdentifier,
projectScopedContainersAccessor,
isObjectVariable
const [errorText, setErrorText] = React.useState<?string>(null);
const inputValue = React.useRef<string | null>(null);
const doValidation = React.useCallback(
() => {
if (!project || !parameterMetadata || !instruction) return null;
// Parsing can be time consuming (~1ms for simple expression,
// a few milliseconds for complex ones).
const parser = new gd.ExpressionParser2();
const expressionNode = parser
.parseExpression(inputValue.current || value)
.get();
const expressionType = parameterMetadata
.getValueTypeMetadata()
.getName();
const objectName = gd.InstructionValidator.getObjectNameForParameter(
projectScopedContainersAccessor.get(),
instruction,
expressionType
);
const { errorText } = extractErrors(
gd.JsPlatform.get(),
project,
projectScopedContainersAccessor,
expressionType,
parameterMetadata,
expressionNode,
objectName,
'no'
);
parser.delete();
setErrorText(errorText);
},
[
instruction,
parameterMetadata,
project,
projectScopedContainersAccessor,
value,
]
);
const errorText =
quicklyAnalysisResult === VariableNameQuickAnalyzeResults.WRONG_QUOTE ? (
<Trans>
It seems you entered a name with a quote. Variable names should not be
quoted.
</Trans>
) : quicklyAnalysisResult ===
VariableNameQuickAnalyzeResults.WRONG_SPACE ? (
<Trans>
The variable name contains a space - this is not recommended. Prefer
to use underscores or uppercase letters to separate words.
</Trans>
) : quicklyAnalysisResult ===
VariableNameQuickAnalyzeResults.WRONG_EXPRESSION ? (
<Trans>
The variable name looks like you're building an expression or a
formula. You can only use this for structure or arrays. For example:
Score[3].
</Trans>
) : forceDeclaration &&
quicklyAnalysisResult ===
VariableNameQuickAnalyzeResults.UNDECLARED_VARIABLE ? (
<Trans>
This variable does not exist.{' '}
<Link onClick={openVariableEditor} href="#">
Click to add it.
</Link>
</Trans>
) : forceDeclaration &&
quicklyAnalysisResult ===
VariableNameQuickAnalyzeResults.NAME_COLLISION_WITH_OBJECT ? (
<Trans>
This variable has the same name as an object. Consider renaming one or
the other.
</Trans>
) : forceDeclaration &&
quicklyAnalysisResult ===
VariableNameQuickAnalyzeResults.PARAMETER_WITH_CHILD ? (
<Trans>Parameters can't have children.</Trans>
) : forceDeclaration &&
quicklyAnalysisResult ===
VariableNameQuickAnalyzeResults.PROPERTY_WITH_CHILD ? (
<Trans>Properties can't have children.</Trans>
) : null;
const warningTranslatableText =
!forceDeclaration &&
quicklyAnalysisResult ===
VariableNameQuickAnalyzeResults.UNDECLARED_VARIABLE
? t`This variable is not declared. It's recommended to use the *variables editor* to add it.`
: !forceDeclaration &&
quicklyAnalysisResult ===
VariableNameQuickAnalyzeResults.NAME_COLLISION_WITH_OBJECT
? t`This variable has the same name as an object. Consider renaming one or the other.`
: null;
const enqueueValidation = useDebounce(() => {
doValidation();
}, 250);
React.useEffect(
() => {
enqueueValidation();
},
[enqueueValidation]
);
const handleValueChange = React.useCallback(
(value: string) => {
inputValue.current = null;
onChange(value);
},
[onChange]
);
const handleInputValueChange = React.useCallback(
(value: string) => {
inputValue.current = value;
enqueueValidation();
},
[enqueueValidation]
);
const isSwitchableInstruction =
instruction &&
@@ -561,16 +479,15 @@ export default (React.forwardRef<Props, VariableFieldInterface>(
margin={isInline ? 'none' : 'dense'}
floatingLabelText={description}
helperMarkdownText={
warningTranslatableText
? i18n._(warningTranslatableText)
: parameterMetadata
parameterMetadata
? parameterMetadata.getLongDescription()
: undefined
}
errorText={errorText}
fullWidth
value={value}
onChange={onChange}
onChange={handleValueChange}
onInputValueChange={handleInputValueChange}
onRequestClose={onRequestClose}
onApply={onApply}
filterOptionById={filterOptionById}
@@ -763,9 +680,6 @@ export const renderVariableWithIcon = (
title={tooltip}
className={classNames({
[nameAndIconContainer]: true,
[instructionWarningParameter]:
quicklyAnalyzeVariableName(value) !==
VariableNameQuickAnalyzeResults.OK,
})}
>
<IconAndNameContainer>
@@ -1,52 +0,0 @@
// @flow
import {
quicklyAnalyzeVariableName,
VariableNameQuickAnalyzeResults,
} from './VariableField';
describe('VariableField', () => {
it('can quickly analyze if a variable name or expression looks good', () => {
expect(quicklyAnalyzeVariableName('Test')).toBe(
VariableNameQuickAnalyzeResults.OK
);
expect(quicklyAnalyzeVariableName('Test123')).toBe(
VariableNameQuickAnalyzeResults.OK
);
expect(quicklyAnalyzeVariableName('Hello world')).toBe(
VariableNameQuickAnalyzeResults.WRONG_SPACE
);
expect(quicklyAnalyzeVariableName(' "Test"')).toBe(
VariableNameQuickAnalyzeResults.WRONG_SPACE
);
expect(quicklyAnalyzeVariableName('"Test"')).toBe(
VariableNameQuickAnalyzeResults.WRONG_QUOTE
);
expect(quicklyAnalyzeVariableName('VariableString(MySubVariable)')).toBe(
VariableNameQuickAnalyzeResults.WRONG_EXPRESSION
);
expect(quicklyAnalyzeVariableName('Test+2')).toBe(
VariableNameQuickAnalyzeResults.WRONG_EXPRESSION
);
expect(quicklyAnalyzeVariableName('MyVariable.MySubVariable')).toBe(
VariableNameQuickAnalyzeResults.OK
);
expect(quicklyAnalyzeVariableName('MyVariable.MySubVariable["Test"]')).toBe(
VariableNameQuickAnalyzeResults.OK
);
expect(quicklyAnalyzeVariableName('MyVariable.MySubVariable[1 + 2]')).toBe(
VariableNameQuickAnalyzeResults.OK
);
expect(quicklyAnalyzeVariableName('MyVariable.MySubVariable+2')).toBe(
VariableNameQuickAnalyzeResults.WRONG_EXPRESSION
);
expect(
quicklyAnalyzeVariableName('MyVariable[VariableString(AnotherVariable)]')
).toBe(VariableNameQuickAnalyzeResults.OK);
expect(
quicklyAnalyzeVariableName('MyVariable[Variable(AnotherVariable)]')
).toBe(VariableNameQuickAnalyzeResults.OK);
expect(
quicklyAnalyzeVariableName('MyVariable[Variable(AnotherVariable) + 2]')
).toBe(VariableNameQuickAnalyzeResults.OK);
});
});