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 boolean expressions (ts2774) #46051

Open
5 tasks done
soffes opened this issue Sep 24, 2021 · 1 comment
Open
5 tasks done

Apply uncalled function checks to boolean expressions (ts2774) #46051

soffes opened this issue Sep 24, 2021 · 1 comment
Labels
Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript

Comments

@soffes
Copy link

soffes commented Sep 24, 2021

Suggestion

Similar to #36048 (implemented in #36402), it would be great if uncalled function checks were applied to expressions like they are for if statements and ternaries.

export default class Range {
  lowerBound: number
  upperBound: number

  constructor(lowerBound: number, upperBound: number) {
    this.lowerBound = lowerBound
    this.upperBound = upperBound
  }

  isEmpty(): boolean {
    return this.lowerBound == this.upperBound
  }

  overlaps(other: Range): boolean {
    const isDisjoint = other.upperBound <= this.lowerBound
      || this.upperBound <= other.lowerBound
      || this.isEmpty || other.isEmpty // <-- Always true without an error
    return !isDisjoint
  }
}

Playground example

🔍 Search Terms

This condition will always return true since this function is always defined. Did you mean to call it instead? ts2774

✅ Viability 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, new syntax sugar for JS, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.

⭐ Suggestion

It would be great if using an uncalled function in a boolean expression was an error.

📃 Motivating Example

It would be great if this was true:

// Error
const bad = false || new Range().isEmpty

// Works!
const good = false || new Range().isEmpty()

💻 Use Cases

Recently spent awhile debugging some code that was always returning true before realizing I was checking the truthyness of an uncalled function.

@soffes soffes changed the title Apply uncalled function checks to boolean expressions Apply uncalled function checks to boolean expressions (ts2774) Sep 24, 2021
@andrewbranch
Copy link
Member

So, we can’t quite do exactly what this issue proposes, because the type of the logical expression ends up including the function itself, which only looks like a mistake if it never gets called. If the variable gets carried forward or passed around, it can be difficult or impossible to decide if it did ever get called. The two things that makes it possible to do these checks for if statements and ternaries are

  1. Typically, the only purpose of the expression being tested for truthiness is to test it for truthiness, so we know if it has constant truthiness, something is suspicious. Contrast that to assigning a logical expression to a variable—that variable could be used for many purposes down the line, including but not limited to testing for truthiness.
  2. In an if statement or ternary, we can increase our confidence that the suspicious code is really suspicious by checking the true branch of the conditional to see if the function is ever called. If it is called, like foo ? foo() : undefined, the user is clearly performing defensive checks, which might appear overly defensive to our type system, but sometimes types are wrong and something can be undefined when we think it can’t. Contrast this to assigning a logical expression to a variable, where we’d have to analyze everything in the same scope as the variable to find its uses. The if/ternary usually presents us a small (and definitely file-local) boundary where we have to check. If a variable for a logical expression appears in the global scope, it could be used anywhere in the whole program.

I think the place where you could maybe argue that we go wrong is not at const isDisjoint = ..., but at return !isDisjoint. The type of isDisjoint is true | (() => boolean), both sides of which are truthy, and so we could make the determination that this function always returns false, and perhaps say that’s suspicious. But on the other hand, it makes total sense for some functions/methods to return constant values, so that analysis alone is insufficient.

The example is super compelling, but I’m having trouble coming up with anything we could do that wouldn’t severely break something else.

@andrewbranch andrewbranch added Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript labels Oct 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

2 participants