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

Apply uncalled function checks to ternaries #36048

Closed
ssalbdivad opened this issue Jan 7, 2020 · 5 comments · Fixed by #36402
Closed

Apply uncalled function checks to ternaries #36048

ssalbdivad opened this issue Jan 7, 2020 · 5 comments · Fixed by #36402
Assignees
Labels
Breaking Change Would introduce errors in existing code Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Help Wanted You can do this Suggestion An idea for TypeScript Update Docs on Next Release Indicates that this PR affects docs

Comments

@ssalbdivad
Copy link

Search Terms

ts2774 ternary condition error function called uncalled

Suggestion

TS2774 (implemented in #32802), i.e. the "This condition will always return true since the function is always defined. Did you mean to call it instead?" error, provides a helpful hint for developers referencing a function in an if statement without calling it. However, the same check doesn't apply to ternaries, which is missing an opportunity to save some debug time and counterintuitive for devs who know about TS2774. I propose reusing TS2774 for developers who forget to call a function in a ternary.

Examples

Now:

const isString = (value: unknown) => typeof value === "string"

// Error (TS2774)
if (isString) {
}
// No error
isString ? true : false

If this suggestion is adopted:

const isString = (value: unknown) => typeof value === "string"

// Error (TS2774)
if (isString) {
}
// Error(TS2774)
isString ? true : false

Checklist

My suggestion meets these guidelines:

  • [✔️] This wouldn't be a breaking change in existing TypeScript/JavaScript code
  • [✔️] This wouldn't change the runtime behavior of existing JavaScript code
  • [✔️] This could be implemented without emitting different JS based on the types of the expressions
  • [✔️] This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, etc.)
  • [✔️] This feature would agree with the rest of TypeScript's Design Goals.
@RyanCavanaugh RyanCavanaugh added Experience Enhancement Noncontroversial enhancements Suggestion An idea for TypeScript labels Jan 7, 2020
@RyanCavanaugh
Copy link
Member

Seems reasonable; we'd have to assess for breakage but I suspect this would turn out well

@dragomirtitian
Copy link
Contributor

@RyanCavanaugh Are you taking PRs on this one?

@DanielRosenwasser DanielRosenwasser changed the title Apply TS2774 to ternaries Apply uncalled function checks to ternaries Jan 21, 2020
@DanielRosenwasser DanielRosenwasser added Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Help Wanted You can do this labels Jan 21, 2020
@DanielRosenwasser
Copy link
Member

Yes

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Jan 28, 2020

Just assigning @sandersn so that we can follow up on the PR for 3.9 sooner rather than later.

@DanielRosenwasser DanielRosenwasser added the Breaking Change Would introduce errors in existing code label Feb 20, 2020
@DanielRosenwasser DanielRosenwasser added Update Docs on Next Release Indicates that this PR affects docs and removed Experience Enhancement Noncontroversial enhancements Experimentation Needed Someone needs to try this out to see what happens labels Feb 20, 2020
@DanielRosenwasser
Copy link
Member

Thanks @a-tarasyuk!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Would introduce errors in existing code Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Help Wanted You can do this Suggestion An idea for TypeScript Update Docs on Next Release Indicates that this PR affects docs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants