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

'let' statements before using statements are now properly converted into 'var' statements #14260

Merged
merged 6 commits into from
Oct 10, 2024

Conversation

snoglobe
Copy link
Contributor

What does this PR do?

fixes #12407 and the regression demonstrated in #11083 where a let statement before a using would not be converted into a var so anything referencing it outside of the generated try/catch would raise a ReferenceError

  • Documentation or TypeScript types (it's okay to leave the rest blank in this case)
  • Code changes

How did you verify your code works?

Copy link
Collaborator

@Jarred-Sumner Jarred-Sumner left a comment

Choose a reason for hiding this comment

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

Several test failures

continue;
if (p.const_values.contains(decl.binding.data.b_identifier.ref)) {
any_decl_in_const_values = true;
const symbol = p.symbols.items[decl.binding.data.b_identifier.ref.innerIndex()];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const symbol = p.symbols.items[decl.binding.data.b_identifier.ref.innerIndex()];
const symbol = &p.symbols.items[decl.binding.data.b_identifier.ref.innerIndex()];

}
}
decls[end] = decl;
end += 1;
}
local.decls.len = @as(u32, @truncate(end));
if (end == 0) {
stmt.* = stmt.*.toEmpty();
if (any_decl_in_const_values) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this if be there? i'm confused

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's the only side effect that happens if it being const/in const_values is true. so we don't want it to happen if the check is false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(the locals.decls.len assignment is a no-op if it's false because end will be the len value. it should probably be moved into the if statement too though then)

Copy link
Collaborator

@Jarred-Sumner Jarred-Sumner left a comment

Choose a reason for hiding this comment

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

merge conflict

@snoglobe
Copy link
Contributor Author

snoglobe commented Oct 8, 2024

fixed

@Jarred-Sumner Jarred-Sumner merged commit ff47631 into main Oct 10, 2024
15 of 25 checks passed
@Jarred-Sumner Jarred-Sumner deleted the snoglobe/using branch October 10, 2024 02:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

using hoisting bug with generator functions in module scope
3 participants