Flag non-nullable functions in if statements as errors (tree walk version)#33178
Conversation
| // try to verify results of module resolution | ||
| for (const { oldFile: oldSourceFile, newFile: newSourceFile } of modifiedSourceFiles) { | ||
| const newSourceFilePath = getNormalizedAbsolutePath(newSourceFile.originalFileName, currentDirectory); | ||
| if (resolveModuleNamesWorker) { |
There was a problem hiding this comment.
Looks like resolveModuleNamesWorker is always assigned above, so this was a dead check
| 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) { |
There was a problem hiding this comment.
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.
|
@typescript-bot test this |
|
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
|
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 |
|
@typescript-bot perf test this |
|
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! |
|
@RyanCavanaugh Here they are:Comparison Report - master..33178
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
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 |
|
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
…nonNullableCallSignaturesTreeWalk 🤖 User test baselines have changed for nonNullableCallSignaturesTreeWalk
|
OK, perf looks acceptable to me. The idea is a good one, and I feel good about the implementation - thanks @jwbay! |
DanielRosenwasser
added
the
Breaking Change
|
@jwbay |
|
@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 const arr = [rec, rec, rec];
const el = arr[someIndex];
if (el) {
// do something
}Separately, many definition files were written prior to the introduction of Ultimately the very narrow check implemented here was the only one that didn't generate a huge amount of false positives. |
|
@RyanCavanaugh Thanks. |
|
Normally if that does happen, a call to |
|
@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 |
|
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?tIt 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("[email protected]", "passw0rd")) {
/* issue auth token */
} |
|
Regarding Uncalled Function Checks, I think it'd be pretty safe to also error on this: function process() {
console.log('Processing...');
}
process; |
|
BTW this is breaking all of my polyfills, like checks for |
|
The problem with this is, it's not allowing any of the valid ways to pass. As @nebrelbug pointed above.
We had to adapt to this, which passes
|
11 participants
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.