Skip to content

Get constraint with this argument of the type parameter for comparisons #21210

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 4 commits into from
May 19, 2018

Conversation

weswigham
Copy link
Member

@weswigham weswigham commented Jan 16, 2018

Fixes #21117
Without doing so, in the given test case, when we check if our inference is assignable to the inference constraint, we get to comparing the methods and see () => MessageList<T> -> () => U, which is not assignable. By getting the constraint with the this type correctly set, we now check () => U -> () => U, which works as expected.

@@ -39,7 +38,6 @@ tests/cases/compiler/fuzzy.ts(25,20): error TS2352: Type '{ oneI: this; }' canno
!!! error TS2322: Types of property 'oneI' are incompatible.
!!! error TS2322: Type 'this' is not assignable to type 'I'.
!!! error TS2322: Type 'C' is not assignable to type 'I'.
!!! error TS2322: Property 'alsoWorks' is missing in type 'C'.
Copy link
Member Author

Choose a reason for hiding this comment

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

This elaboration disappears as the comparison is now cached correctly and is the same comparison done above for the implements constraint check.

@weswigham
Copy link
Member Author

@ahejlsberg As mentioned in-person, I've also added the instantiation to the indexed access branch as discussed because it aught to be required, but there's a soundness issue which prevents the error from occurring in the test without the change:

function swap<
    T,
    TContainer extends { member: T }
>(a: T, b: TContainer): TContainer {
    const tmp = b.member;    
    b.member = a; // This is allowed because we check:
    // T -> TContainer["member"]
    // T -> T
    // Assignment of T to TContainer["member"] erroneously permitted
    // because TContainer["member"] is simplified to a constraint of T
    // The constraint _should_ be "some (unknown) P which extends T"
    // In fact, if you annotate an extra dummy generic that is just that,
    // you'd see the assignment prevented.
    a = tmp;
    return b;
}

const obj = { member: { x: "", y: "" } };
swap({ x: "" }, obj).member.y; // Surprise! It doesn't exist!

This hole prevents the same issue from appearing verbatim with indexed accesses; but because I suspect it could matter in the future I've included the fix anyway.

I think #8474 is actually all about this hole. We could probably fix it nowadays, too (fix element access constraints/assignability with marker type variables, make initialized properties become invariant positions), but it'd likely be a bit breaky.

In any case, I think the equivalent fix for indexed accesses is warranted, since diverging behavior between type variable kinds is undesirable, at least.

@weswigham weswigham force-pushed the constraint-with-this branch from 506eeb1 to 7f49c62 Compare January 18, 2018 23:29
@weswigham
Copy link
Member Author

@mhegazy asked for the perf data for this PR to ensure there's no perf regressions, so here's a 10 iteration comparison from my machine:

Metric master constraint-with-this Delta Best Worst
Compiler - Unions - node (v9.0.0, x64)
Memory used 206,356k (± 0.03%) 206,318k (± 0.03%) -37k (- 0.02%) 206,213k 206,425k
Parse Time 0.74s (± 9.74%) 0.68s (± 1.95%) -0.05s (- 7.31%) 0.65s 0.71s
Bind Time 0.68s (± 6.35%) 0.67s (± 2.46%) -0.02s (- 2.35%) 0.64s 0.72s
Check Time 12.14s (± 3.09%) 11.68s (± 0.72%) -0.46s (- 3.78%) 11.52s 11.90s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 13.56s (± 3.53%) 13.03s (± 0.62%) -0.53s (- 3.92%) 12.89s 13.26s
Monaco - node (v9.0.0, x64)
Memory used 333,386k (± 0.01%) 333,470k (± 0.02%) +84k (+ 0.03%) 333,362k 333,621k
Parse Time 1.69s (± 2.26%) 1.70s (± 4.70%) +0.01s (+ 0.83%) 1.59s 1.88s
Bind Time 0.78s (± 7.79%) 0.76s (± 4.52%) -0.02s (- 2.56%) 0.66s 0.80s
Check Time 3.74s (± 1.83%) 3.63s (± 1.72%) -0.11s (- 3.05%) 3.55s 3.87s
Emit Time 2.83s (± 2.55%) 2.79s (± 1.54%) -0.04s (- 1.55%) 2.71s 2.92s
Total Time 9.04s (± 1.91%) 8.88s (± 1.31%) -0.16s (- 1.77%) 8.66s 9.22s
TFS - node (v9.0.0, x64)
Memory used 289,506k (± 0.01%) 289,487k (± 0.02%) -20k (- 0.01%) 289,314k 289,576k
Parse Time 1.18s (± 2.28%) 1.15s (± 2.15%) -0.03s (- 2.70%) 1.12s 1.24s
Bind Time 0.62s (± 4.58%) 0.59s (± 1.87%) -0.03s (- 4.35%) 0.57s 0.62s
Check Time 3.46s (± 2.53%) 3.35s (± 0.72%) -0.11s (- 3.04%) 3.31s 3.42s
Emit Time 2.29s (± 3.44%) 2.21s (± 1.26%) -0.08s (- 3.62%) 2.16s 2.27s
Total Time 7.55s (± 2.40%) 7.31s (± 0.93%) -0.24s (- 3.22%) 7.22s 7.52s

I don't see much of a difference, so I don't think this'll cause a blanket perf regression.

@mhegazy
Copy link
Contributor

mhegazy commented Jan 19, 2018

LGTM, @ahejlsberg any comments?

@weswigham weswigham force-pushed the constraint-with-this branch from 7f49c62 to e43e1f8 Compare January 19, 2018 21:55
@weswigham
Copy link
Member Author

@ahejlsberg We had a new report of the same bug with a much simpler test case: #24151

I've added it here and synced the branch - we really should accept this.

@weswigham weswigham merged commit 02fe840 into microsoft:master May 19, 2018
@weswigham weswigham deleted the constraint-with-this branch May 19, 2018 01:30
@microsoft microsoft locked and limited conversation to collaborators Jul 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants