Skip to content

Fix destructuring control flow analysis #29053

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

Merged
merged 12 commits into from
Dec 19, 2018
Merged

Conversation

ahejlsberg
Copy link
Member

@ahejlsberg ahejlsberg commented Dec 16, 2018

This PR fixes multiple issues related to control flow analysis of destructuring declarations and assignments. Intuitively, for type checking purposes the following should always be true:

  • obj.x is equivalent to obj["x"] (property access vs. element access)
  • let { x } = obj is equivalent to let x = obj.x (destructuring declaration)
  • { x } = obj is equivalent to x = obj.x (destructuring assignment)

However, there were several situations where this was not the case:

function f1(obj: { a: string | undefined }) {
    if (obj.a) {
        obj = { a: undefined };
        let a1 = obj["a"];  // string (Incorrect)
        let a2 = obj.a;  // string | undefined
    }
}

function f2(obj: [number, string] | null[]) {
    let a0 = obj[0];  // number | null
    let a1 = obj[1];  // string | null
    let [b0, b1] = obj;  // string | number | null (Incorrect)
    ([a0, a1] = obj);  // Error (Incorrect)
    if (obj[0] && obj[1]) {
        let c0 = obj[0];  // number
        let c1 = obj[1];  // string
        let [d0, d1] = obj;  // string | number | null (Incorrect)
        ([c0, c1] = obj); // Error (Incorrect)
    }
}

function f3(obj: { a?: number, b?: string }) {
    if (obj.a && obj.b) {
        let { a, b } = obj;  // number, string
        ({ a, b } = obj);  // Error (Incorrect)
    }
}

function f4() {
    let x: boolean;
    ({ x } = 0);
    ({ ["x"]: x } = 0);
    ({ ["x" + ""]: x } = 0);  // No error (Incorrect)
}

This PR fixes all of the above. The PR changes type checking for destructuring declarations and assignments to consistently use getIndexedAccessType to compute the destructured type and getFlowTypeOfReference on a synthetic element access expression to compute the control flow type. Furthermore, the PR properly unifies all handing of property access expressions (obj.x) and element access expressions (obj["x"]) in control flow analysis, and removes the increasingly convoluted logic we needed to handle binding elements from destructuring declarations and object and array literals from destructuring assignments (they now become synthetic element access expressions).

Fixes #26379.
Fixes #28792.

@ahejlsberg ahejlsberg changed the title Fix destructuring control flow Fix destructuring control flow analysis Dec 16, 2018
@@ -41,18 +41,18 @@ tests/cases/conformance/types/tuple/unionsOfTupleTypes1.ts(41,18): error TS2339:
function f1(t1: T1, t2: T2, t3: T3, t4: T4, x: number) {
let [d10, d11, d12] = t1; // string, number
~~~
!!! error TS2493: Tuple type '[string, number]' with length '2' cannot be assigned to tuple with length '3'.
Copy link
Member

Choose a reason for hiding this comment

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

This error message change is a regression, I believe. An explicit tuple length error here is quite desirable.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the new error is just as good, if not better, and certainly more consistent. I always found this particular message strange as there isn't actually a tuple being assigned to. If we think it is important, we could instead improve the message we give when accessing an out-of-range tuple element in getIndexedAccessType.

Copy link
Member

Choose a reason for hiding this comment

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

We explicitly added the tuple length error message to override the message it regressed to. Changing getIndexedAccessType to use it seems kinda appropriate.

Copy link
Member

Choose a reason for hiding this comment

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

I think it would be nice to give a more specific error message if it can still be rewired into getIndexedAccessType:

Tuple type '{0}' of length '{1}' has no element at index '{2}'.

Seems pretty reasonable.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think, like Daniel, that will be better message to avoid issues like #29058.

function getAccessedPropertyName(access: PropertyAccessExpression | ElementAccessExpression): __String | undefined {
return isPropertyAccessExpression(access) ? access.name.escapedText :
function getAccessedPropertyName(access: AccessExpression): __String | undefined {
return access.kind === SyntaxKind.PropertyAccessExpression ? access.name.escapedText :
isStringLiteral(access.argumentExpression) || isNumericLiteral(access.argumentExpression) ? escapeLeadingUnderscores(access.argumentExpression.text) :
Copy link
Member

Choose a reason for hiding this comment

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

While we're here, we should probably change this to isStringOrNumericLiteralLike so NoSubstitutionTemplateLiterals can act like string literals.

Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

The results of getAccessedPropertyName need to be more strongly inspected - it seems like the results are normally checked with a truthiness check, which will be incorrect for properties named by the empty string. It also looks like some explicit tuple length errors have been replaced with some worse errors - we probably shouldn't regress there.

@@ -3350,6 +3351,7 @@ declare namespace ts {
function isObjectLiteralExpression(node: Node): node is ObjectLiteralExpression;
function isPropertyAccessExpression(node: Node): node is PropertyAccessExpression;
function isElementAccessExpression(node: Node): node is ElementAccessExpression;
function isAccessExpression(node: Node): node is AccessExpression;
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need to export this from the public API?

Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

While I think the error message could still usage a change, this looks OK.

@ahejlsberg
Copy link
Member Author

@DanielRosenwasser @weswigham Now with new and improved error message.

@ahejlsberg
Copy link
Member Author

@DanielRosenwasser And now with no public API changes.

@ahejlsberg ahejlsberg merged commit 3e0639a into master Dec 19, 2018
@ahejlsberg ahejlsberg deleted the fixDestructuringControlFlow branch December 19, 2018 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression in type inference for Promise.all() Uncaught implicit any for computed property in destructure
4 participants