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

Unreachable returns badly break return type inference #55437

Open
rotu opened this issue Aug 20, 2023 · 7 comments · May be fixed by #55601
Open

Unreachable returns badly break return type inference #55437

rotu opened this issue Aug 20, 2023 · 7 comments · May be fixed by #55601
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript

Comments

@rotu
Copy link

rotu commented Aug 20, 2023

🔎 Search Terms

unreachable any return

🕗 Version & Regression Information

  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about let

⏯ Playground Link

https://www.typescriptlang.org/play?target=7&jsx=0&ts=5.2.0-beta&allowUnreachableCode=true#code/GYVwdgxgLglg9mABMAFASgN6IPTcQBwCc58BTQgGwE9FDThy6ATRAQwGdEwQBbAI3IAoAJAVSURAA8A3FMQBeRAEZZdKCEJIZI3IgBypSRIowwpREzilOYOBNbAG0WuI1IoVMolMM6kUgBcIpIKiABEwHBwYYIAvoKgkLAIiADm6Fi6PoykLBxsYFQiYhIycooqLuqaUtI6eAZGiCZmiADWMBQUnGpuiB5e2X4QgSK9NdrxidDwSAAWGTh4Q8xsnKyFY641KvX6hsam5h1dPdvunuZDpP5BouKIVLI0Farnj3XxQA

💻 Code

function f(){ // properly referred as number
	let x; x = 1; return x;
	// Next line does not affect return type inference:
	x = "foo"
}
function g(){ // inferred as any
	let x; x = 1; return x;
	// Next line kills return type inference:
	return x;
}
function h(){ // inferred as any
	return 1;
	// Next line kills return type inference:
	let y; y = 1; return y;
}

🙁 Actual behavior

The types are inferred as:

declare function f(): number;
declare function g(): any;
declare function h(): any;

The case of f demonstrates that return types based on assignment to mutable variables can work. The unreachable assignment to x after return has no effect on the return type.

In the case of g, the return type should be unaffected - x is already inferred to be number and an extra return does not change the type of x.

In the case of h, the return type should be unaffected as well. The second return can be inferred to be number.

🙂 Expected behavior

The types should be inferred as:

declare function f(): number;
declare function g(): number;
declare function h(): number;

Either unreachable return statements should not affect return type or return should not stop type inference.

Also, the code should be flagged by the noImplicitAny rule.

Additional information about the issue

possibly related to #40393
likely related to #26914

Disabling allowUnreachableCode does not alleviate these issues.

@fatcerberus
Copy link

Almost certainly this is due to #26914, though I'd agree that any return statement which is known by the compiler to be unreachable shouldn't play into return type inference in the first place.

@rotu
Copy link
Author

rotu commented Aug 20, 2023

Almost certainly this is due to #26914, though I'd agree that any return statement which is known by the compiler to be unreachable shouldn't play into return type inference in the first place.

Agreed. The type checker should infer based on the code I wrote - not the code it thinks I should have written. It's doubly unfortunate that type inference here degrades to any instead of unknown!

@MartinJohns
Copy link
Contributor

Also related: #51800

When you have unreachable code it trips up the type checker. Solution is to not have unreachable code.

@fatcerberus
Copy link

The type checker should infer based on the code I wrote - not the code it thinks I should have written.

Actually the problem is the opposite - the code it “thinks you should have written” doesn’t include the unreachable bit (hence why CFA doesn’t work there), but it’s trying to infer a return type from it anyway. If it inferred types only from what it thought you should have written then you’d have the desired behavior already. 😉

@rotu
Copy link
Author

rotu commented Aug 20, 2023

@MartinJohns I mean I guess you could also say “Solution is to write code that so obviously correct you don’t need a type checker” :-p. Even so, reachability can change due to changes in upstream dependencies, environment, or feature flags.

Even if CFA does ignore unreachable code and unreachable return statements do contribute to inferred return type, the motivating example code should be and are not flagged by the noImplicitAny checker.

@ahejlsberg
Copy link
Member

ahejlsberg commented Aug 21, 2023

The real issue here is our treatment of "evolving any" types in unreachable code (support for evolving any types was introduced in TS 2.1, see here for details). When a variable with an evolving any type is referenced in unreachable code, we currently assume it has type any. Since the reference in question is never actually evaluated, this isn't unreasonable. However, an argument could be made that we should assume type never which more accurately reflects the situation. This would ensure that unreachable return statements that reference evolving any variables don't poison return type inference (whereas any types cause unions to become any, never types have no effect in unions). I'm going to label this issue a suggestion to do so.

@ahejlsberg ahejlsberg added the Suggestion An idea for TypeScript label Aug 21, 2023
@RyanCavanaugh RyanCavanaugh added the Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature label Aug 21, 2023
@Andarist
Copy link
Contributor

The behavior related to unreachable code isn't limited to auto types. Similar-ish thing happens to variables with declared types:

// inferred: function f(): string | number | boolean
function f() {
  let x: string | number | boolean;
  x = 1;
  return x;

  x = "foo";
  return x;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants