[WIP] Support condition always true in prefix unary expression #60068
+100
−9
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
typescript-bot
added
the
For Milestone Bug
…lways-true-in-prefix-unary-expression
5 tasks
tjenkinson
changed the title
|
@typescript-bot test it |
|
Hey @gabritto, the results of running the DT tests are ready. There were interesting changes: Branch only errors:Package: js-fixtures Package: xdate Package: activex-excel Package: morgan Package: yandex-maps Package: bluebird |
|
@gabritto Here are the results of running the user tests with tsc comparing Something interesting changed - please have a look. Details
|
|
@gabritto Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@gabritto Here are the results of running the top 400 repos with tsc comparing Something interesting changed - please have a look. Details
|
|
@tjenkinson I think both the function and promise cases will require some extra work for avoiding most of the false errors. Let me know if you'd still like to keep working on it, I think it's a very good feature since we had at least 3 bugs detected in the extended tests. |
|
hey @gabritto thanks for looking through them super promising it found some actual issues. Do you think we'll need to handle cases like this one where the usage happens later and is not part of the Happy to continue digging but I think that will be a bit more complex looking at how things are structured at the moment as |
|
Actually maybe this is a different kind of check. If there's an
I'm not sure if (2) will happen much in the wild so maybe the 1st will be enough |
|
Also not sure https://github.com/microsoft/vscode/blob/f71675cbd9ba6aef444321cbe5faf8dd81340e3b/extensions/emmet/src/test/abbreviationAction.test.ts#L223 is a false error? Looking at the types |
I think you are right in this case. However, I think we should still support that case to avoid false errors in code written in a similar way. |
tjenkinson
force-pushed
the
support-condition-always-true-in-prefix-unary-expression
branch
5 times, most recently
from
d2604e2 to
8715473
Compare
tjenkinson
force-pushed
the
support-condition-always-true-in-prefix-unary-expression
branch
from
4e7fe97 to
8f8b45c
Compare
tjenkinson
commented
Dec 30, 2024
|
|
||
| function next() { | ||
| let task = tasks[index] | ||
| if (!task) { |
There was a problem hiding this comment.
Looks like might also have to bail if the source is accessing something from an array 🤔
There was a problem hiding this comment.
This case looks like a bug in your PR code: we should not be erroring because task is used inside the else block.
|
Hey @gabritto I've pushed some changes so that now it should only apply if there's a single Still need to add tests, but would be great if you could run the run the extended tests again and we could see if this reduces the false positives? Also might need to add some logic to bail if it's accessing an item from an array |
|
@typescript-bot test it |
instead of just `if (!object.fnOrPromise)`
|
@gabritto Here are the results of running the user tests with tsc comparing Something interesting changed - please have a look. Details
|
|
Hey @gabritto, the results of running the DT tests are ready. There were interesting changes: Branch only errors:Package: morgan Package: activex-excel Package: activex-shell Package: xdate Package: js-fixtures Package: yandex-maps |
|
@gabritto Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@gabritto Here are the results of running the top 400 repos with tsc comparing Something interesting changed - please have a look. Details
|
|
@gabritto Here are some more interesting changes from running the top 400 repos suite Details
|
gabritto
reviewed
Jan 3, 2025
| @@ -44588,8 +44593,10 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| return; | |||
| } | |||
|
|
|||
| const isUsedInBody = inverted && isIfStatement(condExpr.parent) ? testedSymbol && isSymbolUsedInConditionBody(location, condExpr.parent.parent, testedNode, testedSymbol, /*checkForAssignment*/ true) : testedSymbol && body && isSymbolUsedInConditionBody(condExpr, body, testedNode, testedSymbol, /*checkForAssignment*/ false); | |||
There was a problem hiding this comment.
Why do we call isSymbolUsedInConditionBody with location instead of condExpr, in the first case?
|
|
||
| function next() { | ||
| let task = tasks[index] | ||
| if (!task) { |
There was a problem hiding this comment.
This case looks like a bug in your PR code: we should not be erroring because task is used inside the else block.
| @@ -44606,15 +44613,23 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| } | |||
| } | |||
|
|
|||
| function isSymbolUsedInConditionBody(expr: Expression, body: Statement | Expression, testedNode: Node, testedSymbol: Symbol): boolean { | |||
| function isSymbolUsedInConditionBody(expr: Expression, body: Statement | Expression | Node, testedNode: Node, testedSymbol: Symbol, checkForAssignment: boolean): boolean { | |||
There was a problem hiding this comment.
What's the idea here behind checkForAssignment? Why were the previous checks being done here in isSymbolUsedInConditionBody not enough?
| const location = isLogicalOrCoalescingBinaryExpression(condExpr) ? skipParentheses(condExpr.right) : condExpr; | ||
| let location = condExpr; | ||
| let inverted = false; | ||
| if (isPrefixUnaryExpression(location)) { |
There was a problem hiding this comment.
I think you also need to check that the operator is the exclamation token. Otherwise location could be another kind of expression, e.g. ++a.
| let inverted = false; | ||
| if (isPrefixUnaryExpression(location)) { | ||
| location = skipParentheses(location.operand); | ||
| inverted = true; |
There was a problem hiding this comment.
Another thing to consider (and probably add a test) is that we can have an expression that flips between the negative and positive case, if we have nested negation expressions, e.g.: if (a && !(b || !c)) ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Opening as draft for now so we can run the extended tests and see how much breakage there is. See #46140 (comment)
cc @gabritto
Fixes #46140