Repository navigation
[script] Fix $var not finding an exported variable when an entity of the same name is in scope - #2389
Open
andrewgazelka wants to merge 1 commit into
Conversation
`$name` resolved an exported variable with the regular entity name lookup and
gave up when the first entity it found wasn't a variable. An entity with the
same name in an enclosing scope, such as the one being defined, hid the
variable:
export const x = 10
parent {
x { Position: {$x, 20} } // unresolved reference 'x'
}
The lookup for `$name` now continues past entities that aren't variables, scope
by scope, through the variables declared in the script and the ones already in
the world.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
(AI found and written)
A
$namereference to an exported variable fails withunresolved referencewhen an entity of the same name is in scope, most often the entity being defined. Renaming either one makes it work. I couldn't find an existing issue for it.It also fails within one script (
export const x = 10followed by theparentblock above), and when the variable is exported in an enclosing scope (outer { export const x = 10 }, thenouter { x { inner { x { Position: {$x, 20} } } } }).Cause.
flecs_expr_global_variable_resolve(src/addons/script/expr/visit_type.c:1896) resolves$xwith the regular entity lookup,flecs_script_symbol_lookup(..., FlecsScriptLookupAll), and accepts only the first match. That lookup finds the nearest entity namedx: first the script's own declarations (flecs_script_type_symbol_lookup, which here returns thexbeing defined), then the world lookup fromv->parentwithrecursive = true(src/addons/script/eval/visit_eval.c:251), which findsparent.xbefore the rootx. When that entity isn't a variable, the resolve records an unresolved reference and stops, even though an exported variable namedxis in an enclosing scope.Fix.
$always means a variable, so an entity that isn't a variable shouldn't hide one. The script already treats a local variable that shadows an exported one as an error ("local variable '%s' shadows an exported variable"), so name hiding isn't something the lookup is meant to allow for variables. The newflecs_script_global_var_lookup(visit_eval.c) runs the same lookup first, so every case that resolves today resolves to the same thing. Only when the result is an entity that isn't a variable does it keep searching, innermost scope first, and return the nearest variable:flecs_script_type_global_var_lookup(visit_type.c) walks the scope tables likeflecs_script_type_finddoes, checking each scope for variables the script declares and for variables already in the world under that scope's entity. The scope entity is found without creating it (from its symbol slot, or as an existing child by name). This covers the nested case, where the type pass'sv->parentis still the script root;v->parentup to the root, and theusingscopes.If no variable is found, the error is the same as before. The change only affects
$name. Bare identifiers and assignment to exportedmutvariables still use the normal entity lookup.Tests (
test/script,Eval):export_const_var_w_entity_same_name_in_scope: the example above, two scripts.export_const_var_w_entity_same_name_in_scope_same_scriptexport_const_var_w_entity_same_name_in_nested_scope: variable exported inouterby one script, used fromouter.x.inner.xby the next.export_const_var_w_entity_same_name_nearest_var: withxexported at the root and inouter,$xinouter.inner.xgets the one fromouter.export_mut_var_w_entity_same_name_in_scopevar_w_entity_same_name_no_var:$xwith only the entityxin scope still errors.The first five fail on master. The whole script suite passes on this branch: 8880 passed, 0 failed, with IR both disabled and enabled (macOS arm64, clang,
bake run test/script).distr/flecs.calso builds with the amalgamated CI flags (-pedantic -Wall -Wextra -Werror -Wshadow -Wconversion).