Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

feat(40750): Refactoring to add inferred return type annotation to a function#41052

Merged
sandersn merged 1 commit into
microsoft:mastermicrosoft/TypeScript:masterfrom
a-tarasyuk:feat/40750a-tarasyuk/TypeScript:feat/40750Copy head branch name to clipboard
Nov 4, 2020
Merged

feat(40750): Refactoring to add inferred return type annotation to a function#41052
sandersn merged 1 commit into
microsoft:mastermicrosoft/TypeScript:masterfrom
a-tarasyuk:feat/40750a-tarasyuk/TypeScript:feat/40750Copy head branch name to clipboard

Conversation

@a-tarasyuk

Copy link
Copy Markdown
Contributor

Fixes #40750

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Oct 12, 2020

@sandersn sandersn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good. I think this is a useful refactoring. There is a more general, less useful refactor that adds a type annotation on any declaration that could have one, but doesn't need to. I don't think this PR should add that (1) because it's less useful (2) I can't remember how to make a single refactoring produce multiple fixes -- it may not be possible.

@jessetrinity can you take a look too? What is your opinion on the more general refactoring?

Comment thread src/services/refactors/inferFunctionReturnType.ts Outdated
@sandersn

Copy link
Copy Markdown
Member

@gabritto you might have opinions on the question of the more general refactoring.

@sandersn

Copy link
Copy Markdown
Member

Sounds like nobody else wants to chime in, so I'll merge this after the 4.1 RC goes out.

@jessetrinity jessetrinity self-assigned this Oct 28, 2020

@jessetrinity jessetrinity left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perhaps there should be some tests with multiple signatures:

// bad code - there should still be a test so that we at least know what the refactor does in this case, if doing nothing is too complicated.
function f(x: number) {
    return 1;
}
function f(x: number) {
    return "1";
}
// good code (?) - grabbing the first signature will add "number" as the return type, but we probably wanted "number | string"
function f(x: number): number;
function f(x: string): string;
function f(x: string | number) {
  return typeof(x) === "string" ? "a" : 1;
}

@a-tarasyuk a-tarasyuk force-pushed the feat/40750 branch 2 times, most recently from 6882451 to e021e67 Compare November 3, 2020 19:07
@sandersn sandersn merged commit 0904865 into microsoft:master Nov 4, 2020
@microsoft microsoft locked as resolved and limited conversation to collaborators Oct 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

For Uncommitted Bug PR for untriaged, rejected, closed or missing bug

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Refactoring to add inferred return type annotation to a function

4 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.