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

Check lazy global uses #2632

Merged
merged 3 commits into from Jan 31, 2023
Merged

Check lazy global uses #2632

merged 3 commits into from Jan 31, 2023

Conversation

dcodeIO
Copy link
Member

@dcodeIO dcodeIO commented Jan 23, 2023

Fixes #2622, where it was discovered that global variables annotated as @global may not yet have been initialized when used, leading to subtle issues.

Turns out that the underlying problem is even broader, in that uses of global variables currently unconditionally compile these and their initializers out-of-band, possibly leading to subtle side-effects like the one seen in imported files declaring a @global variable. Hence this PR adds a check whenever a global is to be compiled lazily, and unless the global is annotated as @lazy, is built-in or ambient (where order is not relevant), diagnoses uses before their declaration.

cc @HerrCai0907, as this also supersedes #2624.

  • I've read the contributing guidelines
  • I've added my name and email to the NOTICE file

@dcodeIO
Copy link
Member Author

dcodeIO commented Jan 23, 2023

Interestingly, since this checks at compile time (not at runtime as in JS) for any possible lazy use of this kind, bootstrap finds that checkConstantType_expr in src/builtins.ts for example may be used before its declaration. It actually is not, but it could be indirectly, since functions calling checkConstantType are added to a Map in top-level code above, so it cannot be guaranteed that checkConstantType isn't called via that Map in code executing earlier than the var's initializer. Makes sense upon closer look, but could well lead to unexpected diagnostics, hmm.

let fn = foo;

fn(); // (1) possibly

let a = 1;
function foo() {
  a = 2; // if (1): ReferenceError: Cannot access 'a' before initialization
}

The mangleImportName_* case is similar, but with one more indirection. The function mangleImportName is called in Compiler#compileFunction, and at the top of the file, src/builtins.ts is imported, which adds builtin_memory_copy and builtin_memory_fill to the same map, both of which call Compiler#compileFunction. So at the time src/builtins.ts is imported, mangleImport might have been called with the helper globals not yet initialized.

Overall, my expectation is that this change will break some code in the wild, similar to what happened in the compiler. I do not see a good alternative, though. In fact, the ability to diagnose this at compile time is a lucky coincidence as a side-product of categorical tree-shaking, whereas, unless I am missing something, anything else would involve a bunch of invisible runtime checks.

@dcodeIO dcodeIO requested a review from MaxGraey January 23, 2023 18:11
@MaxGraey
Copy link
Member

Does this a breaking change? I mean, it could potentially break existing code

@dcodeIO
Copy link
Member Author

dcodeIO commented Jan 23, 2023

Yes, this is a breaking change, and I'd expect it to break some existing code.

@dcodeIO dcodeIO merged commit 5cbbf84 into main Jan 31, 2023
@HerrCai0907 HerrCai0907 deleted the issue-2622 branch October 17, 2023 09:00
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.

@global causes compile error
3 participants