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 edge case in 'findBindingAtPosition' when looking up global binding at start of file #1254

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

JohnnyMorganz
Copy link
Contributor

The 'findBindingAtPosition' AstQuery function can be used to lookup a local or global binding.

Inside of this function is a check to "Ignore this binding if we're inside its definition. e.g. local abc = abc -- Will take the definition of abc from outer scope".

However, this check is incorrect when we are looking up a global binding at the start of a file.

Consider a complete file with the contents:

local x = stri|ng.char(1)

and we pass the location of the marker | as the position to the find binding position. We will pick up the global binding of the definition string coming from a builtin source (either defined via C++ code or a definitions file and loaded into the global scope).

The global binding string will have a zero position: 0,0,0,0. However, the findBindingLocalStatement check works by looking up the AstAncestry at the binding's defined begin position in the current source module. This will then incorrectly return the local statement for local x, as that is at the start of the source code. Then in turn, we assume we are in the local abc = abc case, and end up skipping over the correct binding.

We fix this by checking if the binding is at the global position. If so, we early exit because it is impossible for a global binding to be defined in a local statement.

@JohnnyMorganz JohnnyMorganz changed the title Fix edge case in 'findBindingAtPosition' Fix edge case in 'findBindingAtPosition' when looking up global binding at start of file May 12, 2024
@@ -332,6 +332,11 @@ std::optional<TypeId> findExpectedTypeAtPosition(const Module& module, const Sou

static std::optional<AstStatLocal*> findBindingLocalStatement(const SourceModule& source, const Binding& binding)
{
// Bindings coming from global sources (e.g., definition files) have a zero position.
// They cannot be defined from a local statement
if (binding.location == Location{{0, 0}, {0, 0}})
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs to be flagged.

Suggested change
if (binding.location == Location{{0, 0}, {0, 0}})
if (FFlag::LuauFixBindingForGlobalPos && binding.location == Location{{0, 0}, {0, 0}})

(and an associated LUAU_FASTFLAG(LuauFixBindingForGlobalPos) at the top of the file).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants