Avoid circular reference in this-property assignments #37827
Conversation
To do this, don't check this-property assigments that have the
this-property of the lhs appearing somewhere on the rhs:
```js
class C {
m() {
this.x = 12
this.x = this.x + this.y
}
}
```
I tried suppressing the circularity error, but because we cache the
first type discovered for a property, this still results in an implicit
any for `x` in the previous example. It just doesn't have an error.
Fixes #35099
|
@typescript-bot perf test this |
|
Heya @sandersn, I've started to run the perf test suite on this PR at b518ea0. You can monitor the build here. Update: The results are in! |
|
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
|
@sandersn Here they are:Comparison Report - master..37827
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
src/compiler/checker.ts
Outdated
| @@ -7596,6 +7596,9 @@ namespace ts { | |||
| } | |||
| return anyType; | |||
| } | |||
| if (containsThisProperty(expression.left, expression.right)) { | |||
There was a problem hiding this comment.
| if (containsThisProperty(expression.left, expression.right)) { | |
| if (containsSameNamedThisProperty(expression.left, expression.right)) { |
(this PR is a bit drafty so I didn't take time to create good names)
|
Users tests don't change much -- there is evidence of better checking in chrome-devtools-frontend, and that's about it. |
|
Of course, with no performance tests for JS, I'm not sure how to get good numbers there. Maybe I can manually compile chrome-devtools-frontend repeatedly. |
|
Longer: It's only called from getWidenedTypeFromAssignmentDeclaration, which is called on BinaryExpressions from getTypeOfVariable[etal] and getTypeOfFunction[etal]. So it applies in TS to assignments like
class C {
constructor() {
this.x = 0;
}
m() {
this.x = this.x.toString();
}
}These are the same rules that operate normally so you'd get the same results for calling an any-typed function like |
src/compiler/checker.ts
Outdated
| @@ -7596,6 +7596,9 @@ namespace ts { | |||
| } | |||
| return anyType; | |||
| } | |||
| if (containsThisProperty(expression.left, expression.right)) { | |||
| return anyType; | |||
There was a problem hiding this comment.
Rather than returning any on all binary expressions with both lhs and rhs references to the same thing, why not elide such binary expressions from the set of expressions whose types are union's together in the first place? This way the type is still calculated based on all other usages, and not quietly any'd?
There was a problem hiding this comment.
This is a per-declaration type. The caller, getWidenedTypeFromAssignmentDeclaration, does something similar to what you suggest, except that it divides declarations into constructor vs non-constructor, then unions all types from the constructor if any exist, otherwise unioning all non-constructor types. See my previous reply to @andrewbranch's question.
I think this is better than dropping circular types because of Andrew's this.x = 0; this.x = this.x.toString() example. Ideally, we would give x: string, but I think x: any is better than x: number.
There was a problem hiding this comment.
Right, but, like, if we never request the type of the declaration, it'll never get flagged as circular. Something like
if (!jsdocType) {
(types || (types = [])).push(((isBinaryExpression(expression) && !containsSameNamedThisProperty(expression.left, expression.right)) || isCallExpression(expression)) ? getInitializerTypeFromAssignmentDeclaration(symbol, resolvedSymbol, expression, kind) : neverType);
}There was a problem hiding this comment.
I think this is better than dropping circular types because of Andrew's this.x = 0; this.x = this.x.toString() example. Ideally, we would give x: string, but I think x: any is better than x: number.
Moreover, we support
var x;
x = 1;
x = x.toString();
x;without error, so why not
this.x = 1;
this.x = this.x.toString();
this.x;if we find that case of value? We'd just need to assign the declared type to the autoType when we see a self-referential assignment like this.
There was a problem hiding this comment.
- Moving containsSameNamedThisProperty earlier is equivalent to returning
neverinsteading ofany. When you do this, though, you get this:
/** @class */
function C() {
this.x = 0;
this.x = this.x.toString();
~~~~~~
!!! error TS2322: Type 'string' is not assignable to type 'number'.
}Which I don't think is good. I'd rather give x: any than x: number.
- I don't understand how you'd make this work with control flow's
autoType, since I don't think it will work consistently across functions. Can you explain?
There was a problem hiding this comment.
autoType is an any-like that's essentially narrowable by assignment in control flow. Just return it here instead of anyType and check out the behavior.
There was a problem hiding this comment.
I'm sceptical that it'll be helpful, but I'm biased by imagining a lot of short methods that would still end up with any. I filed #37900 to track this, since when I tried just return autoType, control flow didn't kick in and it's not obvious why.
|
Re performance: over 10 runs, the new code is 0.1 s faster to compile chrome-devtools-frontend than before, or 0.63%. I don't have significance measurements, but I think that's down to noise. Because of the user test baselines change, I believe chrome-devtool exercises this code pretty well. |
|
Tested performance when using |
sandersn
deleted the
avoid-circular-reference-in-this-property-assignments
branch
To do this, don't check this-property assigments that have the
this-property of the lhs appearing somewhere on the rhs:
I tried suppressing the circularity error, but because we cache the first type discovered for a property, this still results in an implicit any for
xin the previous example. It just doesn't have an error.I'm not sure how expensive the tree walk is, or how much javascript projects will benefit from this change.
Unfortunately, the perf tests won't help with the former, but the user tests should help with the latter.
Fixes #35099