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

No error when an arrow function uses a variable from an unreachable branch #56827

Closed
SLaks opened this issue Dec 18, 2023 · 24 comments
Closed

No error when an arrow function uses a variable from an unreachable branch #56827

SLaks opened this issue Dec 18, 2023 · 24 comments
Labels
Duplicate An existing issue was already created

Comments

@SLaks
Copy link

SLaks commented Dec 18, 2023

πŸ”Ž Search Terms

variable used before declaration function

πŸ•— Version & Regression Information

  • This is the behavior in every version I tried since v3.3.3333

⏯ Playground Link

https://www.typescriptlang.org/play?ts=5.3.2#code/BTCUAIF4D5wbwFDnASwGbmAWQIYBcALAOhwCMBnYARgkjvBviWXACcBTARwFd3y8AggDsUAW3woA9kIBirHKPYhasAMbTykgDbsiWyQHNgaSZNCgA3M2Qc83VkKvIAvs3VD+4E5KjgATE7g7po6eobGppYIzqBgVkA

πŸ’» Code

(() => {
  if (Math.abs(1) === 1) {
    requestAnimationFrame(() => console.log(foo));
    return;
  }
  const foo = 2;
  console.log(foo);
})();

πŸ™ Actual behavior

Fails at runtime with

Uncaught ReferenceError: Cannot access 'foo' before initialization

πŸ™‚ Expected behavior

The same error, but at compile time

Additional information about the issue

If you replace requestAnimationFrame() with an IIFE (``), it correct

@fatcerberus
Copy link

(Probably) another duplicate of #9998.

@SLaks
Copy link
Author

SLaks commented Dec 18, 2023

I don't think so.

This is not entirely about control flow analysis; the foo variable is entirely out of scope in the always-terminating if(){} block above it.

In particular, I'm not complaining about locals used before they're declared (eg, if you move const foo to directly below requestAnimationFrame()), which is actually valid.

@fatcerberus
Copy link

fatcerberus commented Dec 18, 2023

It’s not actually out of scope, just in the temporal deadzone (the identifier itself is hoisted), which is the purview of CFA.

in other words it’s only β€œout of scope” because you return early and the variable never leaves TDZ.

@craigphicks
Copy link

craigphicks commented Dec 19, 2023

@fatcerebus -

I don't know whether you are talking about JS or TS hoisting of const variable scope.

JS hoists var variables to the stop of their scope, but not their initialization. It doesn't hoist const variables - otherwise the runtime error would not have happened.

TS CFA treats const variables as if they were declared and initialized at the top of their resident scope. If it knew const foo was in a temporal dead zone it would make an error, but it doesn't recognize that in this particular case even if Math.abs(1) is changed to 1==1.

@fatcerberus
Copy link

@craigphicks #9998 still applies; the same design limitations that cause the problems described there are at play here.

It doesn't hoist const variables - otherwise the runtime error would not have happened.

This is a common misconception. const/let are still hoisted, they’re just in TDZ instead of being undefined like var would be (the TDZ check happens at runtime, which is why using const/let can sometimes be a performance issue vs. var). If they weren’t hoisted then this would work:

const foo = "bar";
{
    // console.log(foo);  // nope, runtime error
    const foo = "qux";
    console.log(foo);  // ok, "qux"
}

and the below code does work precisely because the variable is hoisted:

setTimeout(() => console.log(foo), 0);  // ok, "bar"
const foo = "bar";

@craigphicks
Copy link

craigphicks commented Dec 19, 2023

@fatcerberus

Your case 1.1 does show something.

const foo = "bar";
{
    console.log(foo);  // nope, runtime error
    const foo = "qux";
}

The const is not hoisted like var would be, because for var it would show undefined. But the JS runtime compiler has checked all scoping and removed the outer foo from the inner scope before executing the inner scope because a redefinition exists in the inner scope.

The second case works only because setTimeout yields control to continue to the next line - the call back won't execute at least until control yields again. However, that case does provide a good argument for CFA to treat all const variable as though they are declared at the top of their scope rather than erroring, because specializing for async control cases would be time consuming.

@fatcerberus
Copy link

fatcerberus commented Dec 19, 2023

But the JS runtime compiler has checked all scoping and removed the outer foo from the inner scope before executing the inner scope because a redefinition exists in the inner scope.

This is just a roundabout way to say the inner const is hoisted to the top of its scope. Semantically, the hoisting is almost the same as for var, it just throws on access instead of returning undefined (the error is different than the one for accessing an out-of-scope variable). TDZ is a runtime phenomenon; it doesn’t correspond to lexical scope.

@RyanCavanaugh
Copy link
Member

This is a duplicate of #11498. It's sometimes valid and idiomatic to write this code, but we don't know if it's safe or not unless we know the semantics of the receiver of the callback. It's not practical to always error on this, so the only thing we can do is never error.

@RyanCavanaugh RyanCavanaugh added the Duplicate An existing issue was already created label Dec 19, 2023
@fatcerberus
Copy link

Thanks @RyanCavanaugh, I knew duping to #9998 was sketchy and I always forget about #11498 😝

@SLaks
Copy link
Author

SLaks commented Dec 19, 2023

It's sometimes valid and idiomatic to write this code, but we don't know if it's safe or not unless we know the semantics of the receiver of the callback.

Not in this case; because the if() block always terminates, this can never be valid (no matter when the callback runs, the variable can never have been declared.

@SLaks
Copy link
Author

SLaks commented Dec 19, 2023

IOW, if a variable's declaration is never reachable from a nested function's definition, you should emit an error when using the variable from inside the nested function.

@fatcerberus
Copy link

fatcerberus commented Dec 19, 2023

I don't think the control-flow analyzer is that sophisticated; the const is reachable within its own scope, so the compiler is satisfied. I'm pretty sure there's no internal infrastructure for analyzing the control flow of individual references to foo relative to the original declaration, and it would probably be a performance issue to do so.

@craigphicks
Copy link

craigphicks commented Dec 19, 2023

But the JS runtime compiler has checked all scoping and removed the outer foo from the inner scope before executing the inner scope because a redefinition exists in the inner scope.

This is just a roundabout way to say the inner const is hoisted to the top of its scope. Semantically, the hoisting is almost the same as for var, it just throws on access instead of returning undefined (the error is different than the one for accessing an out-of-scope variable). TDZ is a runtime phenomenon; it doesn’t correspond to lexical scope.

It's complicated, but there's help from MDN:

Hoisting is not a term normatively defined in the ECMAScript specification. The spec does define a group of declarations as HoistableDeclaration, but this only includes function, function*, async function, and async function* declarations. Hoisting is often considered a feature of var declarations as well, although in a different way. In colloquial terms, any of the following behaviors may be regarded as hoisting:

  1. Being able to use a variable's value in its scope before the line it is declared. ("Value hoisting")
  2. Being able to reference a variable in its scope before the line it is declared, without throwing a ReferenceError, but the value is always undefined. ("Declaration hoisting")
  3. The declaration of the variable causes behavior changes in its scope before the line in which it is declared.
  4. The side effects of a declaration are produced before evaluating the rest of the code that contains it.

... let, const, and class declarations (also collectively called lexical declarations) are hoisted with type 3 behavior

Could we say TDZ correlates with lexical scope?

@fatcerberus
Copy link

fatcerberus commented Dec 19, 2023

No, because it doesn’t:

switch ("foo" as string) {
    case "bar":
        const foo = 0;
    case "foo":
        console.log(foo);  // runtime error (TDZ)
}

foo is lexically in scope here, but errors anyway because the line that declares it is skipped at runtime.

@craigphicks
Copy link

craigphicks commented Dec 19, 2023

"correlates with" does not mean "equates to". So the counter example is not proof that "correlates with" is not an accurate description. Correlates only means "is not independent of".

@craigphicks
Copy link

craigphicks commented Dec 19, 2023

For what it is worth if the branch is always true:

function requestAnimationFrame(f:()=>void): void;
(() => {
  if (true) {
    requestAnimationFrame(() => console.log(foo));
    return;
  }
  const foo = 2; // error: Unreachable code detected.(7027)
})();

true make the CFA treat the declaration of foo as unreachable. Math.abs(1) === 1, and for that matter 1===1 don't.

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Dec 19, 2023

Not in this case; because the if() block always terminates, this can never be valid (no matter when the callback runs, the variable can never have been declared.

This isn't a useful hypothetical. If it "can never be valid" then no one would write the if in the first place. If Math.abs(1) were -1 and requestAnimationFrame waited until the next event loop tick to invoke its argument, then the code would be valid. The point is that TypeScript doesn't know either of those facts and thus can't conclude that it's invalid.

@fatcerberus
Copy link

fatcerberus commented Dec 19, 2023

@RyanCavanaugh It is true in this case that, if requestAnimationFrame is called, then the const will never be initialized (because the function returns early)β€”regardless of how many frames it takes to call the callback or the exact condition in the if.

@craigphicks
Copy link

craigphicks commented Dec 19, 2023

(because the function returns early)

if the CFA chooses to recognize the branch condition as always true.

@fatcerberus
Copy link

No, that’s irrelevant - even if the branch condition is only sometimes true, calling requestAnimationFrame always corresponds to an early return.

@craigphicks
Copy link

craigphicks commented Dec 19, 2023

No, that’s irrelevant - even if the branch condition is only sometimes true, calling requestAnimationFrame always corresponds to an early return.

  • If the branch is not called, foo is initialized.

  • If the branch is called, foo is never initialized, and the CFA could reliably conclude that accessing foo is an error - IN THIS CASE.

That's seems true. However, because of #11498, for the general case, const declarations are hoisted. So #11489 is the crux here. By hoisting, the CFA can safely assume reference by (async) functions at the top scope and any subscopes, early returns or no. To allow for this case, it would have to know otherwise.

@fatcerberus
Copy link

Yes, I’m fully aware - I was specifically arguing against Ryan’s assertion that this code could ever be valid as written (it can’t).

@craigphicks
Copy link

function requestAnimationFrame(f:()=>void){ f(); }
(() => {
  let maybe = true;
  if (maybe) {
    requestAnimationFrame(f)
    return;
  }
  const foo = 2; 
  function f(){ console.log(foo)};
})();

Just to issue clarity, the above particular case produces a runtime error, as expected.

[ERR]: "Executed JavaScript Failed:" 
[ERR]: can't access lexical declaration 'foo' before initialization 

While this done doesn't:

(() => {
  function f(){ console.log(foo)};
  setTimeout(() => f()), 0); 
  const foo = 2; 
})();

Any program calling requestAnimationFrame is probably not purely synchronous, so the worst case would have to be assumed.

@typescript-bot
Copy link
Collaborator

This issue has been marked as "Duplicate" and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@typescript-bot typescript-bot closed this as not planned Won't fix, can't repro, duplicate, stale Dec 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

5 participants