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

Flag non-nullable functions in if statements as errors (tree walk version) #33178

Merged
merged 5 commits into from
Sep 25, 2019

Conversation

jwbay
Copy link
Contributor

@jwbay jwbay commented Aug 31, 2019

This is an alternate version of #32802 -- see that one for context.

This version attempts to prevent false positives by doing a syntax walk to see if the checked function is used, as opposed to relying on a heuristic where the function returns a boolean. The first commit is the same for both PR; the second adds filtering that differs.

Opened as a separate PR by request (cc @RyanCavanaugh)

Notably, this change flagged a couple things in the compiler itself. Commented below for both.

@@ -1362,18 +1362,16 @@ namespace ts {
// try to verify results of module resolution
for (const { oldFile: oldSourceFile, newFile: newSourceFile } of modifiedSourceFiles) {
const newSourceFilePath = getNormalizedAbsolutePath(newSourceFile.originalFileName, currentDirectory);
if (resolveModuleNamesWorker) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like resolveModuleNamesWorker is always assigned above, so this was a dead check

@@ -1653,7 +1653,7 @@ namespace ts {
*/
export function compose<T>(...args: ((t: T) => T)[]): (t: T) => T;
export function compose<T>(a: (t: T) => T, b: (t: T) => T, c: (t: T) => T, d: (t: T) => T, e: (t: T) => T): (t: T) => T {
if (e) {
if (!!e) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a pretty good example of what TS would now consider suspicious-enough code to error on. Making e optional would also have fixed this.

@orta
Copy link
Contributor

orta commented Sep 11, 2019

@typescript-bot test this
@typescript-bot user test this
@typescript-bot run dt

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 11, 2019

Heya @orta, I've started to run the extended test suite on this PR at 3c9e338. You can monitor the build here. It should now contribute to this PR's status checks.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 11, 2019

Heya @orta, I've started to run the parallelized community code test suite on this PR at 3c9e338. You can monitor the build here. It should now contribute to this PR's status checks.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 11, 2019

Heya @orta, I've started to run the parallelized Definitely Typed test suite on this PR at 3c9e338. You can monitor the build here. It should now contribute to this PR's status checks.

@typescript-bot
Copy link
Collaborator

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

@RyanCavanaugh
Copy link
Member

The RWC diff is unrelated.

The User Test Suite diff is a false positive, but not a bad one: https://github.com/npm/cli/blob/latest/test/tap/check-permissions.js#L27

@RyanCavanaugh
Copy link
Member

@typescript-bot perf test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 11, 2019

Heya @RyanCavanaugh, I've started to run the perf test suite on this PR at 3c9e338. You can monitor the build here. It should now contribute to this PR's status checks.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

@RyanCavanaugh
The results of the perf run you requested are in!

Here they are:

Comparison Report - master..33178

Metric master 33178 Delta Best Worst
Angular - node (v12.1.0, x64)
Memory used 331,291k (± 0.02%) 325,798k (± 0.04%) -5,493k (- 1.66%) 325,266k 325,968k
Parse Time 1.56s (± 0.54%) 1.48s (± 0.45%) -0.08s (- 4.87%) 1.47s 1.50s
Bind Time 0.78s (± 0.67%) 0.75s (± 0.63%) -0.03s (- 3.34%) 0.75s 0.77s
Check Time 4.25s (± 0.60%) 4.19s (± 0.47%) -0.06s (- 1.48%) 4.14s 4.24s
Emit Time 5.22s (± 0.59%) 5.23s (± 0.48%) +0.01s (+ 0.21%) 5.17s 5.28s
Total Time 11.82s (± 0.40%) 11.66s (± 0.30%) -0.16s (- 1.32%) 11.55s 11.74s
Monaco - node (v12.1.0, x64)
Memory used 346,015k (± 0.04%) 345,898k (± 0.03%) -116k (- 0.03%) 345,732k 346,088k
Parse Time 1.22s (± 0.60%) 1.22s (± 0.91%) +0.00s (+ 0.16%) 1.20s 1.24s
Bind Time 0.68s (± 0.74%) 0.68s (± 1.47%) +0.01s (+ 1.33%) 0.67s 0.72s
Check Time 4.25s (± 0.51%) 4.26s (± 0.67%) +0.01s (+ 0.26%) 4.20s 4.34s
Emit Time 2.88s (± 1.37%) 2.86s (± 0.68%) -0.02s (- 0.70%) 2.81s 2.91s
Total Time 9.02s (± 0.58%) 9.03s (± 0.31%) +0.00s (+ 0.03%) 8.98s 9.11s
TFS - node (v12.1.0, x64)
Memory used 301,407k (± 0.02%) 301,420k (± 0.02%) +13k (+ 0.00%) 301,314k 301,503k
Parse Time 0.95s (± 0.98%) 0.94s (± 0.72%) -0.00s (- 0.42%) 0.93s 0.96s
Bind Time 0.63s (± 1.03%) 0.62s (± 1.33%) -0.00s (- 0.64%) 0.61s 0.65s
Check Time 3.86s (± 0.41%) 3.85s (± 0.54%) -0.00s (- 0.13%) 3.79s 3.90s
Emit Time 2.97s (± 0.46%) 2.96s (± 0.49%) -0.00s (- 0.13%) 2.93s 3.00s
Total Time 8.40s (± 0.24%) 8.38s (± 0.38%) -0.02s (- 0.18%) 8.33s 8.48s
Angular - node (v8.9.0, x64)
Memory used 350,095k (± 0.02%) 344,535k (± 0.01%) -5,559k (- 1.59%) 344,452k 344,644k
Parse Time 2.09s (± 0.42%) 1.99s (± 0.43%) -0.10s (- 4.83%) 1.97s 2.01s
Bind Time 0.84s (± 0.57%) 0.82s (± 0.79%) -0.02s (- 2.27%) 0.80s 0.83s
Check Time 5.12s (± 0.59%) 5.04s (± 0.75%) -0.08s (- 1.52%) 4.97s 5.12s
Emit Time 5.97s (± 0.68%) 6.15s (± 0.63%) +0.18s (+ 3.00%) 6.08s 6.25s
Total Time 14.02s (± 0.37%) 14.00s (± 0.48%) -0.02s (- 0.14%) 13.89s 14.16s
Monaco - node (v8.9.0, x64)
Memory used 363,686k (± 0.01%) 363,687k (± 0.01%) +2k (+ 0.00%) 363,550k 363,794k
Parse Time 1.56s (± 0.52%) 1.57s (± 0.32%) +0.00s (+ 0.32%) 1.55s 1.57s
Bind Time 0.89s (± 1.12%) 0.89s (± 1.01%) -0.00s (- 0.45%) 0.86s 0.90s
Check Time 5.11s (± 1.30%) 5.09s (± 1.22%) -0.02s (- 0.47%) 5.00s 5.26s
Emit Time 3.14s (± 4.52%) 3.25s (± 3.67%) +0.11s (+ 3.44%) 2.90s 3.39s
Total Time 10.70s (± 0.89%) 10.79s (± 0.68%) +0.09s (+ 0.82%) 10.60s 10.98s
TFS - node (v8.9.0, x64)
Memory used 317,706k (± 0.01%) 317,645k (± 0.01%) -60k (- 0.02%) 317,584k 317,721k
Parse Time 1.26s (± 0.53%) 1.25s (± 0.54%) -0.01s (- 0.71%) 1.24s 1.27s
Bind Time 0.70s (± 5.02%) 0.68s (± 3.63%) -0.02s (- 2.99%) 0.66s 0.78s
Check Time 4.45s (± 1.36%) 4.49s (± 0.87%) +0.04s (+ 0.83%) 4.36s 4.54s
Emit Time 3.09s (± 0.60%) 3.08s (± 0.87%) -0.01s (- 0.45%) 3.00s 3.12s
Total Time 9.50s (± 0.52%) 9.50s (± 0.44%) -0.00s (- 0.03%) 9.37s 9.57s
Angular - node (v8.9.0, x86)
Memory used 198,187k (± 0.02%) 195,180k (± 0.02%) -3,007k (- 1.52%) 195,060k 195,274k
Parse Time 2.03s (± 0.47%) 1.92s (± 0.47%) -0.11s (- 5.28%) 1.90s 1.94s
Bind Time 0.95s (± 1.16%) 0.94s (± 0.43%) -0.01s (- 1.26%) 0.93s 0.95s
Check Time 4.66s (± 0.45%) 4.57s (± 0.52%) -0.09s (- 1.85%) 4.51s 4.62s
Emit Time 5.70s (± 0.56%) 5.82s (± 1.76%) +0.11s (+ 2.00%) 5.58s 6.08s
Total Time 13.35s (± 0.34%) 13.25s (± 0.78%) -0.10s (- 0.73%) 13.07s 13.54s
Monaco - node (v8.9.0, x86)
Memory used 203,221k (± 0.02%) 203,189k (± 0.03%) -33k (- 0.02%) 203,062k 203,346k
Parse Time 1.62s (± 0.78%) 1.61s (± 0.69%) -0.01s (- 0.37%) 1.60s 1.64s
Bind Time 0.72s (± 0.77%) 0.72s (± 0.65%) +0.00s (+ 0.28%) 0.71s 0.73s
Check Time 4.89s (± 0.59%) 4.86s (± 0.58%) -0.03s (- 0.70%) 4.80s 4.94s
Emit Time 3.19s (± 0.73%) 3.15s (± 0.64%) -0.03s (- 1.04%) 3.12s 3.22s
Total Time 10.41s (± 0.50%) 10.34s (± 0.38%) -0.07s (- 0.67%) 10.26s 10.46s
TFS - node (v8.9.0, x86)
Memory used 178,509k (± 0.01%) 178,518k (± 0.01%) +10k (+ 0.01%) 178,460k 178,582k
Parse Time 1.31s (± 0.72%) 1.32s (± 0.88%) +0.00s (+ 0.15%) 1.30s 1.34s
Bind Time 0.65s (± 0.77%) 0.64s (± 1.75%) -0.00s (- 0.62%) 0.63s 0.68s
Check Time 4.31s (± 0.61%) 4.28s (± 0.53%) -0.03s (- 0.79%) 4.23s 4.32s
Emit Time 2.88s (± 1.00%) 2.86s (± 1.53%) -0.02s (- 0.69%) 2.77s 2.96s
Total Time 9.15s (± 0.38%) 9.10s (± 0.60%) -0.05s (- 0.58%) 8.97s 9.24s
Angular - node (v9.0.0, x64)
Memory used 349,762k (± 0.03%) 344,113k (± 0.02%) -5,648k (- 1.61%) 343,976k 344,249k
Parse Time 1.82s (± 0.45%) 1.72s (± 0.64%) -0.10s (- 5.49%) 1.69s 1.74s
Bind Time 0.78s (± 0.77%) 0.77s (± 0.64%) -0.01s (- 1.16%) 0.76s 0.78s
Check Time 4.88s (± 0.65%) 4.76s (± 0.38%) -0.12s (- 2.54%) 4.73s 4.79s
Emit Time 5.78s (± 1.32%) 5.70s (± 1.63%) -0.08s (- 1.42%) 5.52s 5.89s
Total Time 13.26s (± 0.68%) 12.95s (± 0.86%) -0.31s (- 2.35%) 12.74s 13.16s
Monaco - node (v9.0.0, x64)
Memory used 363,480k (± 0.02%) 363,441k (± 0.02%) -40k (- 0.01%) 363,284k 363,702k
Parse Time 1.32s (± 0.61%) 1.32s (± 0.46%) -0.00s (- 0.23%) 1.30s 1.33s
Bind Time 0.83s (± 1.49%) 0.84s (± 1.13%) +0.01s (+ 1.44%) 0.81s 0.86s
Check Time 5.07s (± 1.60%) 4.92s (± 0.89%) -0.15s (- 2.92%) 4.86s 5.08s
Emit Time 3.08s (± 5.47%) 3.30s (± 3.09%) +0.22s (+ 7.13%) 2.90s 3.40s
Total Time 10.30s (± 1.04%) 10.38s (± 0.74%) +0.08s (+ 0.81%) 10.10s 10.50s
TFS - node (v9.0.0, x64)
Memory used 317,468k (± 0.01%) 317,422k (± 0.01%) -46k (- 0.01%) 317,296k 317,557k
Parse Time 1.04s (± 0.50%) 1.05s (± 0.35%) +0.01s (+ 0.48%) 1.04s 1.05s
Bind Time 0.62s (± 0.72%) 0.62s (± 0.79%) +0.00s (+ 0.32%) 0.61s 0.63s
Check Time 4.38s (± 0.57%) 4.38s (± 0.42%) -0.00s (- 0.02%) 4.35s 4.42s
Emit Time 3.20s (± 0.78%) 3.19s (± 0.41%) -0.00s (- 0.13%) 3.17s 3.22s
Total Time 9.24s (± 0.51%) 9.24s (± 0.27%) -0.00s (- 0.01%) 9.19s 9.30s
Angular - node (v9.0.0, x86)
Memory used 198,329k (± 0.02%) 195,251k (± 0.04%) -3,079k (- 1.55%) 195,113k 195,376k
Parse Time 1.73s (± 0.58%) 1.63s (± 0.38%) -0.10s (- 5.99%) 1.62s 1.65s
Bind Time 0.90s (± 0.56%) 0.89s (± 0.87%) -0.00s (- 0.45%) 0.87s 0.91s
Check Time 4.35s (± 0.50%) 4.25s (± 0.30%) -0.10s (- 2.25%) 4.23s 4.28s
Emit Time 5.53s (± 0.69%) 5.51s (± 0.28%) -0.03s (- 0.49%) 5.48s 5.56s
Total Time 12.51s (± 0.37%) 12.28s (± 0.18%) -0.23s (- 1.83%) 12.24s 12.33s
Monaco - node (v9.0.0, x86)
Memory used 203,274k (± 0.02%) 203,274k (± 0.03%) +1k (+ 0.00%) 203,155k 203,372k
Parse Time 1.36s (± 0.74%) 1.35s (± 0.52%) -0.01s (- 0.44%) 1.33s 1.36s
Bind Time 0.64s (± 1.18%) 0.65s (± 0.73%) +0.00s (+ 0.31%) 0.64s 0.66s
Check Time 4.72s (± 0.48%) 4.69s (± 0.49%) -0.03s (- 0.61%) 4.64s 4.74s
Emit Time 3.10s (± 1.07%) 3.10s (± 0.52%) -0.00s (- 0.03%) 3.07s 3.15s
Total Time 9.82s (± 0.48%) 9.79s (± 0.31%) -0.03s (- 0.31%) 9.72s 9.85s
TFS - node (v9.0.0, x86)
Memory used 178,596k (± 0.02%) 178,585k (± 0.02%) -11k (- 0.01%) 178,504k 178,673k
Parse Time 1.07s (± 0.54%) 1.07s (± 0.65%) +0.00s (+ 0.37%) 1.05s 1.08s
Bind Time 0.58s (± 0.57%) 0.58s (± 0.86%) -0.00s (- 0.69%) 0.57s 0.59s
Check Time 4.16s (± 0.55%) 4.15s (± 1.17%) -0.01s (- 0.14%) 4.06s 4.30s
Emit Time 2.79s (± 1.85%) 2.79s (± 0.99%) -0.00s (- 0.07%) 2.71s 2.85s
Total Time 8.59s (± 0.64%) 8.59s (± 0.86%) -0.01s (- 0.08%) 8.45s 8.81s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-161-generic
Architecturex64
Available Memory16 GB
Available Memory10 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v12.1.0, x64)
  • node (v8.9.0, x64)
  • node (v8.9.0, x86)
  • node (v9.0.0, x64)
  • node (v9.0.0, x86)
Scenarios
  • Angular - node (v12.1.0, x64)
  • Angular - node (v8.9.0, x64)
  • Angular - node (v8.9.0, x86)
  • Angular - node (v9.0.0, x64)
  • Angular - node (v9.0.0, x86)
  • Monaco - node (v12.1.0, x64)
  • Monaco - node (v8.9.0, x64)
  • Monaco - node (v8.9.0, x86)
  • Monaco - node (v9.0.0, x64)
  • Monaco - node (v9.0.0, x86)
  • TFS - node (v12.1.0, x64)
  • TFS - node (v8.9.0, x64)
  • TFS - node (v8.9.0, x86)
  • TFS - node (v9.0.0, x64)
  • TFS - node (v9.0.0, x86)
Benchmark Name Iterations
Current 33178 10
Baseline master 10

@orta
Copy link
Contributor

orta commented Sep 23, 2019

Hrm, none of these builds have accessible logs.

I bet it's likely that community projects will need changes like:

-        if (e) {
+       if (!!e) {

Will re-run and take a look.

@typescript-bot test this
@typescript-bot user test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 23, 2019

Heya @orta, I've started to run the extended test suite on this PR at 3c9e338. You can monitor the build here. It should now contribute to this PR's status checks.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 23, 2019

Heya @orta, I've started to run the parallelized community code test suite on this PR at 3c9e338. You can monitor the build here. It should now contribute to this PR's status checks.

@typescript-bot
Copy link
Collaborator

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

@orta
Copy link
Contributor

orta commented Sep 23, 2019

OK @jwbay - to get this mergeable you'll need to accept jwbay#3

jwbay and others added 2 commits September 24, 2019 19:50
…nonNullableCallSignaturesTreeWalk

🤖 User test baselines have changed for nonNullableCallSignaturesTreeWalk
@orta orta added the Update Docs on Next Release Indicates that this PR affects docs label Sep 25, 2019
@orta
Copy link
Contributor

orta commented Sep 25, 2019

OK, perf looks acceptable to me.

The idea is a good one, and I feel good about the implementation - thanks @jwbay!

@orta orta merged commit 91be368 into microsoft:master Sep 25, 2019
@DanielRosenwasser DanielRosenwasser added the Breaking Change Would introduce errors in existing code label Oct 1, 2019
@user753
Copy link

user753 commented Oct 2, 2019

@jwbay
Is there any reason why it affects only functions and not arrays, records, etc?

function foo(array: string[]) {
  if (array) {
    return true
  }
}

function bar(record: Record<string, string>) {
  if (record) {
    return true
  }
}

@RyanCavanaugh
Copy link
Member

@user753 a few major problems appeared when we tried that approach.

Array access is optimistically assumed to be in-bounds, so code like this gets incorrectly flagged as an error. The same thing happens for Map.get and a few other APIs that are assumed to "always return non-undefined values when used correctly".

const arr = [rec, rec, rec];
const el = arr[someIndex];
if (el) {
  // do something
}

Separately, many definition files were written prior to the introduction of strictNullChecks, so correct null/undefined checks on properties from those definitions would get incorrectly flagged as an error.

Ultimately the very narrow check implemented here was the only one that didn't generate a huge amount of false positives.

@user753
Copy link

user753 commented Oct 3, 2019

@RyanCavanaugh Thanks.
Do I understand correctly that It isn't a problem for functions because a function inside an array isn't a common use case?

const arr = [func, func, func];
const el = arr[someIndex];
if (el) {
  // do something
}

@RyanCavanaugh
Copy link
Member

Normally if that does happen, a call to el() appears in the body of the if, which is the additional check that this PR implements. If el *isn't` called, that'll be a false positive.

@RyanCavanaugh
Copy link
Member

@jwbay this might be my favorite check this release. https://github.com/microsoft/vscode/blob/ecba37c6ef3aab85b7a9b15afb3610bbe4eaac8c/extensions/css-language-features/server/src/cssServerMain.ts#L78

@twavv
Copy link

twavv commented Oct 6, 2019

It would be great if this was extended to functions that return promises as well.

In the announcement for 3.7 (which I'm thrilled about!!!), you include this code example.

function doAdminThing(user: User) {
    if (user.isAdministrator) {
    //  ~~~~~~~~~~~~~~~~~~~~
    // error! This condition will always return true since the function is always defined.
    //        Did you mean to call it instead?t

It would be incredible if this was also extended to raise an error when the function returns a promise and it's not awaited before using it in a condition (I've personally been bitten by this exact bug before).

class User {
    // ...
    async isAdministrator() { /* ... */ }
}
function doAdminThing(user: User) {
    if (user.isAdministrator()) {
    //  ~~~~~~~~~~~~~~~~~~~~~~
    // error! This condition will always return true since a promise is always truthy.
    //        Did you mean to await it instead?

I tried searching for issues related to this but couldn't find any (it's possible that I just missed it because there are so many issues and I'm not sure what keywords to search).

I've definitely been bitten by a bug where I did something like this:

async function passwordMatches(email: string, password: string) {
  /* return true if the password matches the result in the database */
}

if (passwordMatches("foo@bar.gov", "passw0rd")) {
  /* issue auth token */
}

@omidkrad
Copy link

Regarding Uncalled Function Checks, I think it'd be pretty safe to also error on this:

function process() {
  console.log('Processing...');
}

process;

@nebrelbug
Copy link

BTW this is breaking all of my polyfills, like checks for if (String.prototype.trimLeft) {}. Any way to disable this?

@vinothbabu
Copy link

The problem with this is, it's not allowing any of the valid ways to pass. As @nebrelbug pointed above.

if (String.prototype.trimLeft) {} //fails
if (typeof String.prototype.trimLeft === 'function') {} //fails
if ('trimLeft' in String.prototype) //fails

We had to adapt to this, which passes

// eslint-disable-next-line no-extra-boolean-cast
if (!!String.prototype.trimLeft)

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 Update Docs on Next Release Indicates that this PR affects docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.