Skip to content

#31711 breaks assignability #31762

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

Closed
sandersn opened this issue Jun 4, 2019 · 7 comments · Fixed by #31802
Closed

#31711 breaks assignability #31762

sandersn opened this issue Jun 4, 2019 · 7 comments · Fixed by #31802
Assignees
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue

Comments

@sandersn
Copy link
Member

sandersn commented Jun 4, 2019

// @strict: true
function g(headerNames: any) {
    let t = [{ hasLineBreak: false, cells: [] }];
    const table = [{cells: headerNames }].concat(t);
}

Expected behavior:
No error.

Actual behavior:
Error on concat(t): { hasLineBreak: boolean, cells: never[] } is not assignable to { cells: any }. hasLineBreak is missing.

Playground Link:

Related Issues:

@sandersn sandersn added the Bug A bug in TypeScript label Jun 4, 2019
@sandersn sandersn added this to the TypeScript 3.6.0 milestone Jun 4, 2019
@sandersn
Copy link
Member Author

sandersn commented Jun 4, 2019

Found in the user tests, in prettier.

@fatcerberus
Copy link

Looks like it got the assignability check backwards, that’s odd.

@sandersn
Copy link
Member Author

sandersn commented Jun 5, 2019

From in-person discussion, I think we are going to take this as a breaking change for 3.6.

@fatcerberus
Copy link

fatcerberus commented Jun 5, 2019

Wait, what's the accepted breaking change? The error mentioned in the OP? Because if so the error message is nonsensical:

{ hasLineBreak: boolean, cells: never[] } is not assignable to { cells: any }. hasLineBreak is missing.

With that wording, it looks like a bug--the apparent bug being that a structural assignability check was done with the source and target swapped. Even if the breakage is acceptable, the error message IMO isn't as it's exactly backwards a.c.t. typical structural typing semantics.

@ahejlsberg
Copy link
Member

The issue here is when to widen the object expression in property access (obj.x) and element access (obj["x"]) expressions. We have been quite inconsistent here. It used to the that we'd always widen obj in a property access and never widen obj in an element access. That's obviously not right as the two are supposed to be synonymous. In #31711 we switched to widening obj only when it is an empty array literal in a property access expression (e.g. [].concat(...)), but that isn't right either.

I've given it some thought, and I think the real solution is to widen obj when the property or element access is (a) the target of an assignment, or (b) a method access in a call expression (i.e. the this of the call). These are the situations in which we are mutating (or possibly mutating) the object and therefore should widen its type.

@ahejlsberg
Copy link
Member

To demonstrate the inconsistency, this is our behavior in 3.5.1 and earlier:

// @strict: true
function g1(headerNames: any) {
    let t = [{ hasLineBreak: false, cells: [] }];
    const table = [{cells: headerNames }].concat(t);  // No error
}

function g2(headerNames: any) {
    let t = [{ hasLineBreak: false, cells: [] }];
    const table = [{cells: headerNames }]["concat"](t);  // Error
}

@fatcerberus
Copy link

fatcerberus commented Jun 6, 2019

@ahejlsberg I’m not actually doubting the type error itself - I’m just confused as to how TypeScript arrives at the conclusion that “(derived) is not assignable to (base)” (per usual structural rules) - even in cases of contravariance it normally gets this right:

// @strictFunctionTypes: true
let f: (x: Derived) => void;
let g: (x: Base) => void;
g = f;
// error because (ultimately) Base is not assignable to Derived

@ahejlsberg ahejlsberg added the Fixed A PR has been merged for this issue label Jun 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants